Bug #10864

Cannot edit metadata for resources in ArchivesSpace pane

Added by Sarah Romkey over 3 years ago. Updated almost 3 years ago.

Status:FeedbackStart date:02/07/2017
Priority:HighDue date:
Assignee:Kelly Stewart% Done:

0%

Category:-
Target version:1.6.2
Google Code Legacy ID: Pull Request:
Sponsored:No Requires documentation:

Description

When attempting to edit metadata for a resource in the ArchivesSpace pane (e.g. Conditions governing access note) you get a javascript error:

Error: cyclic object value
toJson@http://tinder.conk.archivematica.org/media/js/appraisal_tab.js:1959:11
$HttpProvider/this.defaults.transformRequest<@http://tinder.conk.archivematica.org/media/js/appraisal_tab.js:11427:75
transformData/<@http://tinder.conk.archivematica.org/media/js/appraisal_tab.js:11370:13
forEach@http://tinder.conk.archivematica.org/media/js/appraisal_tab.js:965:12
transformData@http://tinder.conk.archivematica.org/media/js/appraisal_tab.js:11369:4
serverRequest@http://tinder.conk.archivematica.org/media/js/appraisal_tab.js:12199:24
processQueue@http://tinder.conk.archivematica.org/media/js/appraisal_tab.js:17027:29
scheduleProcessQueue/<@http://tinder.conk.archivematica.org/media/js/appraisal_tab.js:17043:28
$RootScopeProvider/this.$get</Scope.prototype.$eval@http://tinder.conk.archivematica.org/media/js/appraisal_tab.js:18326:17
$RootScopeProvider/this.$get</Scope.prototype.$digest@http://tinder.conk.archivematica.org/media/js/appraisal_tab.js:18139:16
$RootScopeProvider/this.$get</Scope.prototype.$apply@http://tinder.conk.archivematica.org/media/js/appraisal_tab.js:18434:14
ngEventHandler/<@http://tinder.conk.archivematica.org/media/js/appraisal_tab.js:26534:18
m.event.dispatch@http://tinder.conk.archivematica.org/media/vendor/jquery.js:4:8497
m.event.add/r.handle@http://tinder.conk.archivematica.org/media/vendor/jquery.js:4:5235

This was tested on tinder.conk. The Appraisal tab says to check the dashboard logs, but that's a red herring, there's nothing being logged there.

Nick, could we please have a developer look at this? It blocks the release.

History

#1 Updated by Nick Wilkinson over 3 years ago

  • Assignee changed from Nick Wilkinson to Holly Becker

Hi Holly, can you please investigate this? This is a high priority issue.

#2 Updated by Holly Becker over 3 years ago

  • Status changed from New to In progress
  • Appraisal tab branch dev/issue-10864-edit-archivesspace-metadata

While investigating this, I found 4 related issues, including the one originally described.

  1. Cyclic error in JS if the element's children have been loaded (original bug)
  2. Error about missing date_expression when updating ArchivesSpace
  3. Access note field not properly hooked up to JS model
  4. Access note not sent because we only handle one note in agentarchives

The first two were fixed by only submitting data that could have been changed by the form, instead of everything from the Javascript representation of the node. 1 was caused by trying to serialize an element where the children had been loaded. Because nodes contain a link to both their parent and their children, when trying to serialize the children it found a cyclic dependency because they pointed back to the original node. 2 was caused because the Javascript 'unset' date values were being set back to the server, even though they weren't valid, which caused an error from ArchivesSpace.

When editing notes, the 'Conditions governing access note' was being silently ignored. Part of this is the Javascript not correctly passing data to be displayed. Part is that agentarchives only handles updating a single note, and will ignore all others. Because we have both a 'note' and an 'access note', the access note is being ignored.

#3 Updated by Holly Becker over 3 years ago

  • Status changed from In progress to Code Review
  • Assignee changed from Holly Becker to Nick Wilkinson

The appraisal tab PR includes fixes for issues 1-3. The agentarchives PR includes a fix for 4.

Archivematica will need an update for the new appraisal tab webpack. Agentarchives will want a new release (likely 0.3.0), and Archivematica will the need the updated dependency.

#4 Updated by Nick Wilkinson over 3 years ago

  • Assignee changed from Nick Wilkinson to Joel Dunham

Hi Joel, can you please CR this?

#5 Updated by José Raddaoui Marín over 3 years ago

The appraisal tab PR looks great.

#6 Updated by Holly Becker over 3 years ago

  • Status changed from Code Review to QA/Review
  • Assignee changed from Joel Dunham to Sarah Romkey

Appraisal tab merged, agentarchives merged & updated on PyPI, Archivematica PR merged to qa/1.6.x & qa/1.x.

#7 Updated by Holly Becker over 3 years ago

  • Status changed from QA/Review to Feedback
  • agentarchives branch: dev/delete-notes-fix

Sarah discovered a bug where the Access note was only updated if the General note was also submitted.

This was caused because the general note is always submitted, even if the content is empty, and agentarchives interpreted this [1] as a request to delete the existing notes [2] even though other notes existed.

The proposed fix would correctly handle adding general and access notes (together or on their own), but deletes any existing notes that are not general or access notes when submitted. That is, the existing list of notes on the archival object is replaced with the ones from the appraisal tab. Is this acceptable behaviour?

A different fix could be to modify the appraisal tab to not submit an empty general note if that field has no content. This would remove the ability to delete a general note from the appraisal tab.

[1] https://github.com/artefactual-labs/agentarchives/blob/v0.3.0/agentarchives/archivesspace/client.py#L209
[2] https://github.com/artefactual-labs/agentarchives/blob/v0.3.0/agentarchives/archivesspace/client.py#L229-L233

#8 Updated by Holly Becker over 3 years ago

Clarification: the existing solution also deletes all note that are not general or access notes when updating the metadata.

If deleting non-access & non-general notes is acceptable, then this is ready for code review. Once merged, a 0.3.1 release is probably the best way to get the changes into Archivematica.

#9 Updated by Sarah Romkey over 3 years ago

  • Assignee changed from Sarah Romkey to Holly Becker
  • Target version changed from Release 1.6 to 1.6.1

I think we'd definitely better not implement any solution that deletes notes. Existing General and Conditions of Access notes should be updated if there is an update, or ignored if there is not.

It seems this would be best left for a fix after the release when we can give this some further work.

#10 Updated by Nick Wilkinson over 3 years ago

  • Status changed from Feedback to In progress
  • Assignee changed from Holly Becker to Nick Wilkinson
  • Priority changed from High to Medium
  • Target version deleted (1.6.1)

For addition to the Resource Scheduling list.

#11 Updated by Nick Wilkinson over 3 years ago

  • Status changed from In progress to Feedback
  • Assignee changed from Nick Wilkinson to Sarah Romkey

Hi Sarah, should we add this to the AM 1.7 release list?

#12 Updated by Sarah Romkey over 3 years ago

  • Status changed from Feedback to In progress
  • Assignee deleted (Sarah Romkey)
  • Target version set to Release 1.7.0

Good thinking, I'll add it now.

#13 Updated by Nick Wilkinson over 3 years ago

  • Assignee set to Joel Dunham

#14 Updated by Joel Dunham almost 3 years ago

  • Assignee changed from Joel Dunham to Nick Wilkinson

Hi Nick,

Holly merged https://github.com/artefactual-labs/agentarchives/pull/38 back in February and Jesús merged Holly's https://github.com/artefactual-labs/agentarchives/pull/40 in June.

It is not clear to me what still needs to be done here. It seems like Sarah's comment at https://projects.artefactual.com/issues/10864#note-9 is indicating that PR #40 should NOT have been merged, yet it has been merged. Do we need to revisit this issue? Holly gave a nice succinct overview of the new behaviour in terms of when notes can be deleted in the GH conversation for PR #40. Is this behaviour acceptable? I'd like to get confirmation that some kind of fix is needed here before delving into this further.

#15 Updated by Nick Wilkinson almost 3 years ago

  • Status changed from In progress to Feedback
  • Assignee changed from Nick Wilkinson to Justin Simpson

Hi Justin, do you have any advice for Joel on this?

#16 Updated by Justin Simpson almost 3 years ago

  • Status changed from Feedback to In progress
  • Assignee changed from Justin Simpson to José Raddaoui Marín
  • Priority changed from Medium to High
  • Target version changed from Release 1.7.0 to 1.6.2

Joel Dunham is correct, I think https://github.com/artefactual-labs/agentarchives/pull/40 should not have been merged.

I tested this on a resource component that had 3 notes (one bibliographic, one general, one conditions governing access). After updating the general and conditions governing access note in the Appraisal tab (the only 2 that show up), in ArchivesSpace the bibliographic note is gone.

This is very bad. At least for anyone who has put a lot of time into adding many notes on a resource in ArchivesSpace.

I think the PR needs to be removed from agentarchives, or re-worked, or something, to ensure that no notes are deleted from ArchivesSpace. Then Archivematica needs to be updated to use the new release of agentarchives.

I would consider this for a 1.6.2 release (i.e. do the work in stable/1.6.x and qa/1.x branches of archivematica).

#17 Updated by Justin Simpson almost 3 years ago

Radda and I have reviewed this, and we have decided that it is not possible to safely support editing existing notes at the present time.

This integration between Archivematica 1.6.x has been tested with ArchivesSpace 1.3.0. For the present time, we are going to allow new notes to be created, but disable the ability to edit existing notes. This will be available in a 1.6.2 bugfix release.

There are two ways that editing notes can accidentally delete content from ArchivesSpace:

1. if there are notes not of type 'general' or type 'conditions governing access'. Any notes of other types get deleted at present.
2. if the general or cga notes ar multipart, or have other data in them. Archivematica is not checking on anything but the basic content of simple notes, so any other data could get overwritten.

#18 Updated by José Raddaoui Marín almost 3 years ago

  • Status changed from In progress to Code Review
  • Assignee changed from José Raddaoui Marín to Nick Wilkinson

Github issue and pull request can be found in https://github.com/artefactual/archivematica/issues/713

#19 Updated by Nick Wilkinson almost 3 years ago

  • Assignee changed from Nick Wilkinson to Jesús García Crespo

Hi Jesús, assigning to you for CR.

#20 Updated by José Raddaoui Marín almost 3 years ago

  • Status changed from Code Review to In progress
  • Assignee changed from Jesús García Crespo to José Raddaoui Marín

Merged in AM stable/1.6.x. Merging to qa/1.x is still a WIP

#21 Updated by José Raddaoui Marín almost 3 years ago

  • Status changed from In progress to Deploy
  • Assignee changed from José Raddaoui Marín to Nick Wilkinson

This is now merged in AM qa/1.x too. Not sure if it need to be deployed to somewhere else than the Bentley instances.

Please notice the new dependency and step in the AM Ansible role to build the front-end assets, the one deploying this changes should verify this is happening without problem:

https://github.com/artefactual-labs/ansible-archivematica-src/commit/cdc48ec2e67ad806df081c9478b6250dbe4fe6ed

The builds are included directly in the stable/1.6.x branch, so no need to check anything if that's being deployed.

#22 Updated by Nick Wilkinson almost 3 years ago

  • Status changed from Deploy to Feedback
  • Assignee changed from Nick Wilkinson to Sara Allain

Hi Sara, assigning to you for next steps. Since the work is in AM qa/1.x, this will get deployed to the Bentley test site the next time we do a deployment there. But I'm passing this to you so you know the fix is ready for any other QA/documentation that needs to happen.

#23 Updated by Sara Allain almost 3 years ago

  • Assignee changed from Sara Allain to Kelly Stewart

Also available in: Atom PDF