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

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

Closed
kiranparajuli589 opened this issue Mar 13, 2020 · 3 comments · Fixed by #3241
Assignees
Labels
Type:Bug Something isn't working

Comments

@kiranparajuli589
Copy link
Contributor

kiranparajuli589 commented Mar 13, 2020

Steps to reproduce

  1. Enable setting shareapi_default_expire_date_user_share of app core
  2. Enable setting shareapi_enforce_expire_date_user_share of app core
  3. Set setting shareapi_default_expire_date_user_share of app core to say 10
  4. Create a collaborator share with default expiration date (which is set to enforced default expiration date)
  5. Change setting shareapi_default_expire_date_user_share to say 5
  6. Visit the last created share to find expiration date

Expected behaviour

Expiration date should be changed to cope with changed setting shareapi_default_expire_date_group_share
Note: similar behavior is present in core UI

Actual behaviour

Expiration date remains unchanged!

In Phoenix:
phoe-expiration

In Core:
core-expiration

Server configuration

latest core master
latest phoenix master

@kiranparajuli589 kiranparajuli589 changed the title Expiration date doesn't change after changing default expiration days in admin sharing setting Expiration date doesn't get updated after changing default expiration days in admin sharing setting Mar 13, 2020
@PVince81 PVince81 added the Type:Bug Something isn't working label Mar 16, 2020
@kulmann kulmann closed this as completed Mar 17, 2020
@kulmann kulmann reopened this Mar 17, 2020
@kulmann
Copy link
Member

kulmann commented Mar 17, 2020

sorry for the noise, closed accidentally.

@kulmann kulmann self-assigned this Mar 22, 2020
@kulmann
Copy link
Member

kulmann commented Mar 22, 2020

Thanks for bringing this up. However, the expected behaviour is not what should actually happen. You still made valid points as the Phoenix UI was in other ways not reacting properly regarding enforced expiration dates (see linked PR, #3241).

When changing the enforced expiration date, this only applies for shares which are created or changed from that time onwards. It is not a good idea to change existing shares - think about changing to a wrong number of days by accident and suddenly all shares are irreversibly changed. Still, when saving a share which has an expiration date that doesn't comply with the new forced expiration date span, an error message has to be shown. The backend reacts properly, but Phoenix didn't show the error and instead had a behaviour which left the impression that the share was saved successfully (and in reality it was not saved at all). The PR will change this to showing a proper error message in such a case.

@kiranparajuli589
Copy link
Contributor Author

kiranparajuli589 commented Mar 23, 2020

Thanks for bringing this up. However, the expected behaviour is not what should actually happen. You still made valid points as the Phoenix UI was in other ways not reacting properly regarding enforced expiration dates (see linked PR, #3241).

When changing the enforced expiration date, this only applies for shares which are created or changed from that time onwards. It is not a good idea to change existing shares - think about changing to a wrong number of days by accident and suddenly all shares are irreversibly changed. Still, when saving a share which has an expiration date that doesn't comply with the new forced expiration date span, an error message has to be shown. The backend reacts properly, but Phoenix didn't show the error and instead had a behaviour which left the impression that the share was saved successfully (and in reality it was not saved at all). The PR will change this to showing a proper error message in such a case.

That'd be nice @kulmann. You can ping anyone from the QA team (or me 😄) to add acceptance test!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type:Bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants