bug #10198
closed
Termremoval for OrderedTermVocabularies does not work correctly due to incorrect compareTo and orderIndex decrement
Added by Andreas Müller over 1 year ago.
Updated over 1 year ago.
Description
The termTree in OrderedTermVocabulary identifies entries via compareTo. The compareTo for OrderedTerms in the same vocabulary works via orderIndex.
The removal method first decreases the orderIndex of following terms and therefore creates a duplicate (in terms of comparison) with the term to remove, before calling terms.remove(term). Therefore the wrong term might be removed. This was not so critical as long as there was another bug which prevented the decrement of the the following terms. Since this is fixed and since "equal" terms are not allowed anymore in OrderedTerms this issue becomes critical now.
- Status changed from New to Resolved
- Assignee changed from Andreas Müller to Katja Luther
Can you please review?
This should include checking if term handling (especially removal) in vocabularies still works as expected in TaxEditor. Please also check correctness of orderindex after removal. But have in mind, that orderIndex probably did not work correctly before so there might be missing numbers (or even duplicates) in existing databases.
Please also check the call hierarchy of OrderedTermBase.removeTerm(term). There are 4 methods calling it in cdmlib (any in TaxEditor?). 2 of them tests which are not problematic. 2 of them delete or move methods that you have maybe written. Are they covered by tests (I have not checked)? If not, can you verify that they (still) work as expected?
Andreas Müller wrote in #note-4:
Can you please review?
This should include checking if term handling (especially removal) in vocabularies still works as expected in TaxEditor. Please also check correctness of orderindex after removal. But have in mind, that orderIndex probably did not work correctly before so there might be missing numbers (or even duplicates) in existing databases.
Please also check the call hierarchy of OrderedTermBase.removeTerm(term). There are 4 methods calling it in cdmlib (any in TaxEditor?). 2 of them tests which are not problematic. 2 of them delete or move methods that you have maybe written. Are they covered by tests (I have not checked)? If not, can you verify that they (still) work as expected?
The delete method is covered by a test, but not the move method. I will implement a test.
Katja Luther wrote in #note-5:
Andreas Müller wrote in #note-4:
Can you please review?
This should include checking if term handling (especially removal) in vocabularies still works as expected in TaxEditor. Please also check correctness of orderindex after removal. But have in mind, that orderIndex probably did not work correctly before so there might be missing numbers (or even duplicates) in existing databases.
Please also check the call hierarchy of OrderedTermBase.removeTerm(term). There are 4 methods calling it in cdmlib (any in TaxEditor?). 2 of them tests which are not problematic. 2 of them delete or move methods that you have maybe written. Are they covered by tests (I have not checked)? If not, can you verify that they (still) work as expected?
The delete method is covered by a test, but not the move method. I will implement a test.
Added a test for moveTerm, while implementing this test I recognized that the implementation for move with a position was not correct, this is also fixed with the last commit.
- Assignee changed from Katja Luther to Andreas Müller
Also tested move and delete in TaxEditor.
@AM: Please review the move with position method.
- Status changed from Resolved to Feedback
- Assignee changed from Andreas Müller to Katja Luther
Katja Luther wrote in #note-7:
Also tested move and delete in TaxEditor.
@AM: Please review the move with position method.
I do not really understand the change. Why should TermMovePosition.AFTER be the same as inserting the term above and BEFORE the same as inserting it below.
I also had problems to move terms in TaxEditor. They never ended up where I exptected them to occur though the position where they ended up was according to the DB orderIndex finally. So it is not a UI issue but an issue how BEFORE and AFTER are evaluated IMO.
- Status changed from Feedback to Resolved
- Assignee changed from Katja Luther to Andreas Müller
I debugged now in TaxEditor with rem_conf_am NamedArea voc "Test sorting". Moving "Europe2" below Land3 (eclipse ViewerDropAdapter.LOCATION_AFTER and parent UUID 3c78631 which is Land3) resulted in "Europe2" being above Land3 (below Land2) with orderindex between Land2 and Land3 in the DB. This can not be true.
I will revert the changes and try again.
- Priority changed from Highest to Priority14
- % Done changed from 70 to 90
Moving in TaxEditor now works as expected.
There is still an issue that orderindex misses some numbers in between when moving a term from one position to another in the same vocabulary.
But as the order in general is still correct and as we will very soon move to handle order like in term tree we may neglect this issue for now.
- Status changed from Resolved to Closed
- % Done changed from 90 to 100
Andreas Müller wrote in #note-10:
Moving in TaxEditor now works as expected.
There is still an issue that orderindex misses some numbers in between when moving a term from one position to another in the same vocabulary.
But as the order in general is still correct and as we will very soon move to handle order like in term tree we may neglect this issue for now.
I added SQL queries to #6794#note-48 for further checking issues in existing data. This will be handled within #6974. So we can close this ticket here.
Also available in: Atom
PDF