Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve error handling for updating valkyrie collections #5610

Merged
merged 7 commits into from
May 2, 2022

Conversation

elrayle
Copy link
Contributor

@elrayle elrayle commented Apr 28, 2022

Description

When processing an update for a valkyrie collection, the updated collection instance was not being handled correctly when failures occurred.

If the form validation failed, the updates were not processed and after_update was called instead of after_update_error.

If the transactions failed, @collection was set to false by .value_or before it called `after_update_error.

Similar issues were happening with the create process that were fixed in PR #5604.

WAS:

form.validate(collection_params) &&
          @collection = transactions['change_set.update_collection']
                        .call(form)
                        .value_or { return after_update_error }
after_update

Now, the form validation is processed before setting @collection. If a failure occurs, the process is aborted and calls after_update_errors. Likewise, if there is a failure during the transaction process, after_update_errors is called to process the error and redisplay the edit form. @collection is only updated if there are no form or transaction failures.

NOTE: This PR also moves after and before methods for update to private, deprecating the public versions. This is consistent with the pattern used in works and for create collection.

Related work

The approach is based on the same change that was made for works in PR #5448.

This is follow-on work for the changes to the create collection process in PR #5604.

@samvera/hyrax-code-reviewers

@elrayle elrayle marked this pull request as ready for review April 28, 2022 16:51
Copy link
Member

@cjcolvar cjcolvar left a comment

Choose a reason for hiding this comment

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

Looks good. I had a couple questions about what might be slight changes in behavior.

For backward compatibility, errors are returned as simple strings for create and update collections when the collection is an ActiveFedora collection.

For Valkyrie collections, errors are converted to a JSON hash with more information about the response.  This is how works respond when there is an error.  So going forward, it is better to have consistent error processing.
Copy link
Member

@cjcolvar cjcolvar left a comment

Choose a reason for hiding this comment

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

This looks great!

@elrayle elrayle merged commit b5262c3 into main May 2, 2022
@elrayle elrayle deleted the val_col_update branch May 2, 2022 14:14
@elrayle elrayle mentioned this pull request May 3, 2022
@dlpierce dlpierce added the notes-valkyrie Release Notes: Valkyrie specific label Jun 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
notes-valkyrie Release Notes: Valkyrie specific
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants