Merge lp:~openerp-dev/openobject-server/trunk-orm-resequence-method-xmo into lp:openobject-server
Status: | Needs review |
---|---|
Proposed branch: | lp:~openerp-dev/openobject-server/trunk-orm-resequence-method-xmo |
Merge into: | lp:openobject-server |
Diff against target: |
418 lines (+340/-2) 9 files modified
openerp/osv/orm.py (+94/-0) openerp/tests/addons/test_resequence/__init__.py (+2/-0) openerp/tests/addons/test_resequence/__openerp__.py (+15/-0) openerp/tests/addons/test_resequence/ir.model.access.csv (+2/-0) openerp/tests/addons/test_resequence/models.py (+15/-0) openerp/tests/addons/test_resequence/tests/__init__.py (+13/-0) openerp/tests/addons/test_resequence/tests/test_assumptions.py (+37/-0) openerp/tests/addons/test_resequence/tests/test_resequence.py (+158/-0) openerp/tests/common.py (+4/-2) |
To merge this branch: | bzr merge lp:~openerp-dev/openobject-server/trunk-orm-resequence-method-xmo |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
OpenERP Core Team | Pending | ||
Review via email: mp+167250@code.launchpad.net |
Description of the change
Adds a `resequence(ids, field='sequence')` to BaseModel. This is mostly used in drag&drop of the Kanban and List views, when the object has a sequencing field (which doesn't have to be named `sequence` though that's the most common case).
There are currently a pure-JS implementation in the list view and a basic Python implementation in a controller for the kanban view, both rewrite pretty much all records (not exactly true but close enough).
AL wanted resequencing to limit sequence number changes similar to the GTK client, which implements the behavior in `widget.
How the system works in the GTK client as far as I understood it:
* set_sequence gets the source and destination indexes for the moved record, and currently loaded records are set in `self.model`
* if there are duplicate sequence numbers across loaded models, all of `self.models` is resequenced by first taking existing unique sequence numbers then counting up from the biggest existing number (with an increment of 1).
* if there are no duplicates, find just the sub-sequence affected by the moved record (basically all records between the source and destination), rotate the sequence numbers (à la collections.
This both keeps sequence numbers more stable (especially if records are not moved around much) and optimizes the number of writes (the default length of a list view being 80 records…).
Theoretically, the optimization could be applied as long as there are no duplicates within the affected sub-sequence, but that's not implemented in the GTK client and there's more work to be done to avoid odd effects where the sub-sequence itself has no duplicates but there's a series of duplicates straddling the boundary (consecutive records with the same sequence number, but only the second one is included in the sub-sequence), since read() will order by oid (implicit) after having used _order, there's a risk of "shuffling" records as far as the user can see. So it's been left out for now.
Unmerged revisions
- 4889. By Xavier (Open ERP)
-
[IMP] simplify slicing of records to resequence
- 4888. By Xavier (Open ERP)
-
[MERGE] from trunk
- 4887. By Xavier (Open ERP)
-
[IMP] check that the reseq optimization does indeed reduce the number of writes where it should
- 4886. By Xavier (Open ERP)
-
[IMP] add partial resequencing optimization
- 4885. By Xavier (Open ERP)
-
[ADD] basic resequencing
take all existing unique sequence numbers, set them on provided ids
list, extend with new sequence numbers (from biggest one available) if
there were seq num dupes. - 4884. By Xavier (Open ERP)
-
[TESTS] check resequencing assumption about ordering of read result
- 4883. By Xavier (Open ERP)
-
[FIX] setting and cleanup of cr in TransactionCase
* set cr on current class, not TransactionCase itself, to better
isolate various test cases* delete cr attribute at the end of the test, so things blow up
clearly when setUp was not correctly called (e.g. overridden but did
not call its own parent), otherwise the result is an odd error about
a cursor already closed within the test.
You assume `read()` follows `_order`, which seems good. But this means that you can't choose the `field` parameter in `resequence()`: you make a diff between the given ids and the ids as returned by `read()`.
You raise ValueError on some conditions. If multiple `insert` and `delete` are returned by get_opcodes(), they will reuse the same ops dictionary entry, so the len(ops)!=2 doesn't seem the right way to assert what you want. (You want exactly one insert and one delete.) Maybe you can loop over get_opcodes and directly assign `source` and `dest`... Maybe something like:
>>> source = None
>>> for tag, (i1, _, _, _) in get_opcodes():
... if tag == 'insert':
... source, multiple_sources = i1, source
...
>>> assert source and multiple_sources is None
I think the code would be simpler by having a single if source < dest, without factoring out to and reusing a single slice object.
zip(seq, ids) is funny, I would have thought of zip(ids, seq) instead :)
Otherwise, seems good to me.