Allow work form to display when wings is disabled #5242
Merged
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
When `disable_wings=true`, the `Wings` constant is not defined. As a result, you cannot check directly to see if the metadata adapter is “Wings::Valkyrie::MetadataAdapter”. This check results in a “NameError”. If we check for `disable_wings` first, then it avoids the check of the metadata adapter.
elrayle
commented
Nov 4, 2021
| before do | ||
| allow(Hyrax).to receive_message_chain(:config, :disable_wings).and_return(true) # rubocop:disable RSpec/MessageChain | ||
| hide_const("Wings") # disable_wings=true removes the Wings constant | ||
| end |
There was a problem hiding this comment.
It already has a test using a non-wings adapter, but it doesn't fully simulate disabled wings. When it is disabled, the Wings constant doesn't exist. The new before block simulates disable_wings returning true and the Wings constant not existing.
elrayle
commented
Nov 4, 2021
| it 'prepopulates as empty before save' do | ||
| expect(Hyrax.logger).to receive(:info) | ||
| .with(starting_with("trying to prepopulate a lock token for")) |
There was a problem hiding this comment.
It isn't a change that the logger adds this info message. This expect line just confirms the message was logged.
elrayle
commented
Nov 4, 2021
| self.version = | ||
| case Hyrax.metadata_adapter | ||
| when Wings::Valkyrie::MetadataAdapter | ||
| if Hyrax.config.disable_wings || !Hyrax.metadata_adapter.is_a?(Wings::Valkyrie::MetadataAdapter) |
There was a problem hiding this comment.
The case statement didn't allow for checking disable_wings before trying to use the Wings constant. The change is a simple rewrite of the case into an if to allow for the more complex check. The code that is executed for each case is the same.
dlpierce
approved these changes
Nov 4, 2021
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.
Description
When
Hyrax.config.disable_wings # truetheWingsconstant is not defined. This is the correct behavior, but means that any reference toWingsinappneeds to check ifdisable_wings #truebefore using theWingsconstant.This PR addresses a specific instance in
Hyrax::ResourceFormwhere there was a check to see if the metadata adapter is wings.This PR includes
Wingsconstant so that it provides a failing test for the scenario described in this PRExpected
Dashboard -> Works -> Add new work -> displays the work form
Actual
Dashboard -> Works -> Add new work -> raises an exception
Related Work
There are other places where
Wingsis used without checkingdisable_wings #true. A cursory review indicates that these locations are places not expected to be encountered when using a non-wings adapter for Valkyrie (e.g. actor stack code, AF forms). It is possible that other locations may be uncovered later that would need a similar approach as applied in this PR.@samvera/hyrax-code-reviewers