5844 deactivated embargoes #6098
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…tch whats in the dassie app. ref: #5844
…ng the embargo. remove an errant pry
…er the deactivated embargoes tab. the edit page now does not reflect that the embargo is gone though. co-authored-by: braydon <braydon.justice@1268456bcltd.ca>
…ing there is an embargo. before this commit if a user simply changed the visibility of a work to "restricted", then the app would think there was an embargo on the work since the embargo visibility was also "restricted". co-authored-by: braydon <braydon.justice@1268456bcltd.ca>
…en deactivated so that it shows up on the "deactivated embargoes" tab of embargoes/index. co-authored-by: braydon <braydon.justice@1268456bcltd.ca>
…ntrolList persists it for us. something is still going on weird though. it appears that the embargo object in the database is different than the embargo metadata thats getting saved on the work. this means that the "past embargoes" section on the embargo/edit page shows different data than the data in the "deactivated embargoes" tab on the embargo/index page. co-authored-by: braydon <braydon.justice@1268456bcltd.ca>
… itself. this change fixes the problem where the updated embargo is different than the embargo that got saved on the work. co-authored-by: braydon <braydon.justice@1268456bcltd.ca>
… we need to find them by their id
…source. embargo_manager lazy loads embargo.
…r when calling methods in the valkyrie_work_indexer
co-authored-by: braydon <braydon.justice@1268456bcltd.ca>
alishaevn
added
notes-valkyrie
…. that work can now be found on #6111.
|
a couple of these specs are flappy. it goes between 6 then 1 fail, but works locally for @sephirothkod and I. |
alishaevn
added a commit
that referenced
this pull request
Jul 14, 2023
alishaevn
force-pushed
the
5844-deactivated-embargoes
branch
from
c3f6caf
to
7d99819
Compare
alishaevn
added a commit
that referenced
this pull request
Jul 17, 2023
co-authored-by: braydon <braydon.justice@1268456bcltd.ca>
bkiahstroud
reviewed
Jul 19, 2023
Co-authored-by: Benjamin Kiah Stroud <32469930+bkiahstroud@users.noreply.github.com>
Co-authored-by: Benjamin Kiah Stroud <32469930+bkiahstroud@users.noreply.github.com>
bkiahstroud
approved these changes
Jul 19, 2023
orangewolf
approved these changes
Jul 20, 2023
Merged
alishaevn
added a commit
that referenced
this pull request
Jul 24, 2023
Idk why we were not able to. the work that was done on #6098 was to be able to deactivate embargoes, and we were able to save them as well. however, when that pr got merged to main, I was no longer able to set an embargo at all in koppie. saving the embargo in app/models/hyrax/resource.rb#embargo= is something we committed at efffe9b, but we then removed it and opted to save the embargo in lib/hyrax/transactions/steps/save.rb#call. however, the `value` argument that gets passed to #embargo= does not have a valid id. just an empty string. so we do not ever set or find the embargo. this change does at least work.
alishaevn
added a commit
that referenced
this pull request
Jul 24, 2023
these were committed when #6098 was accidentally merged before its pipeline was green. the fixes will be handled in another pr. the keyword is "embargogeddon"
This was referenced Jul 25, 2023
alishaevn
added a commit
that referenced
this pull request
Aug 7, 2023
…rb:72 because the failures are unrelated to the #5844 embargo work. both of these specs were failing on main before #6098 was accidentally merged to main with a failing pipeline. in an attempt to get main back to a green pipeline I skipped all failing specs, including these two. ref: 58de8c6. this pr was to fix the broken embargo specs from #6098. these two are still unrelated however and will remain skipped. only in valkyrie, as that is where they are failing. there is work being done on the valkyrie spec suite to fix its several problems. hopefully those fixes will also resolve these issues. as for the riif spec specifically, @orangewolf did some further digging and found: "It should have never been passing because Riiif looks up the file metadata but no file metadata is ever created. Before our valk work, riiif wasn’t working at all. So I do not know how this could have ever passed in Valkyrie land."
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
co-authored-by: @sephirothkod
refs
related to:
Summary
with this commit we will be able to properly deactivate, and reactivate, embargoes that are related to valkyrie backed works
Demo
Screen.Recording.2023-07-12.at.04.27.39.PM.mp4
Guidance for testing, such as acceptance criteria or new user interface behaviors:
Detailed Description
previously, the embargo was getting updated and then saved to the resource (work). however, the version of the embargo that was saved to the database, did not get updated. this led to issues in being able to deactivate them.
this pr proposes saving the embargo to the database only, and then saving the id of the embargo to the resource. now, when
resource.embargois called, it goes to find the appropriately updated embargo in the database. it also means we have one source of truth.we are also bringing over the logic from hydra head that allows us to deactivate properly and return the correct embargo history message.
@samvera/hyrax-code-reviewers