refine Hyrax::EmbargoSearch behavior to find only enforced embargoes #6241
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
no-reply
force-pushed
the
refine-embargo-search
branch
from
8fb3e58
to
16ceb68
Compare
no-reply
commented
Aug 29, 2023
|
if this looks good. similar changes are needed for Lease. |
|
@alishaevn if this solves the UI problem for #6235, i'd suggest we move the |
alishaevn
reviewed
Aug 29, 2023
|
when you say "they didn't remove the embargo either" what do you mean
specifically?
…On Tue, Aug 29, 2023, 8:43 AM Alisha Evans ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In app/models/concerns/hyrax/solr_document_behavior.rb
<#6241 (comment)>:
> @@ -91,9 +91,9 @@ def creator
end
def visibility
I might be able to look into adding some tests next week, depending on
what my schedule looks like when I return. however,
scientist-softserv/atla-hyku#121
<scientist-softserv/atla-hyku#121> is original
problem I was aiming to solve.
when I tested the rake tasks against hyrax main yesterday, they didn't
return 400 errors, but they didn't remove the embargo either. my intention
was to fix that problem in hyrax, then move it to atla.
—
Reply to this email directly, view it on GitHub
<#6241 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAKMO4FFU7GLBXO4RZ4KAX3XXYE2JANCNFSM6AAAAAA4CNKCI4>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
no-reply
force-pushed
the
refine-embargo-search
branch
from
ae7e880
to
08ebfe0
Compare
|
@alishaevn i added tests for the rake task in 1660516 |
no-reply
force-pushed
the
refine-embargo-search
branch
3 times, most recently
from
b28a252
to
9502ad1
Compare
added 5 commits
August 31, 2023 10:05
since sometime in 2014, `Hyrax::EmbargoSearch` has looked for ALL resources that have ANY embargo date when determining which objects are under embargo. the actual embargo object metadata is indexed (under both AF and Valkyrie), so there's no need to be this agressive about it. instead, find only the objects that are actually under a currently enforced embargo (according to the index). as a first pass at implementing this, we find all the objects that have indexed embargo dates, then drop any that aren't currently being enforced (where the indexed visibility differs from the embargo's presribed visibility). we could probably improve upon this, but this should suit as a first pass.
there has been some question about whether this rake task actually releases expired embargoes. this tests that it does, and ensures that it integrates with the UI correctly by checking against `Hyrax::EmbargoHelper` (which lists enforced embargoes for use by views).
Hyrax counts on the `embargo_history` being present after an embargo is released. move the logic for setting it to `EmbargoManager#release`. since `#release` now involves updating both the `Hyrax::Embargo` and the `Hyrax::AccessControl`, it's necessary to persist both after a release for it to take effect. so far we've avoided having `EmbargoManager` handle persistence, but maybe we want to change that to make this easier to use correctly at this point? ---- this keeps the `nullify` call in `deactivate`, even though it should no longer be needed. it's arguably a good idea if a user manually deletes an embargo to empty the embargo's data---but then a little inconsistent to continue to use that embargo to host the Work's embargo history(?).
no-reply
force-pushed
the
refine-embargo-search
branch
from
9502ad1
to
9d13dfa
Compare
|
just want to bump this to see if we can resolve this issue. |
dlpierce
approved these changes
Sep 6, 2023
Closed
|
I tested this locally and it's working! (ref: this comment) the same will need to be done for leases. |
alishaevn
added a commit
to scientist-softserv/atla-hyku
that referenced
this pull request
Sep 26, 2023
This code is copied from samvera/hyrax#6241 to solve #121. In that PR, the ability to expire embargoes using a rake task works. The copied code has not been included in a release yet. At the time of this writing, September 26, 2023, it is in hyrax main. The latest release is `hyrax-v4.0.0`. This app is currently using hyrax-v3.5.0. However, since the changes being brought in are coming from hyrax main, I'm including the entire files instead of writing decorators.
no-reply
pushed a commit
that referenced
this pull request
Sep 27, 2023
repeats the work in #6241, but with the lease infrastructure. it would still be really nice to flatten these into one concept, but the data layer is still an issue (stored data has different names for functionally identical Embargo and Lease abstractions).
no-reply
pushed a commit
that referenced
this pull request
Oct 26, 2023
repeats the work in #6241, but with the lease infrastructure. it would still be really nice to flatten these into one concept, but the data layer is still an issue (stored data has different names for functionally identical Embargo and Lease abstractions).
no-reply
pushed a commit
that referenced
this pull request
Oct 26, 2023
repeats the work in #6241, but with the lease infrastructure. it would still be really nice to flatten these into one concept, but the data layer is still an issue (stored data has different names for functionally identical Embargo and Lease abstractions).
no-reply
pushed a commit
that referenced
this pull request
Oct 26, 2023
repeats the work in #6241, but with the lease infrastructure. it would still be really nice to flatten these into one concept, but the data layer is still an issue (stored data has different names for functionally identical Embargo and Lease abstractions).
dlpierce
pushed a commit
that referenced
this pull request
Nov 14, 2023
repeats the work in #6241, but with the lease infrastructure. it would still be really nice to flatten these into one concept, but the data layer is still an issue (stored data has different names for functionally identical Embargo and Lease abstractions).
dlpierce
pushed a commit
that referenced
this pull request
Nov 30, 2023
repeats the work in #6241, but with the lease infrastructure. it would still be really nice to flatten these into one concept, but the data layer is still an issue (stored data has different names for functionally identical Embargo and Lease abstractions).
dlpierce
added a commit
that referenced
this pull request
Nov 30, 2023
* refine Hyrax::LeaseService behavior to find only enforced leases repeats the work in #6241, but with the lease infrastructure. it would still be really nice to flatten these into one concept, but the data layer is still an issue (stored data has different names for functionally identical Embargo and Lease abstractions). * fin --------- Co-authored-by: Daniel Pierce <dlpierce@indiana.edu>
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.
since sometime in 2014,
Hyrax::EmbargoSearchhas looked for ALL resources that have ANY embargo date when determining which objects are under embargo. the actual embargo object metadata is indexed (under both AF and Valkyrie), so there's no need to be this aggressive about it.for the UI, find only the objects that are actually under a currently enforced embargo (according to the index). as a first pass at implementing this, we find all the objects that have indexed embargo dates, then drop any that aren't currently being enforced (where the indexed visibility differs from the embargo's prescribed visibility).
we could probably improve upon this, but this should suit as a first pass.
related to ##6235
Changes proposed in this pull request:
Hyrax::EmbargoManagerdefinitionsEmbargoManager#releasesets#embargo_historycorrectly; after running this method, it's necessary to save both the Embargo and the AccessControl to persist the changes.@samvera/hyrax-code-reviewers