-
Notifications
You must be signed in to change notification settings - Fork 124
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
6126 deactivated leases #6127
6126 deactivated leases #6127
Conversation
technically the lease work is not part of #5844, however, it will need to updated similarly to embargoes and I do not see a ticket for it. so the work is being done here wile the embargo work is fresh on our minds. co-authored-by: braydon <braydon.justice@1268456bcltd.ca>
co-authored-by: braydon <braydon.justice@1268456bcltd.ca>
…hat we can create and deactivate them too.
… being passed to Hyrax::Transactions::Steps::Save#call
4e6e965
to
6314351
Compare
…r better ux and ui
…rsister#save because we have already done validation as a Valkyrie object. co-authored-by: rob <rob@scientist.com>
af_object.lease.class has the same name as resource.lease.class; however, each class has a different object_id. so a type mismatch happens. the code below coerces the one object into the other. TODO: fix this! co-authored-by: jeremy <jeremy.n.friesen@gmail.com>
…e because valkyrie does not respond to `valid?`
…ide Wings::ActiveFedoraConverter#apply_attributes_to_model
lib/wings/valkyrie/persister.rb
Outdated
@@ -23,15 +23,16 @@ def initialize(adapter:) | |||
# Persists a resource using ActiveFedora | |||
# @param [Valkyrie::Resource] resource | |||
# @return [Valkyrie::Resource] the persisted/updated resource | |||
def save(resource:) | |||
def save(resource:, valid: true) |
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.
Hmm…thinking about this parameter name valid
? It is a bit confusing.
Does it mean "Treat this record as valid?" Or does it mean "validate this record". Looking at how it is later used, it looks like it could be perform_af_validation: false
@@ -292,8 +292,7 @@ | |||
let(:admin_set) { FactoryBot.build(:invalid_hyrax_admin_set) } # Missing title | |||
|
|||
it 'will not call the workflow_importer' do | |||
expect { service.create! }.to raise_error(RuntimeError) | |||
expect(workflow_importer).not_to have_received(:call) | |||
expect { service.create! }.to raise_error(ActiveRecord::RecordNotFound) |
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.
The spec says "will not call the workflow_importer"
but that part was removed. I think we need to restore the expect(workflow_importer).not_to have_received(:call)
as that's the purpose of the spec. Namely to ensure that we're not having side effects when an error is raised.
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 removed it because at the time I was testing it...it was no longer valid. related to the note below.
if I recall correctly...since we weren't validating in Hyrax::Persister#save
, any longer, we did not get the runtime error anymore from Hyrax::AdminSetCreateService#create!
. therefore the workflow importer was called. later down the stacktrace in Sipity::Workflow#activate!
, Sipity::Workflow.find_by!
failed because we didn't have a workflow_id
. that caused the ActiveRecord::RecordNotFound
we're now checking for.
when I spoke to Rob about it, the different error and change in the save
method, he said to change the error we were now expecting. I did not however explain the full background of the spec with him.
now that I'm passing a perform_af_validation
param to save
though...I can pass perform_af_validation: true
inside Hyrax::AdminSetCreateService#create!
.
hopefully that doesn't break another spec. 😩 🤞🏾
note
Tuesday, August 1, 2023
while pairing last week, rob suggested this change. I noticed last night that committing that change made the 7 failing works_controller_behavior_spec’s, pass.
a little bit ago however, I noticed that the change caused spec/services/hyrax/admin_set_create_service_spec.rb:294 to fail.
I’m now unsure whether I should or should not use validate: false in Wings::Valkyrie::Persister#save. or the best practice way forward on making sure that the specs in both files pass…and that we’re doing the right thing.
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.
my proposed change broke several things because Hyrax.persister.save
that gets called inside of Hyrax::AdminSetCreateService#create!
is not the same as Wings::Persister#save
. it instead comes from "/usr/local/bundle/gems/valkyrie-3.0.3/lib/valkyrie/persistence/postgres/persister.rb", 20.
I've reverted my change and updated the assertion.
ref: f992e10
@@ -90,7 +90,7 @@ | |||
expect(manager.lease) | |||
.to have_attributes visibility_after_lease: 'authenticated', | |||
visibility_during_lease: 'open', | |||
lease_expiration_date: an_instance_of(Date), | |||
lease_expiration_date: an_instance_of(DateTime), |
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'm surprised that we're changing the data type. A Date object only includes Month Day Year, and maps to the concept of lease_expiration_date
. Whereas a DateTime includes hour, minute, second.
This seems like a subtle shift but I'd like to know how this came about.
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 changed it because on main
, when you add a lease to a work, it's saved as a DateTime. so I made koppie do the same. idk how the tests weren't failing before. 🤷🏾♀️
[1] pry(main)> ActiveFedora::Base.find('dr26xx36k').lease
=> #<Hydra::AccessControls::Lease id: "0db70bc6-1bd0-4b95-a991-9b4e275b6f33", visibility_during_lease: "open", visibility_after_lease: "restricted", lease_expiration_date: "2023-08-31 00:00:00", lease_history: []>
[2] pry(main)> ActiveFedora::Base.find('dr26xx36k').lease.lease_expiration_date.class
=> DateTime
@@ -362,7 +363,9 @@ class Resource < Valkyrie::Resource | |||
let(:work) { FactoryBot.create(:leased_work) } | |||
|
|||
it 'repopulates the lease' do | |||
expect(converter.convert).to have_attributes(lease_id: work.lease_id) | |||
# the lease does get recreated on the converted item, but with a new id |
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.
Reading this comment, and the following spec, seems like we're not actually testing the assertion of the comment.
Does the converter.convert
have a lease_id
key? Is the corresponding value different from work.lease_id
? That would be helpful to align the spec with your comment.
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 went back to test this and was able to revert the test to what it already was. 🤷🏾♀️
ref: 580d817
… ones. accounting for a work with file set that did not have a lease, then added one. a work that was created with a file set and lease, plus a work that already had a lease and added a file set.
…ts previous expectations by passing `perform_af_validation: true` in `Hyrax::AdminSetCreateService#create!`
…s funny business in Wings::ActiveFedoraConverter#apply_attributes_to_model in order for spec/wings/active_fedora_converter_spec.rb:365 to pass! 🎉 now lets hope it also passes in ci. 😩
…ameter combo to #valkyrie_save, and therfore Persister#save
@@ -25,6 +25,14 @@ def initialize(handler: Hyrax::WorkUploadsHandler) | |||
# @return [Dry::Monads::Result] | |||
def call(obj, uploaded_files: [], file_set_params: []) | |||
if @handler.new(work: obj).add(files: uploaded_files, file_set_params: file_set_params).attach | |||
if obj.lease |
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.
a new file set gets added to the work in this #call
method, which happens after "steps/save.rb". doing this call here correctly puts the lease on new files and existing ones.
# @param [Array<Valkyrie::Resource>] members | ||
# @param [Hyrax::Work] work | ||
# rubocop:disable Metrics/AbcSize, Metrics/MethodLength | ||
def create_or_update_lease_on_members(members, work) |
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.
putting this method in the LeaseManger feels like a better place for it to be than in "transactions/save.rb". it also makes it easier to use when #6131 gets worked on.
…dora_converter.rb
…ices/hyrax/admin_set_create_service_spec.rb:294
…idiculously tricky and I wish we did not have to.
I think this is addressed and I want to get it merged before anything else breaks it =-) if its not, we can alway open up a new ticket for this one small thing
Fixes
Summary
fix deactivating leases in koppie
Guidance for testing, such as acceptance criteria or new user interface behaviors:
Detailed Description
while working on #6098, braydon and I began updating some of the lease code to follow the pattern of the embargo code we were implementing. however, actually fixing the ability to deactivate leases was out of scope for #5844, so I created a ticket and the work was moved here.
some other things to note:
lease_id
attrubutemain
however, we're able to go to "/leases" and see the file set. then click on it and deactivate it@samvera/hyrax-code-reviewers