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

Fix share expiration date change #3241

Merged
merged 5 commits into from
Apr 9, 2020
Merged

Conversation

kulmann
Copy link
Member

@kulmann kulmann commented Mar 22, 2020

Description

As pointed out in the linked issue we were not showing errors when saving shares in an invalid state. Instead we were going back to the sidebar panel, leaving the user with the impression that the share was saved successfully. Now we show errors that were raised when saving the share - same style as in the public link editing sidebar panel.

While fixing this I noticed, that the public link editing sidebar panel didn't react properly to forced expiration dates anymore (calendar plugin was not picking up the enforced date and the input field was missing the (required) label). So I fixed that as well and applied the same (code) style as in the collaborators editing sidebar panel.

Related Issue

Motivation and Context

Saving shares should show errors if any were raised.

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:

Open tasks:

  • changelog entry about bugfix

@kulmann kulmann added the Status:Needs-Review Needs review from a maintainer label Mar 22, 2020
@kulmann kulmann self-assigned this Mar 22, 2020
@update-docs
Copy link

update-docs bot commented Mar 22, 2020

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

@kulmann kulmann force-pushed the fix-share-expiration-date-change branch from ce901c6 to 09589c3 Compare March 23, 2020 07:48
@individual-it
Copy link
Member

Acceptance tests are missing. @kulmann do you want the QA-team to add tests?

@kulmann kulmann force-pushed the fix-share-expiration-date-change branch from 09589c3 to ff8fa9e Compare March 23, 2020 18:50
@kulmann
Copy link
Member Author

kulmann commented Mar 23, 2020

Acceptance tests are missing. @kulmann do you want the QA-team to add tests?

@individual-it yes, that would be great, thanks! 👍

@Talank
Copy link
Contributor

Talank commented Mar 25, 2020

@kulmann is it alright if I take over this PR to write some test and make CI happy?

@kulmann
Copy link
Member Author

kulmann commented Mar 25, 2020

@kulmann is it alright if I take over this PR to write some test and make CI happy?

yes, awesome, thanks!

@individual-it individual-it force-pushed the fix-share-expiration-date-change branch from f6a25d2 to cf34879 Compare March 30, 2020 10:19
@individual-it
Copy link
Member

this test (and one similar) fail because it used to be possible to select an invalid date and then click save the share, but now the invalid dates are greyed out

  Scenario: user cannot set an expiry date when creating a public link to a date that is past the enforced max expiry date
    Given the setting "shareapi_default_expire_date" of app "core" has been set to "yes"
    And the setting "shareapi_enforce_expire_date" of app "core" has been set to "yes"
    And user "user1" has logged in using the webUI
    When the user tries to create a new public link for resource "simple-folder" using the webUI with
      | expireDate | +8 |
    Then the user should see an error message on the public link share dialog saying "Cannot set expiration date more than 7 days in the future"
    And user "user1" should not have created any shares

@kulmann Is that is an expected outcome of your changes?

@individual-it individual-it force-pushed the fix-share-expiration-date-change branch from cf34879 to 3e4e6ce Compare March 30, 2020 12:09
@individual-it
Copy link
Member

@kulmann @Talank I've rebased the branch and force-pushed, so if you work on it please get the most recent version before making changes

@kulmann
Copy link
Member Author

kulmann commented Mar 31, 2020

@individual-it @Talank the greyed out invalid dates were like that before. I'm only showing the error alert at the top of the sidebar if the API throws an error on saving.

There could be other errors on saving as well, not only expiration date related. But for expiration dates the error is not reproducible anymore, because as we learned yesterday

a) saving an unchanged invalid date doesn't throw an error anymore
b) selecting a different invalid date is not possible (and already was not before my PR)

The only edge case I can think of is, that an admin shortens the enforced expiration date period AFTER the user has loaded the UI. In that case the user would be able to select an invalid expiration date and receive the error message. IMHO not worth the effort to write tests for it. The more important test in this case is on the API level, not on the UI level.

Copy link
Member Author

@kulmann kulmann left a comment

Choose a reason for hiding this comment

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

@Talank or @individual-it could you please clarify the comment I added? Other than that nice work! 👍

@Talank Talank force-pushed the fix-share-expiration-date-change branch from 1f6811d to 7875993 Compare April 8, 2020 12:09
Copy link
Contributor

@LukasHirt LukasHirt left a comment

Choose a reason for hiding this comment

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

Code-wise LGTM. If tests are completely done then feel free to merge 👍

@kulmann kulmann merged commit d9c4218 into master Apr 9, 2020
@delete-merged-branch delete-merged-branch bot deleted the fix-share-expiration-date-change branch April 9, 2020 08:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
QA:p2 QA:team Status:Needs-Review Needs review from a maintainer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Expiration date doesn't get updated after changing default expiration days in admin sharing setting
4 participants