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

T458 license field display #562

Merged
merged 42 commits into from
Sep 5, 2024
Merged

T458 license field display #562

merged 42 commits into from
Sep 5, 2024

Conversation

dolsysmith
Copy link
Contributor

Workflow notes

This branch has been rebased with #300 (PR #555), so the latter should be merged first.

Changes

  • Updated GwWork and GwEtd views to include the license field presenter (making the field visible).
  • Updated the authorities file (license.yml):
    • The deprecated Europeana license value has been set to inactive.
    • A new All rights reserved value has been added, which will display as plain text.
  • Fixed a Hyrax bug whereby works bearing an inactive authority value are not editable.
  • Added a rake task to update all works in the repository bearing the deprecated license value, replacing it with the new value.
  • Added a spec test to validate the visibility of the license field.
  • Minor spec changes:
    • Modified the Bulkrax importer spec to use the TEMP_FILE_BASE environment variable, if present, instead of defaulting to scholarspace-hyrax/tmp.
    • Added values for all required fields to the gw_work factory, and added logic associating a user's abilities with that work. (See note on test coverage below.)

Reviewer instructions

  1. Follow setup instructions on T300 bulkrax8.0.0 #555 (if you haven't already done so).
  2. Verify that all spec tests run correctly.
  3. Verify that on visiting a work's page in the UI, the license field is visible.
  4. Edit a work in the UI and verify that you can change the value in the license field and save the work.
  5. Work through the steps in T300 bulkrax8.0.0 #555 to perform a Bulkrax import, but prior to uploading, edit the metadata.csv file, adding a license column and populating this column with the value All rights reserved.
  6. Confirm that the works created by the Bulkrax importer have the correct value in the license field.
  7. Create some works with the deprecated license value (which will appear in the form as All rights reserved (deprecated), and then run bundle exec rake gwss:replace_license_value (in the appropriate environment). Verify that the license values have been updated for those works to the non-deprecated All rights reserved.

Test coverage

The current spec tests on this branch verify the visibility of the license field. I attempted to write a test for the Edit functionality in the UI, but I can't seem to get that working. A logged in content-admin user can access the Edit form for the work and click the Save changes button, but a) the changes don't seem to persist, and b) the work then becomes inaccessible to the user, even though the visibility on the work itself is still open.

I can't tell if this stems from a Capybara issue, or from an issue with how we're creating test works. I adapted code from the create_dummy_works Rake task for adding a user's ability to a work; this step seemed necessary in order for the logged-in user to be able to access the Edit page. But I note that the works created with FactoryBot lack a value for the state field, which normally would contain a reference to a Fedora object -- could this be a problem?

I haven't tried creating the work through Capybara actions; if the problem lies with FactoryBot, presumably, this approach would fix it. I'm happy to keep working on this, but perhaps @alepbloyd and I can coordinate efforts.

kerchner and others added 30 commits May 8, 2024 18:05
… to bulkrax zip staging directory, but will need to segregate files from each ETD into separate directoroes
…sn't yet an admin set for admin to deposit to
@dolsysmith dolsysmith requested review from alepbloyd and kerchner and removed request for kerchner August 8, 2024 15:07
@alepbloyd
Copy link
Contributor

Just got through these steps - couple of things I noticed, though might not be issues.

  • When I go to create a new work, the options in the license field didn't include "All rights reserved (deprecated)" - just "All rights reserved" (among the other more specific options). I never encountered "All rights reserved (deprecated)", and don't see any works with that value querying through the rails console, so might be misunderstanding this.
  • I was able to add a license field to the metadata.csv for a bulkrax import, and that seemed to work without issue for including the field on the imported works, and was able to see the works and edit them in the UI.
  • Running the replace_license_value task successfully filled in missing license fields with "All rights reserved", so that seems like it worked as well.

Re: tests - I tagged you in this PR for just directly taking the more fully functioned factories from Hyrax as we briefly discussed, so maybe we can take another pass at tests for this once we get that checked/merged.

@dolsysmith
Copy link
Contributor Author

dolsysmith commented Aug 14, 2024

My apologies -- the deprecated field isn't available in the dropdown because it's marked active: false in the authorities YAML file. I should have specified creating those works before switching to the new branch. But since everything else is working, I think we can safely merge (and just verify that the Rake task correctly updates all the deprecated values on the test server).

If we're merging #557 after this one, we'll want to update the seeded works to use the non-deprecated license value.

@alepbloyd
Copy link
Contributor

Sounds good! Yeah, seems fine to merge.

Re: #557 - I'm going to work on it some more and see if I can figure out that problem with editing works, so I'll make that license field change while figuring that out.

@kerchner kerchner self-requested a review August 27, 2024 14:37
@dolsysmith dolsysmith merged commit 509f225 into master Sep 5, 2024
1 check passed
@dolsysmith dolsysmith deleted the t458-license-field-display branch September 5, 2024 15:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants