samvera / hyrax

Hyrax is a Ruby on Rails Engine built by the Samvera community. Hyrax provides a foundation for creating many different digital repository applications.
http://hyrax.samvera.org/
Apache License 2.0
183 stars 124 forks source link

Previously-set, now inactive Questioning Authority values should not cause issues #5316

Open conorom opened 2 years ago

conorom commented 2 years ago

Descriptive summary

This is essentially a regression of very old issues: https://github.com/samvera-deprecated/curation_concerns/issues/747 https://github.com/samvera-deprecated/curation_concerns/issues/775

Since these commits were added I think the code has mostly been moved around, but there must also be a tweak causing the regression. I'll add it if I see it.

The problem has been around a while. I can confirm it's in 2.9.5 and main

Rationale

It is obviously common for, e.g., new license versions to arrive. In order to encourage editors to use the new values we have the ability to set the old versions (CC v3.0 or whatnot) to inactive in the authority yml file.

Of course, Works/FileSets that have been previously set with now-inactive values should be unaffected by this.

Expected behavior

Any object that has an inactive Questioning Authorities (QA) value set in its metadata should not break (500 error) show/edit pages and should not have any metadata (unintentionally) lost during editing.

Actual behavior

Setting some Questioning Authorities (QA) metadata, then marking the used values inactive causes (at least) two issues: 1) single-value QA dropdown fields (like Rights statement in out-of-the-box Hyrax) will now show the blank entry on the edit page, even though there is still the inactive metadata value set. Clicking save (when editing another field, say) will blank out this single-valued QA value, probably without the editor even noticing. 2) multi-valued QA fields (like License in out-of-the-box Hyrax) will break the edit page with a TypeError (no implicit conversion of String into Array) on this line.

Steps to reproduce the behavior

  1. Start up a vanilla Hyrax development "internal test app"
  2. Create a Work, add a file and accept the deposit agreement
  3. Fill in the minimum metadata, which includes choosing a "Rights statement" value from the dropdown
  4. Under "Additional fields", choose at least one dropdown value for License
  5. Edit the respective YML files, under .internal_test_app\config\authorities, setting the previously-used Rights statement and (at least one of) the License values to active: false
  6. Reload the Work's edit page. It will be broken with a TypeError as described above
  7. Set the inactive value in licenses.yml back to active
  8. Reload the Work's edit page. Note that the Rights statement value is blank. aside: if you edit this line to include_blank: false it will show the first active value in the list, again just as if there was no value set
  9. Open a rails console in the .internal_test_app directory with bundle exec rails console
  10. Enter ActiveFedora::Base.find('999999999').rights_statement, where 999999999 is the Work's NOID and observe that there is a value set for Rights statement
  11. Click "Save changes" on the Work edit page
  12. Repeat step 10 in the Rails console and note that the Rights statement value has been lost.

Related work

Link to related tickets or prior related work here. https://github.com/samvera-deprecated/curation_concerns/issues/747 https://github.com/samvera-deprecated/curation_concerns/pull/767

https://github.com/samvera-deprecated/curation_concerns/issues/775 https://github.com/samvera-deprecated/curation_concerns/pull/812

dolsysmith commented 1 month ago

We've just encountered this bug, and I believe I've identified the problem. html_options[:class] on this line is an array of HTML class names. To fix the error, it seems to be sufficient to add the force-select class to the end of the last string in that array:

html_options[:class][-1] += ' force-select'

With this change, works that bear a QA value that has been marked active: false are editable. (The deprecated value still appears in the dropdown menu for those works, though it does not do so for works without the deprecated value or for newly created works.)