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

Can now go through wizard with an old license #1539

Conversation

ConnorSheremeta
Copy link
Contributor

@ConnorSheremeta ConnorSheremeta commented Mar 13, 2020

PR #1359

@ualbertalib-bot
Copy link

ualbertalib-bot commented Mar 13, 2020

1 Warning
⚠️ Please add a detailed summary in the description.

Generated by 🚫 Danger


begin
CONTROLLED_VOCABULARIES[:license].send(code)
rescue RuntimeError
Copy link
Contributor

@murny murny Mar 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a way to make this more specific? What error does CONTROLLED_VOCABULARIES[:license].send(code) actually throw when its a bad code in that we want to fallback to try using old_license controlled vocab?

Got similar thinkings as rubocop here around this: https://docs.rubocop.org/en/stable/cops_style/#styleimplicitruntimeerror

If we accidentally had a spelling mistake or something, I believe this would also raise a RuntimeError? this error would then be silenced by this rescue RuntimeError and no one would be any wiser about it, introducing a nasty bug that be hard to debug (since we get no exceptions as we are swallowing them here).

This very well could be valid, just might be extra cautious here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error thrown is specified now.

click_on I18n.t('items.draft.save_and_continue')
click_on I18n.t('items.draft.save_and_deposit_edits')
assert_text I18n.t('items.draft.successful_deposit')

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice job adding a test 👍 .

Curious if we can verify that the edited item on the show page actually has a an old license attached to it?

My worry is this test just checked that we can walk through the wizard. What if the wizard drops the license or fallbacks to a default and just allowed you to save? But if we can assert the final item after its been edited that it still has the correct license (an old license in this case), then we can be sure it all worked successfully 👍

Copy link
Contributor

@murny murny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice work 👍.

Great job figuring away to specify the Exception error

@ConnorSheremeta
Copy link
Contributor Author

ConnorSheremeta commented Mar 30, 2020

Not sure how I requested review from you three right now, its not required as Shane as already approved.

Looks like I cannot remove you three? Did we turn on a new setting on the repo?

@ConnorSheremeta ConnorSheremeta merged commit d1ad4a1 into ualbertalib:integration_postmigration Mar 30, 2020
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