EDIT

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.

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)

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.

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.

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.

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?

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.

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?

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.

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)