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.

Sign up for GitHub

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

Jump to bottom

Setting lease with date in the past has error messaging problems #4845

Closed
jlhardes opened this issue Mar 31, 2021 · 6 comments · Fixed by #4889
Closed

Setting lease with date in the past has error messaging problems #4845

jlhardes opened this issue Mar 31, 2021 · 6 comments · Fixed by #4889
Assignees
@jeremyf
Labels
bug

Comments

@jlhardes
Copy link
Contributor

Descriptive summary

Setting the Visibility of a Work with a Lease that has a date in the past does not work, which is correct. However, the error messaging that shows is not helpful and ends up leaving you somewhat stuck in the Work edit form.

Rationale

If it does not work to set a lease date in the past (which is correct, it should not work that way), the other visibility options (Public, Embargo, Private) should still be available while the error message inside Lease shows that the date must be a future date. It should be possible to either correct the Lease date and then save to apply the lease or select another visibility option and continue editing the work.

Expected behavior

If a Lease date is in the past, the lease is not applied and the other Visibility options are still available while the error message inside Lease shows that the date must be a future date. The flash error message at the top contains text to explain that the Lease date needs to be a date in the future (or the date value is not valid).

Actual behavior

Empty flash error message appears at the top and the Visibility looks like a Lease has been applied (no other visibility options show) and there is an error message that the date must be a future date. User can either correct the date and continue applying the Lease or they have to exit the Work edit form (using menu options or something similar to abandon the edit form without saving) and come back into the Work edit form, where the visibility options are all available, set as they were before editing, and the invalid Lease has not been applied.

Screen Shot 2021-03-31 at 4 34 56 PM

Steps to reproduce the behavior

  1. Edit a Work
  2. Under Visibility, select Lease
  3. Change the date on the lease to a date in the past
  4. Save changes
  5. See empty flash error message and no visibility options except for Lease (which has to have a corrected date to be applied).
@jlhardes jlhardes added the bug label Mar 31, 2021
@no-reply
Copy link
Contributor

no-reply commented Apr 2, 2021

is this different from #4758

@jlhardes
Copy link
Contributor Author

jlhardes commented Apr 2, 2021

I think this might be an update on #4758. The behavior described in that issue is not occurring anymore. Safari on Mac doesn't provide a calendar picker so dates are entered manually. Entering a date in the past manually was producing a stack trace error but now is giving this empty flash error message along with the message with the date field that it must be a future date. The behavior described here is happening across all browsers now (also tried Chrome and Firefox, both of those have calendar pickers for this field). I can't track down what actually changed to address the stack trace error but it looks like #4758 can be closed.

@jeremyf jeremyf self-assigned this Apr 26, 2021
@jeremyf
Copy link
Contributor

jeremyf commented Apr 26, 2021

Testing this requires #4833

@jeremyf
Copy link
Contributor

jeremyf commented Apr 26, 2021

Work log with #4833, when I add a lease date that is a past date, I get the following:

No route matches {:action=>"edit", :controller=>"hyrax/leases", :id=>Hyrax::Forms::FailedSubmissionFormWrapper(#<Hyrax::GenericWorkForm:0x00005566fc825858>), :locale=>:en}, possible unmatched constraints: [:id]

Screen Shot 2021-04-26 at 10 12 51 AM

We are invoking edit_lease_path(f.object) when f.object is not persisted. That indicates some switching logic is in play that echoes the observed problem of not rendering the full UI element for managing the visibility.

jeremyf added a commit that referenced this issue Apr 26, 2021
@jeremyf
Fixing rendering logic for visibility component
5fd6b8a 
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.
jeremyf added a commit that referenced this issue Apr 26, 2021
@jeremyf
Adding rendering of lease related error messages
ac06cee 
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
@jeremyf jeremyf mentioned this issue Apr 26, 2021
Merged
jeremyf added a commit that referenced this issue Apr 26, 2021
@jeremyf
Adding rendering of lease related error messages
90bc2fd 
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
@jeremyf jeremyf mentioned this issue Apr 26, 2021
Merged
no-reply pushed a commit that referenced this issue Apr 26, 2021
@jeremyf
Adding rendering of lease related error messages
ca7f382 
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
jeremyf added a commit that referenced this issue Apr 26, 2021
@jeremyf
Fixing rendering logic for visibility component
b624674 
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.
jeremyf added a commit that referenced this issue Apr 26, 2021
@jeremyf
Adding rendering of lease related error messages
441fd75 
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
jeremyf added a commit that referenced this issue May 4, 2021
@jeremyf
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.

```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.
jeremyf added a commit that referenced this issue May 4, 2021
@jeremyf
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
jeremyf added a commit that referenced this issue May 4, 2021
@jeremyf
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][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
no-reply pushed a commit that referenced this issue May 4, 2021
@jeremyf
Fixing rendering logic for visibility component
840a08f 
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.
no-reply pushed a commit that referenced this issue May 4, 2021
@jeremyf
Adding rendering of lease related error messages
625a1a9 
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
no-reply pushed a commit that referenced this issue May 4, 2021
@jeremyf
Adjusting lease/embargo_enforced helper logic
9d51cb1 
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
@jlhardes
Copy link
Contributor Author

jlhardes commented May 5, 2021

Testing this locally shows this is fixed when trying to use a past date in the Lease field on a new work or for editing an existing work. Nurax-dev is giving the above described response when editing an existing work and a stack trace error when using a past date in the Lease field on a new work. These changes do not seem to have made it to nurax-dev yet.

@jlhardes
Copy link
Contributor Author

jlhardes commented May 5, 2021

All changes have now made it to nurax-dev and this is fixed. A helpful error message appears that the Lease date must be in the future when editing a work or creating a new work and entering a Lease date that is in the past. This is done!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@jeremyf jeremyf

Labels
bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Gh 4845 settinig lease date in past samvera/hyrax
3 participants
@jeremyf @no-reply @jlhardes