Project

General

Profile

Actions

bug #10198

closed

Termremoval for OrderedTermVocabularies does not work correctly due to incorrect compareTo and orderIndex decrement

Added by Andreas Müller about 2 months ago. Updated about 1 month ago.

Status:
Closed
Priority:
Priority14
Category:
cdmlib
Target version:
Start date:
Due date:
% Done:

100%

Estimated time:
Severity:
normal
Found in Version:
Tags:

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.


Related issues

Related to EDIT - feature request #6794: Improve term structureIn ProgressAndreas Müller

Actions
Actions #1

Updated by Andreas Müller about 2 months ago

  • Status changed from New to Resolved
Actions #2

Updated by Andreas Müller about 2 months ago

Actions #4

Updated by Andreas Müller about 2 months ago

  • 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?

Actions #5

Updated by Katja Luther about 2 months ago

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.

Actions #6

Updated by Katja Luther about 2 months ago

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.

Actions #7

Updated by Katja Luther about 2 months ago

  • Assignee changed from Katja Luther to Andreas Müller

Also tested move and delete in TaxEditor.
@AM: Please review the move with position method.

Actions #8

Updated by Andreas Müller about 2 months ago

  • 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.

Actions #9

Updated by Andreas Müller about 2 months ago

  • 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.

Actions #10

Updated by Andreas Müller about 2 months ago

  • 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.

Actions #11

Updated by Andreas Müller about 1 month ago

  • 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.

Actions

Also available in: Atom PDF