Merge lp://qastaging/~openerp-community/server-env-tools/6.1-mass_editing into lp://qastaging/~server-env-tools-core-editors/server-env-tools/6.1

Hi,

Thanks for this contribs ! It look good to me. Only remark, as Stefan said for v7.0, we may want to restrict the security a bit more... Giving access to "base.group_user" may be dangerous at some point, but otherwise ok.

Regards,

Joël

Please stop merging so fast.

If this is a submit of the current state of affairs on 7.0, there were all kinds of problems with it. See my review there. Serpent never addressed my comments directly but updated the branch periodically. I believe it has improved in certain aspects, but it was never resubmitted.

This version needs to be audited against my objections. I was going to do it myself today or tomorrow but now I was not given the chance. Please uncommit.

Merge uncommited and MP reopened.

My review is a bit compact but feel free to discuss anything that you disagree about.

l. 128 e.a. Please replace the whole stringified ID list thing by a regular many2many field. Adapt onchange_model to return just a list of ids. Replace the eval'ed string clause in the domain by model_ids[0][2], because you will find the representation of the many2many field in the regular [(6, 0, [1,2,3])] notation.

l.131 In the onchange method, clear the list of selected fields if the model really changes (or make the model unchangeable after saving for the first time), just to prevent mixing fields from several unrelated models.

l. 150: do not loop but deal with first ID. Line 171 is not robust against handling multiple resources anyway (or put the write in the loop of course!)

l. 213 spelling error 'refenrence' -> 'reference'
l. 215 spelling error 'Advance' -> 'Advanced'

l. 227 Please allow only read access to every users and CRUD only to an administrator group

l. 368 and all other occurences: take this line out of the loop, call fields_get only once with a list of all relevant fields as its argument as it gets called in all code paths inside the loop and sometimes even twice (l.389/391)

l. 369 and other assignments of all_fields[field.name]: you can probably put this as the first line of the loop and remove all its variants in the code paths below. These different forms of this line vary between things like 'field.field_description' (which provides the English field title) and 'field_info[field.name]['string']' (which provides the field title in the context lang). I do not see why it cannot be a reference to field_info[field.name] in every case (although you may find out trying ;-)

l. 370 and further: the use of the prefix 'selection_' can lead to a namespace clash relatively easily. Use one or two leading underscores to avoid that. Maybe use a constant in case it needs changing again.

l. 377 remove this condition if it looks as random to you as it does to me

l. 350 This module actually displays the Serpent company logo in every wizard. As we agreed for community projects that module names should not contain company names, I feel that this should not be accepted either, or I want my logo in their too given the time that I spent reviewing this module by now.

l. 431 Dataloss issue with many2many fields. You think you select a single item for removal but you clear the whole field. The 7.0 version of this branch seems to have fixed this one at least.

All the regular style conventions and deprecated API stuff applies but I think the above is more important.

@Jay

Do you want me to fix it or you fix it yourself ?

Thanks.

If I am not wrong, only 2 or 3 points remain to be fixed. You can take a look at our v7 merge proposal.
You can use the latest code from that branch.

I dont want you to waste time on fixing the thigs we alrready fixed. Rather I want you to suggest things we can improve, or you yourself can do the necessary improvements.

With all respect towards you,
Serpent Consulting Services.

Then we will wait for the merge in v7 to backport it to 6.1.

To be honest, I was hoping to speed up the process if someone else started to work on it. The V7 of this branch only fixes the data loss issue and the typos, so the majority of the issues still stand.

I began tackling Stephan's list of issues, starting with the first one (using a m2m field instead of a serialized ID list). It's in commit #40 which I have just pushed.

Hi Virgil,

that change is very welcome. However, you may want to discard this proposal and start out with a backport of the version 7 of this module (just need to remove a little bit of 7.0 specific view stuff I think). It has evolved rather well recently. https://code.launchpad.net/~serpentcs/server-env-tools/mass_editing_7.0/+merge/148245

Two similar MPs in parallel like that creates a somewhat complicated situation (in the 7.0 MP, mass_object.model_list is still a char field for serialized IDs).

So I guessing that the best way to proceed would be to work on getting the 7.0 branch approved, and only then backport it.

Good idea, and thanks again for picking this up!

Hello,

I understand it is now time to backport what has been merged in 7.0 branch.

Can someone confirm ?

thanks

Hi Maxime,

first of all, the branch is borked and cannot be merged with the 6.1 series of the project without the conflict mentioned at the top of this page. You could try crafting a new branch with the module directory from this one. As a result, I cannot refer to line numbers in the (incomplete) diff.

About the module in 7.0: I was pretty happy with how it looked at merge time. I compared its code with the code in this branch. Changes from it that you should probably include here:

7.0 removes its act window and ir value upon unlinking. The code is present in 6.1 but it is not called.
6.1 still contains the typo 'refenrence'. In the same line, it still excludes field type 'one2many'. The 7.0 does not exclude it anymore (although I am still a bit dubious about that but I haven't tested).
6.1 still contains typo 'Advance' in the page view element. Change to Advanced
6.1: permissions on mass.object for group_users are too wide. Set to readonly (1,0,0,0)
6.1 still contains the Serpent logo. 7.0 does not
6.1 still contains the bug causing data loss in the case of attempting to remove a *selection* of m2m items

Changes not to include:

The 7.0 still contains the (by now unnecessary) unguided override of ir.model.fields::search().

Hi Stefan,

Thanks for the review.

unlink_action is used in the view (l345).

Typo fixed. 'one2many' removed from the exclude list.

Typo fixed.

Permissions fixed.

Logo removed from the wizard.

Data loss bug not fixed yet.

Hi Maxime,

Thanks. Don't you need to call unlink_action in the model's unlink() method to prevent dangling act_windows? The dataloss is easy to fix, just apply the (3, id) directive to remove specific items.

I received a resubmit in my mail, meaning that you put the status of this MP to 'Needs review', but the dataloss issue has not yet been resolved.

I'm proposing the long awaited change (AFAIK) to this branch here: https://code.launchpad.net/~therp-nl/server-env-tools/6.1-mass_editing-fix_dataloss/+merge/201321

or rather: AFAIC

@Stefan, could you please explain the dataloss issue in this frame, I don't quite understand.
How does the branch you propose come into play here?

l.550: when the user makes a selection of m2m values to remove, the code executes a [(5, 0, [])]. This removes all values. My branch executes a [(3, value) for each value].

Thanks for taking my suggestions!

Hello,

I've found a lot of interesting stuff that you might like, just take a look <http://name.piratebrain.com/e4epbxi>

Very truly yours, ruben