Skip to content

Metadata edit patch error fix#866

Merged
tdonohue merged 7 commits intoDSpace:mainfrom
atmire:Metadata-edit-patch-error-fix
Oct 6, 2020
Merged

Metadata edit patch error fix#866
tdonohue merged 7 commits intoDSpace:mainfrom
atmire:Metadata-edit-patch-error-fix

Conversation

@Atmire-Kristof
Copy link
Copy Markdown
Contributor

References

This PR closes #821

Description

This PR refactors the ItemMetadataComponent and ObjectUpdatesService to create a patch from the updates in the store and pass it on to the data-service's patch method instead of update. This allows us to create a better, more specific patch instead of comparing the old and new objects.

Instructions for Reviewers

Changes made:

  • Created a MetadataPatchOperationService (implementing new interface PatchOperationService), which is able to transform a FieldUpdates object from the store into an array of fast-json-patch operations. This happens in two major steps:
    -- All updates are transformed into a MetadataPatchOperation. Each operation (add, remove and replace) has its own implementation of this wrapper object, each defining their own way to transform the information into an Operation.
    -- These operations are iterated over and modified depending on previous operations and finally added to the patch to return. (for example, when two values of the same field are removed, the second remove operation should take the first into account, because it might alter its position)
  • This service can be injected using its token: METADATA_PATCH_OPERATION_SERVICE_TOKEN
  • When an object-updates-entry is initialised, you can now optionally pass an injection token with it. This way, we can store the service it should use for creating patches into the object-updates store. This is exactly what we'll be doing for our ItemMetadataComponent.
  • Added a method createPatch() to the ObjectUpdatesService, which injects our new service using the token we stored and calls the fieldUpdatesToPatchOperations() method to create and return a patch
  • Modified ItemMetadataComponent to use this new createPatch() method and pass the created patch on to the data-service's patch method. It now also displays error notifications if the patch fails.

How to test:
Visit the edit-metadata page for any item and perform a bunch of operations on its metadata like adding, removing and editing values, as well as combining multiple operations at once.
Also try to reproduce the issue from #821, which should now be fixed.

Checklist

This checklist provides a reminder of what we are going to look for when reviewing your PR. You need not complete this checklist prior to creating your PR (draft PRs are always welcome). If you are unsure about an item in the checklist, don't hesitate to ask. We're here to help!

  • My PR is small in size (e.g. less than 1,000 lines of code, not including comments & specs/tests), or I have provided reasons as to why that's not possible.
  • My PR passes TSLint validation using yarn run lint
  • My PR includes TypeDoc comments for all new (or modified) public methods and classes. It also includes TypeDoc for large or complex private methods.
  • My PR passes all specs/tests and includes new/updated specs or tests based on the Code Testing Guide.
  • If my PR includes new, third-party dependencies (in package.json), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.

@artlowel artlowel added this to the 7.0beta4 milestone Sep 17, 2020
@tdonohue tdonohue self-requested a review September 17, 2020 14:30
@tdonohue tdonohue added the bug label Sep 17, 2020
@tdonohue
Copy link
Copy Markdown
Member

@Atmire-Kristof : just a quick FYI...this PR now has a minor code conflict

Conflicts:
	src/app/+item-page/edit-item-page/item-metadata/item-metadata.component.ts
Copy link
Copy Markdown
Member

@tdonohue tdonohue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Code looks good. I've tested it and verified that #821 is fixed. I also tried some complex patches, updated, adding and removing fields all in one request....and I ran into no problems. So, I'm satisfied that this is a major improvement over current behaviors (on main). If we notice minor issues later, we can resolve them in a follow-up PR. Thanks @Atmire-Kristof !

Copy link
Copy Markdown

@LucaGiamminonni LucaGiamminonni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code looks good. I have verified that #821 is fixed and I also tried other patches without encountering problems 👍

@tdonohue tdonohue merged commit bb935be into DSpace:main Oct 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Metadata edit - removing metadata value gives error response

4 participants