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

Show who can see archived submissions #3646

Merged
merged 13 commits into from
Dec 1, 2023

Conversation

wes-otf
Copy link
Contributor

@wes-otf wes-otf commented Nov 6, 2023

Closes #3388 & Closes #3389.

This PR adds an indication as to what user roles can see an archived submission based off of existing settings.

Screenshots

With default Hypha settings
Screenshot 2023-11-06 at 12 04 13 PM

After updating settings to allow Staff and Staff Admin to see archives
Screenshot 2023-11-06 at 12 05 02 PM

After updating settings to only allow Staff Admin (not Staff) to see archives
Screenshot 2023-11-06 at 12 03 09 PM

@wes-otf wes-otf marked this pull request as ready for review November 6, 2023 19:56
Copy link
Member

@theskumar theskumar left a comment

Choose a reason for hiding this comment

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

Added notes to make the code more modular and readable.

hypha/apply/funds/views.py Outdated Show resolved Hide resolved
@frjo
Copy link
Contributor

frjo commented Nov 7, 2023

The po and pot files I mostly update in separate PR for practical reasons.

If you update po and pot files in more than one PR you tend to get merge conflicts. No problem keeping them here, good to get them updated.

@wes-otf
Copy link
Contributor Author

wes-otf commented Nov 7, 2023

@theskumar this is great feedback, thanks a ton!

I was originally working to make it more sentence-like (ie. "role, role and role") but wondered how that would impact translations if different linguistics were to come into play. If that might be an issue I can refactor my last commit. Definitely makes sense to have it more modular though.

@wes-otf
Copy link
Contributor Author

wes-otf commented Nov 7, 2023

@frjo Noted with the translation files - would it make more sense to revert them to what's on main and follow up with the altered files? I have translations committed to #3636 that I can revert too and then do all of them at once after it's all merged.

@wes-otf
Copy link
Contributor Author

wes-otf commented Nov 13, 2023

Reverting this back to a draft as I add features from #3389.

@wes-otf wes-otf marked this pull request as draft November 13, 2023 17:28
hypha/apply/funds/views.py Dismissed Show dismissed Hide dismissed
hypha/apply/funds/views.py Dismissed Show dismissed Hide dismissed
hypha/apply/funds/views.py Dismissed Show dismissed Hide dismissed
@wes-otf wes-otf marked this pull request as ready for review November 14, 2023 19:37
@wes-otf
Copy link
Contributor Author

wes-otf commented Nov 14, 2023

@frjo @theskumar I believe this is ready to go. The only piece that is still up for discussion would be the behavior of the 403 Forbidden page when a user without archive access tries to view and archived submission. The options I see are:

  • Redirect the user to the submission dashboard
  • Create a new "Access Denied" page/view that would allow all instances of raise PermissionDenied to be a little more descriptive and clean
  • Leave the existing functionality as is

Let me know your thoughts!

@theskumar
Copy link
Member

@frjo @theskumar I believe this is ready to go. The only piece that is still up for discussion would be the behavior of the 403 Forbidden page when a user without archive access tries to view and archived submission. The options I see are:

  • Redirect the user to the submission dashboard
  • Create a new "Access Denied" page/view that would allow all instances of raise PermissionDenied to be a little more descriptive and clean
  • Leave the existing functionality as is

Let me know your thoughts!

Explicit is better than implicit. I would suggest going with -- Create a new "Access Denied" page/view that would allow all instances of raise PermissionDenied to be a little more descriptive and clean.

@frjo frjo added Type: Enhancement This is an improvement of an existing thing (not a new thing, which would be a feature). Status: Needs testing Tickets that need testing/qa Type: Minor Minor change, used in release drafter labels Nov 23, 2023
@wes-otf
Copy link
Contributor Author

wes-otf commented Nov 28, 2023

@theskumar All sounds good! The functionality for the 403 page is pretty much mirrored from the 404. It can be customized in Wagtail admin and is triggered when any django.core.exceptions.PermissionDenied error is raised. By default, it looks like the following:

Screenshot 2023-11-28 at 8 55 34 AM

With the Wagtail admin prompts:
Screenshot 2023-11-28 at 9 02 15 AM

@wes-otf
Copy link
Contributor Author

wes-otf commented Nov 28, 2023

This was also tested in the archive context (user with staff role attempted to view an archived submission) and worked well.

@wes-otf
Copy link
Contributor Author

wes-otf commented Dec 1, 2023

This was tested by @Techslammer & all worked as intended. The latest changes of the 403 page hadn't been deployed to test but that was a minor change that shouldn't change too much. Can retest with that if you'd prefer @frjo, otherwise I'll mark it as ready.

Have a nice weekend y'all!

@wes-otf wes-otf added Status: Tested - approved for live ✅ and removed Status: Needs testing Tickets that need testing/qa labels Dec 1, 2023
@frjo frjo merged commit ea0b1b4 into main Dec 1, 2023
10 checks passed
@theskumar theskumar deleted the enhancement/show-who-see-archived-submission branch December 5, 2023 04:05
wes-otf added a commit that referenced this pull request May 7, 2024
Closes #3388 & Closes #3389.

This PR adds an indication as to what user roles can see an archived
submission based off of [existing
settings](#3388 (comment)).
wes-otf added a commit that referenced this pull request May 8, 2024
Closes #3388 & Closes #3389.

This PR adds an indication as to what user roles can see an archived
submission based off of [existing
settings](#3388 (comment)).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Tested - approved for live ✅ Type: Enhancement This is an improvement of an existing thing (not a new thing, which would be a feature). Type: Minor Minor change, used in release drafter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Permission improvements of the submission archive feature Make it clear who sees an archived submission
3 participants