-
Notifications
You must be signed in to change notification settings - Fork 121
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
Remove depositors access to FileSets in mediated deposit #1207
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a question about child works.
def perform(work, user_key) | ||
# Iterate over ids because reifying objects is slow. | ||
work.member_ids.each do |file_set_id| | ||
RevokeEditJob.perform_now(file_set_id, user_key) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this also act on child works? If so, file_set_id
may cause confusion in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the method in RevokeEditJob
is always called with perform_now
and only ever called here, then move that perform method into a private method on this job and dispense with the extra job.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@darrenleeweber If we do that, that means a single failure in updating one of the members would halt the whole batch. This way, the failure is isolated and can be restarted independently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mjgiarlo I'm not sure what we should do about child works. Should I explicitly include them or exclude them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jcoyne - so your saying the whole thing does not need to be an atomic "transaction"? It's OK if any one of the users is not removed? As it is, I guess this parent job will never fail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@darrenleeweber There is no way in ActiveFedora to use a transaction (mostly because there is no support for transactions in Solr).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jcoyne I suspect that excluding them until we have to analyze the effects and side-effects of that change can be explored. Write up a new issue and focus on FSes here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I'll make some changes. Here's the ticket for child works: #1213
|
||
# @param [String] file_set_id - the identifier of the object to revoke access from | ||
# @param [String] user_key - the user to remove | ||
def perform(file_set_id, user_key) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as above.
ad31e03
to
8341adc
Compare
8341adc
to
57cef5a
Compare
Fixes #1091
@samvera/hyrax-code-reviewers