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

Use confirmation modal when Admin deletes a user #5696

Merged

Conversation

prateekj117
Copy link
Contributor

@prateekj117 prateekj117 commented Jan 11, 2021

Status

Work in progress

Description of Changes

Fixes #4649

Changes proposed in this pull request:
This PR removes the logic of only JS supported confirmation modal when Admin deletes a User and replaces it with the confirmation modal which works without JS.

Testing

How should the reviewer test this PR?
Login as Admin and try deleting a user. Try out the confirmation modal. Should also work with Safest Setting for Tor Browser (i.e, without JS).

Checklist

If you made changes to the server application code:

  • Linting (make lint) and tests (make test) pass in the development container

If you made changes to securedrop-admin:

  • Linting and tests (make -C admin test) pass in the admin development container

If you made non-trivial code changes:

  • I have written a test plan and validated it for this PR

Choose one of the following:

  • I have opened a PR in the docs repo for these changes, or will do so later
  • I would appreciate help with the documentation
  • These changes do not require documentation

@prateekj117
Copy link
Contributor Author

prateekj117 commented Jan 11, 2021

@eloquence @kushaldas @rmol For passing the specific user_id to the confirmation modal, I have used URL Params and added an optional attribute of formaction in _confirmation_modal.html. I am not sure if it is the safest thing or not to pass user_id like this. Though proper checks are implemented in the backend endpoint logic, so I went ahead with this approach. If you think there is a better way to handle this, do let me know and I will make the changes.
If you agree with this approach, let me know and I will add proper tests for the PR as well.

@prateekj117
Copy link
Contributor Author

@eloquence I actually needed a small review on this.

@eloquence
Copy link
Member

@prateekj117 My apologies, the team is currently very busy with the Ubuntu 20.04 migration, but we'll aim to have a look soon.

@rmol
Copy link
Contributor

rmol commented Apr 1, 2021

@prateekj117 I think the way you're passing the user ID is fine. Production traffic is always encrypted. We're already including the user ID in the URL of the next step, the deletion route, e.g. /admin/delete/3.

We could consider using the journalist UUID instead, both here and for the deletion route, but I think the utility of that change would be minimal and it's definitely outside the scope of this PR.

We could alter the list of users to work like the source list on the JI index page, with a checkbox on each row and Edit and Delete buttons at the top, so that the confirmation modals here could work the same as that page, submitting IDs in form parameters, but again, I don't see the need, certainly not in the context of this PR.

I'm curious if @emkll or @zenmonkeykstop have any thoughts, having worked on the recent source deletion changes.

This PR does need a rebase onto latest develop.

@prateekj117
Copy link
Contributor Author

@rmol I will rebase it with the latest develop and also add tests for the same. Was just waiting for the review. If this approach looks good to you, I will add the tests and finalize the PR.

@prateekj117
Copy link
Contributor Author

@rmol Corrected the tests and rebased the PR.

@eloquence
Copy link
Member

@prateekj117 Could you rebase once more to pick up the recent Xenial removal? This should get app tests passing again.

@codecov-commenter
Copy link

codecov-commenter commented May 5, 2021

Codecov Report

Merging #5696 (0d43652) into develop (2882d1e) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop    #5696   +/-   ##
========================================
  Coverage    85.11%   85.11%           
========================================
  Files           55       55           
  Lines         3863     3863           
  Branches       479      479           
========================================
  Hits          3288     3288           
  Misses         463      463           
  Partials       112      112           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1a2494d...0d43652. Read the comment docs.

@prateekj117
Copy link
Contributor Author

@eloquence Ready to Review.

Copy link
Contributor

@zenmonkeykstop zenmonkeykstop left a comment

Choose a reason for hiding this comment

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

Test plan passes, changes look good. Thank you for sticking with this one during some long delays!

@cfm cfm mentioned this pull request Oct 8, 2021
26 tasks
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.

Deleting a user in the Journalist Interface should trigger a confirmation dialog, even without JavaScript
5 participants