Project

General

Profile

Actions

bug #8874

open

move taxon is reverted when taxon is edited afterwards

Added by Katja Luther almost 3 years ago. Updated almost 2 years ago.

Status:
Resolved
Priority:
Highest
Category:
taxeditor
Target version:
Start date:
Due date:
% Done:

70%

Estimated time:
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 NewKatja Luther

Actions
Actions #1

Updated by Andreas Müller over 2 years ago

  • Target version changed from Release 5.13 to Release 5.14
Actions #2

Updated by Andreas Müller over 2 years ago

  • Target version changed from Release 5.14 to Release 5.15
Actions #3

Updated by Andreas Müller over 2 years ago

  • Target version changed from Release 5.15 to Release 5.18
Actions #4

Updated by Katja Luther about 2 years ago

  • Assignee changed from Katja Luther to Andreas Müller

this should be fixed. please review.

Actions #5

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

Actions #6

Updated by Katja Luther almost 2 years 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.

Actions #7

Updated by Andreas Müller almost 2 years 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.

Actions #8

Updated by Andreas Müller almost 2 years 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)

Actions #9

Updated by Andreas Müller almost 2 years 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.

Actions #10

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

Actions #11

Updated by Katja Luther almost 2 years 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.

Actions #12

Updated by Katja Luther almost 2 years 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.

Actions #13

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

Actions #14

Updated by Katja Luther almost 2 years 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.

Actions #15

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

Actions #16

Updated by Katja Luther almost 2 years 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.

Actions #17

Updated by Katja Luther almost 2 years ago

  • Status changed from Feedback to Closed
Actions #18

Updated by Katja Luther almost 2 years 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)

Actions #19

Updated by Andreas Müller almost 2 years 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.

Actions #20

Updated by Katja Luther almost 2 years ago

Actions

Also available in: Atom PDF