Project

General

Profile

Actions

bug #9578

closed

Multiselect for input of distribution editor creates duplicates

Added by Andreas Müller over 2 years ago. Updated over 2 years ago.

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

100%

Estimated time:
Severity:
normal
Found in Version:

Description

if 1 record select only this record is returned

if 2 records are selected 1 record is duplicated

if 3 records are selected 1 record appears 3x onother is duplicated

etc.


Related issues

Related to EDIT - feature request #8889: Remaining issues for distribution editor - continueIn ProgressKatja Luther

Actions
Related to EDIT - bug #9581: Strange focus behavior in taxon navigator after right mouse clickNewKatja Luther

Actions
Actions #1

Updated by Andreas Müller over 2 years ago

  • Status changed from New to Resolved
  • % Done changed from 0 to 50
Actions #2

Updated by Andreas Müller over 2 years ago

  • Assignee changed from Andreas Müller to Katja Luther

This works now for taxon nodes not sharing duplicates (e.g. for sibblings). It still shows duplicates if e.g. one selects a parent and some of it's children. The later should either not be possible (I don't know if this is easy to implement in the navigator) or it should be handled such that the algorithm should recognize the child duplicates and remove them.

Can you please review my changes and either implement the open issue or create a new ticket?

Actions #3

Updated by Katja Luther over 2 years ago

Andreas Müller wrote:

This works now for taxon nodes not sharing duplicates (e.g. for sibblings). It still shows duplicates if e.g. one selects a parent and some of it's children. The later should either not be possible (I don't know if this is easy to implement in the navigator) or it should be handled such that the algorithm should recognize the child duplicates and remove them.

Can you please review my changes and either implement the open issue or create a new ticket?

Your changes work fine. To forbid the selection of a parent and some of his children is a little bit more difficult, I adapt the algorithm to avoid duplicates of children as a first step, but maybe this is a little bit confusing because the user selected the parent and for example 4 of 6 children but gets all 6 children.

Actions #4

Updated by Katja Luther over 2 years ago

  • Assignee changed from Katja Luther to Andreas Müller

please review my changes.

Actions #5

Updated by Andreas Müller over 2 years ago

  • Assignee changed from Andreas Müller to Katja Luther

Katja Luther wrote:

Andreas Müller wrote:

This works now for taxon nodes not sharing duplicates (e.g. for sibblings). It still shows duplicates if e.g. one selects a parent and some of it's children. The later should either not be possible (I don't know if this is easy to implement in the navigator) or it should be handled such that the algorithm should recognize the child duplicates and remove them.

Can you please review my changes and either implement the open issue or create a new ticket?

Your changes work fine. To forbid the selection of a parent and some of his children is a little bit more difficult, I adapt the algorithm to avoid duplicates of children as a first step, but maybe this is a little bit confusing because the user selected the parent and for example 4 of 6 children but gets all 6 children.

Thats true but not so relevant I think. Selecting parent & children does not make sense anyway so I don't expect many users to do this. If they do it however, they will probably understand why the result is as it is.

Actions #6

Updated by Andreas Müller over 2 years ago

  • Status changed from Resolved to Feedback

Katja Luther wrote:

please review my changes.

Have you tested this? It does not work. And it can't work as parent seem to be handled elsewhere. So the selected children are also added as parents (because they are selected).
This maybe happens on TaxEditor side or elsewhere but for sure not in getTaxonDistributionDTO()

Actions #7

Updated by Katja Luther over 2 years ago

Andreas Müller wrote:

Katja Luther wrote:

please review my changes.

Have you tested this? It does not work. And it can't work as parent seem to be handled elsewhere. So the selected children are also added as parents (because they are selected).
This maybe happens on TaxEditor side or elsewhere but for sure not in getTaxonDistributionDTO()

Yes, I tested it locally. I will test again with nightly.

Actions #8

Updated by Andreas Müller over 2 years ago

Katja Luther wrote:

Andreas Müller wrote:

Katja Luther wrote:

please review my changes.

Have you tested this? It does not work. And it can't work as parent seem to be handled elsewhere. So the selected children are also added as parents (because they are selected).
This maybe happens on TaxEditor side or elsewhere but for sure not in getTaxonDistributionDTO()

Yes, I tested it locally. I will test again with nightly.

Upps, maybe only a server restart is needed as this is cdmlib-side. However, I do not understand the handling of parents in the method. How are they added to the final result?

Actions #9

Updated by Katja Luther over 2 years ago

Andreas Müller wrote:

Katja Luther wrote:

Andreas Müller wrote:

Katja Luther wrote:

please review my changes.

Have you tested this? It does not work. And it can't work as parent seem to be handled elsewhere. So the selected children are also added as parents (because they are selected).
This maybe happens on TaxEditor side or elsewhere but for sure not in getTaxonDistributionDTO()

Yes, I tested it locally. I will test again with nightly.

Upps, maybe only a server restart is needed as this is cdmlib-side. However, I do not understand the handling of parents in the method. How are they added to the final result?

The criteria for getting the children is

        crit.add( Restrictions.like("treeIndex", node.treeIndex()+ "%") );

therefore also the node itself is found.

Actions #10

Updated by Andreas Müller over 2 years ago

Katja Luther wrote:

The criteria for getting the children is

        crit.add( Restrictions.like("treeIndex", node.treeIndex()+ "%") );

therefore also the node itself is found.

Yes, but later there is

results.remove(node);

which removes the parent node. The correct behavior is tested in TaxonNodeDaoHibernateImplTest.testListChildren(). If the result contains the parent, too, we urgently need to fix the method or rename it because this is not the expected behavior for the given method name I guess.

Actions #11

Updated by Katja Luther over 2 years ago

Andreas Müller wrote:

Katja Luther wrote:

The criteria for getting the children is

        crit.add( Restrictions.like("treeIndex", node.treeIndex()+ "%") );

therefore also the node itself is found.

Yes, but later there is

results.remove(node);

which removes the parent node. The correct behavior is tested in TaxonNodeDaoHibernateImplTest.testListChildren(). If the result contains the parent, too, we urgently need to fix the method or rename it because this is not the expected behavior for the given method name I guess.

Sorry it is the method listChildrenOfRecursive, there the node is added to the result:

if (!previousResult.contains(node)){
            previousResult.add(node);
        }
Actions #12

Updated by Andreas Müller over 2 years ago

Actions #13

Updated by Andreas Müller over 2 years ago

Katja Luther wrote:

Sorry it is the method listChildrenOfRecursive, there the node is added to the result:

if (!previousResult.contains(node)){
            previousResult.add(node);
        }

Ok, this is obviously an error in the method with recursive=true and comparator != null as the method returns another collection size then calling it without comparator. I fixed this and added a test.
As the parent is now missing I added it in getTaxonDistributionDTO() instead which IMO is the correct place for adding.

Can you please review? I havn't tested the result yet.

Actions #14

Updated by Andreas Müller over 2 years ago

  • Related to bug #9581: Strange focus behavior in taxon navigator after right mouse click added
Actions #15

Updated by Andreas Müller over 2 years ago

  • % Done changed from 50 to 80

I tested now and it seems to work as expected. Please also review.

Actions #16

Updated by Katja Luther over 2 years ago

  • Status changed from Feedback to Closed
  • Assignee changed from Katja Luther to Andreas Müller

looks fine and works as expected

Actions #17

Updated by Katja Luther over 2 years ago

  • % Done changed from 80 to 100
Actions

Also available in: Atom PDF