Project

General

Profile

bug #9578

Multiselect for input of distribution editor creates duplicates

Added by Andreas Müller 3 months ago. Updated 2 months ago.

Status:
Closed
Priority:
New
Category:
cdmlib
Target version:
Start date:
04/02/2021
Due date:
% Done:

100%

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 - continue In Progress 03/10/2020
Related to Edit - bug #9581: Strange focus behavior in taxon navigator after right mouse click New 04/14/2021

Associated revisions

Revision ba87a40e (diff)
Added by Andreas Müller 3 months ago

fix #9578 remove duplicates from distributionDto service

Revision c60e36b3 (diff)
Added by Andreas Müller 3 months ago

fix #9578 remove duplicates from distributionDto service (complete test)

Revision e5083306 (diff)
Added by Katja Luther 2 months ago

ref #9578: avoid duplicates for parent and child selection

Revision bf69d17d (diff)
Added by Andreas Müller 2 months ago

ref #9578 , ref #8889 fix behavior of TaxonNodeDaoHibernateImpl.listChildrenOf() with recursive=true and comparator

  • current implementation also returns the parent which is semantically not correct and differs from implementation without comparator

Revision 273d91ee (diff)
Added by Andreas Müller 2 months ago

ref #9578 , ref #8889 switch on sorting for recursive=false in TaxonNodeServiceImpl.loadChildNodesOfTaxonNode()

Revision 78be9787 (diff)
Added by Andreas Müller 2 months ago

ref #9578 , ref #8889 add parentNode in getTaxonDistributionDTO() instead and handle openChildren correctly there

Revision ab18e527 (diff)
Added by Andreas Müller 2 months ago

ref #9578 , ref #8889 fix duplicate handling in getTaxonDistributionDTO

History

#1 Updated by Andreas Müller 3 months ago

  • Status changed from New to Resolved
  • % Done changed from 0 to 50

#2 Updated by Andreas Müller 2 months 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?

#3 Updated by Katja Luther 2 months 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.

#4 Updated by Katja Luther 2 months ago

  • Assignee changed from Katja Luther to Andreas Müller

please review my changes.

#5 Updated by Andreas Müller 2 months 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.

#6 Updated by Andreas Müller 2 months 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()

#7 Updated by Katja Luther 2 months 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.

#8 Updated by Andreas Müller 2 months 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?

#9 Updated by Katja Luther 2 months 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.

#10 Updated by Andreas Müller 2 months 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.

#11 Updated by Katja Luther 2 months 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);
        }

#12 Updated by Andreas Müller 2 months ago

#13 Updated by Andreas Müller 2 months 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.

#14 Updated by Andreas Müller 2 months ago

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

#15 Updated by Andreas Müller 2 months ago

  • % Done changed from 50 to 80

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

#16 Updated by Katja Luther 2 months ago

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

looks fine and works as expected

#17 Updated by Katja Luther 2 months ago

  • % Done changed from 80 to 100

Also available in: Atom PDF

Add picture from clipboard (Maximum size: 40 MB)