Project

General

Profile

bug #8874

move taxon is reverted when taxon is edited afterwards

Added by Katja Luther 12 months ago. Updated about 1 month ago.

Status:
Resolved
Priority:
Highest
Category:
taxeditor
Target version:
Start date:
03/05/2020
Due date:
% Done:

70%

Severity:
normal
Found in Version:

Description

mail W.B.:

ein merkwürdiger Bug – ist mir schon mehrfach aufgefallen, aber ich habe ihn nicht so recht dokumentiert. Caryophyllales_spp:
Ich verschiebe ein als doubtful gekennzeichnetes Taxon (Alternanthera eupatorioides Remy) im Baum (über den Kontextdialog) – in diesem Fall aus der Gattung Alternanthera unter das Pseudotaxon _Amaranthaceae excluded. Dort öffne ich’s im Node-Dialog, markiere es als excluded und gebe meinen Kommentar ein.
Wenn ich es jetzt wieder im Name Editor aufmache und dort etwas ändere (z.B. [früher aufgefallen] die doubtful flag entferne oder [jetzt] die sec.-Ref. ändere, dann wird das excluded flag entfernt und das Taxon springt zurück an seinen ursprünglichen Ort. Der Kommentar zu excluded ist noch da, aber das flag ist weg.
Taxon wieder verschoben, Flag wieder gesetzt, Name Editor, Änderung (diesmal das doubtful-flag entfernt) – speichern – Alles gut.


Related issues

Related to Edit - feature request #9420: Harmonize Update- and ImportResult New 01/25/2021

Associated revisions

Revision 136d38e9 (diff)
Added by Katja Luther 5 months ago

ref #8874: add taxonnode to getUpdatedTaxon

History

#1 Updated by Andreas Müller 12 months ago

  • Target version changed from Release 5.13 to Release 5.14

#2 Updated by Andreas Müller 11 months ago

  • Target version changed from Release 5.14 to Release 5.15

#3 Updated by Andreas Müller 8 months ago

  • Target version changed from Release 5.15 to Release 5.18

#4 Updated by Katja Luther 5 months ago

  • Assignee changed from Katja Luther to Andreas Müller

this should be fixed. please review.

#5 Updated by Andreas Müller 3 months ago

  • Assignee changed from Andreas Müller to Katja Luther
  • Target version changed from Release 5.18 to Release 5.19

Katja, do you remember why this ticket was assigned to me? Simply a mistake?
Can we reproduce this behavior?

#6 Updated by Katja Luther about 1 month ago

  • Status changed from New to Resolved
  • Assignee changed from Katja Luther to Andreas Müller

this ticket was solved and assigned to you for review, sorry forgot to set to resolved.

#7 Updated by Andreas Müller about 1 month ago

I think I remember the mail discussion about this issue. There, we discussed that this happens only if the taxon was opened before. Is this correct?
As it differs from the above description "Wenn ich es jetzt wieder im Name Editor aufmache" we should store such information in the ticket, too. This makes it easier to review (and implement) and avoids false "WorksForMe"s.

#8 Updated by Andreas Müller about 1 month ago

It seems to work. However, I do not fully understand, why. Don't the changes try to guarantee that all open taxa related to the move are saved before?
So, obviously they are not saved. Is this what is expected? (I mean it is good if they do not need to be saved, I want to understand how it works)

#9 Updated by Andreas Müller about 1 month ago

I also wonder, why in the code only the UpdateResult part was adapted, but not the ImportResult. Is this specific to UpdateResult?

If not, we should try to remove code redundancy by moving the code

                Taxon taxon = null;

                if (object instanceof Taxon){
                    taxon = HibernateProxyHelper.deproxy(object, Taxon.class);
                }else if (object instanceof Synonym){
                    Synonym syn = HibernateProxyHelper.deproxy(object, Synonym.class);
                    taxon = syn.getAcceptedTaxon();
                }else if (object instanceof TaxonNode){
                    taxon = ((TaxonNode)object).getTaxon() != null? ((TaxonNode)object).getTaxon():null;
                }
                if (taxon != null){
                    taxaToUpdate.add(taxon);
                }

to a separate method.

#10 Updated by Andreas Müller about 1 month ago

  • Status changed from Resolved to Feedback
  • Assignee changed from Andreas Müller to Katja Luther
  • % Done changed from 0 to 70

Another more critical issue related to the code reviewed:

The following code:

for (Map.Entry<String, Integer> object: result.entrySet()){
                Taxon taxon = null;
                if (object instanceof Taxon){

to me looks strange. How can "object" be of type Taxon if it is a map.
I don't think this can work. Has it ever been tested?

#11 Updated by Katja Luther about 1 month ago

Andreas Müller wrote:

Another more critical issue related to the code reviewed:

The following code:

for (Map.Entry<String, Integer> object: result.entrySet()){
                Taxon taxon = null;
                if (object instanceof Taxon){

to me looks strange. How can "object" be of type Taxon if it is a map.
I don't think this can work. Has it ever been tested?

I had a look and this part is nonesense... the updatedRecords only count the updated records.

I would remove the code.

#12 Updated by Katja Luther about 1 month ago

Andreas Müller wrote:

I also wonder, why in the code only the UpdateResult part was adapted, but not the ImportResult. Is this specific to UpdateResult?

If not, we should try to remove code redundancy by moving the code

                Taxon taxon = null;

                if (object instanceof Taxon){
                    taxon = HibernateProxyHelper.deproxy(object, Taxon.class);
                }else if (object instanceof Synonym){
                    Synonym syn = HibernateProxyHelper.deproxy(object, Synonym.class);
                    taxon = syn.getAcceptedTaxon();
                }else if (object instanceof TaxonNode){
                    taxon = ((TaxonNode)object).getTaxon() != null? ((TaxonNode)object).getTaxon():null;
                }
                if (taxon != null){
                    taxaToUpdate.add(taxon);
                }

to a separate method.

The import result does not contains specific information about the updated or new elements therefore I would remove the code as mentioned above.

Maybe we should think about a more detailed ImportResult, but therefore we need to implement it in cdmlib/io.

#13 Updated by Andreas Müller about 1 month ago

Katja Luther wrote:

Andreas Müller wrote:

I also wonder, why in the code only the UpdateResult part was adapted, but not the ImportResult. Is this specific to UpdateResult?

If not, we should try to remove code redundancy by moving the code

                Taxon taxon = null;

                if (object instanceof Taxon){
                    taxon = HibernateProxyHelper.deproxy(object, Taxon.class);
                }else if (object instanceof Synonym){
                    Synonym syn = HibernateProxyHelper.deproxy(object, Synonym.class);
                    taxon = syn.getAcceptedTaxon();
                }else if (object instanceof TaxonNode){
                    taxon = ((TaxonNode)object).getTaxon() != null? ((TaxonNode)object).getTaxon():null;
                }
                if (taxon != null){
                    taxaToUpdate.add(taxon);
                }

to a separate method.

The import result does not contains specific information about the updated or new elements therefore I would remove the code as mentioned above.

By "moving the code" I did not mean "remove" but extract the code to a separate method and call it 2x (so the typical thing to avoid redundancy). Did you check if it is really not used for imports at all? Otherwise we need the ImportResult part and can't simly remove it, or?

#14 Updated by Katja Luther about 1 month ago

Andreas Müller wrote:

Katja Luther wrote:

Andreas Müller wrote:

I also wonder, why in the code only the UpdateResult part was adapted, but not the ImportResult. Is this specific to UpdateResult?

If not, we should try to remove code redundancy by moving the code

                Taxon taxon = null;

                if (object instanceof Taxon){
                    taxon = HibernateProxyHelper.deproxy(object, Taxon.class);
                }else if (object instanceof Synonym){
                    Synonym syn = HibernateProxyHelper.deproxy(object, Synonym.class);
                    taxon = syn.getAcceptedTaxon();
                }else if (object instanceof TaxonNode){
                    taxon = ((TaxonNode)object).getTaxon() != null? ((TaxonNode)object).getTaxon():null;
                }
                if (taxon != null){
                    taxaToUpdate.add(taxon);
                }

to a separate method.

The import result does not contains specific information about the updated or new elements therefore I would remove the code as mentioned above.

By "moving the code" I did not mean "remove" but extract the code to a separate method and call it 2x (so the typical thing to avoid redundancy). Did you check if it is really not used for imports at all? Otherwise we need the ImportResult part and can't simly remove it, or?

I know that you meant move not remove but this method only get the information about which taxa where updated and updates the editors.

The import result only contains informations about the type of elements and how many of them are updated. This information is used for the report but can not be used for updating. As I already mentioned, should we think about a more detailed ImportResult.

#15 Updated by Andreas Müller about 1 month ago

Katja Luther wrote:

Maybe we should think about a more detailed ImportResult, but therefore we need to implement it in cdmlib/io.

Yes, we definitely should unify ImportResult and UpdateResult somehow, e.g. by having a common base class. I think we have talked about this alrady. Can you have a look, if there is a ticket already and crate one if not?
May we discuss this in standup later on?

#16 Updated by Katja Luther about 1 month ago

Andreas Müller wrote:

It seems to work. However, I do not fully understand, why. Don't the changes try to guarantee that all open taxa related to the move are saved before?
So, obviously they are not saved. Is this what is expected? (I mean it is good if they do not need to be saved, I want to understand how it works)

Yes, but after the operation the updated taxa need to be loaded again in the already open editors, otherwise the taxon would overwrite the changes of the serverside operation.

#17 Updated by Katja Luther about 1 month ago

  • Status changed from Feedback to Closed

#18 Updated by Katja Luther about 1 month ago

Andreas Müller wrote:

Katja Luther wrote:

Maybe we should think about a more detailed ImportResult, but therefore we need to implement it in cdmlib/io.

Yes, we definitely should unify ImportResult and UpdateResult somehow, e.g. by having a common base class. I think we have talked about this alrady. Can you have a look, if there is a ticket already and crate one if not?
May we discuss this in standup later on?

There is no ticket handling UpdateResult and ImportResult, actually they do not have the same base class like Update- and DeleteResult. I create a new ticket about this issue. (#9420)

#19 Updated by Andreas Müller about 1 month ago

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

I want to check again what I meant therefore I put it back to review to remind myself to do so.

#20 Updated by Katja Luther about 1 month ago

Also available in: Atom PDF

Add picture from clipboard (Maximum size: 40 MB)