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

Gh 4845 settinig lease date in past #4889

Merged
merged 3 commits into from May 4, 2021

Conversation

jeremyf
Copy link
Contributor

@jeremyf jeremyf commented Apr 26, 2021

Release Notes

Fixes #4845. This fix adds both a meaningful error message and renders the correct form controls when you enter an invalid lease date.

Fixing rendering logic for visibility component

e1fd224

Fixes #4845

Prior to this fix, if we had a new record that either set an embargo or
a lease, we would narrow our rendering of the visibility component to
only a partial related to the embargo or lease.

Given a form that set an embargo (or release date)
And that form had invalid data
When I submitted the form
Then the response form would inidicate there was invalid data
And would render only the portion of the visibility related to
  embargo (or release date if set)

With this change, the form renders the entire visibility component,
letting the user change their visibility options (e.g., go from with
Lease to Public or what have you). However, the error messaging is not
providing insight into the problem; That is to say there's a red flash
error, but the content is empty; and there is no indicator on the lease
field that there existed an error. This warrants further work, and will
be a commit that builds on this change.

Adding rendering of lease related error messages

9511dce

Prior to this commit, the form_visibility_error partial did not
include the errors for the :lease_expiration_date nor the
:visibility_after_lease. Per convention of Hyrax, we place the error
messages on the form and not in close DOM proximity to the HTML field
in which we encountered the error.

This relates to #4845

Adjusting lease/embargo_enforced helper logic

adb14ec

This reflects moving the change from e1fd224 — "Fixing rendering logic
for visibility component" into the helper method. This suggestion comes
from no_reply's review comment, and makes sense. I don't think we
want to push the logic further down the stack, as there's a case
statement that would add complexity, as well as it may have unintended
consequences. The change at this level (e.g., a helper method) makes it
more clear that this is for rendering UI components.

Related to #4845.

@samvera/hyrax-repo-managers

@jeremyf jeremyf changed the title WIP - Gh 4845 settinig lease date in past Gh 4845 settinig lease date in past Apr 26, 2021
@jeremyf jeremyf force-pushed the gh-4845-settinig-lease-date-in-past branch from 65181de to 441fd75 Compare April 26, 2021 19:19
@@ -1,6 +1,6 @@
<% if embargo_enforced?(f.object) %>
<% if embargo_enforced?(f.object) && !f.object.new_record? %>
Copy link
Member

Choose a reason for hiding this comment

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

i'm wondering whether the embargo_enforced? helper shouldn't take the new_record? case into account? then this && could be inlined there and kept out of the view?

the other place this is used is _form_permission.html.erb and, at first glance, i'm guessing that partial will have similar issues in the same cases as this one. it might be fair to say that embargo/lease_enforced? isn't true when the object isn't yet saved.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This makes sense, and a new commit is incoming!

Fixes #4845

Prior to this fix, if we had a new record that either set an embargo or
a lease, we would narrow our rendering of the visibility component to
only a partial related to the embargo or lease.

```gherkin
Given a form that set an embargo (or release date)
And that form had invalid data
When I submitted the form
Then the response form would inidicate there was invalid data
And would render only the portion of the visibility related to
  embargo (or release date if set)
```

With this change, the form renders the entire visibility component,
letting the user change their visibility options (e.g., go from with
Lease to Public or what have you).  However, the error messaging is not
providing insight into the problem; That is to say there's a red flash
error, but the content is empty; and there is no indicator on the lease
field that there existed an error.  This warrants further work, and will
be a commit that builds on this change.
Prior to this commit, the `form_visibility_error` partial did not
include the errors for the `:lease_expiration_date` nor the
`:visibility_after_lease`.  Per convention of Hyrax, we place the error
messages on the form and not in close DOM proximity to the HTML field
in which we encountered the error.

This relates to #4845
This reflects moving the change from e1fd224 — "Fixing rendering logic
for visibility component" into the helper method.  This suggestion comes
from [no_reply's review comment][1], and makes sense.  I don't think we
want to push the logic further down the stack, as there's a case
statement that would add complexity, as well as it may have unintended
consequences.  The change at this level (e.g., a helper method) makes it
more clear that this is for rendering UI components.

Related to #4845.

[1]:https://github.com/samvera/hyrax/pull/4889/files#r625466486
@jeremyf jeremyf force-pushed the gh-4845-settinig-lease-date-in-past branch from 441fd75 to adb14ec Compare May 4, 2021 13:54
Copy link
Member

@no-reply no-reply left a comment

Choose a reason for hiding this comment

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

fantastic! thanks!

@no-reply no-reply merged commit 9d51cb1 into main May 4, 2021
@no-reply no-reply deleted the gh-4845-settinig-lease-date-in-past branch May 4, 2021 15:47
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.

Setting lease with date in the past has error messaging problems
2 participants