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/remove user/soft removal #1724

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from
Open

Conversation

ObadaS
Copy link
Collaborator

@ObadaS ObadaS commented Jan 9, 2025

Original PR : #1718 from @OhMaley

A brief description of the purpose of the changes contained in this PR.

Fixes some small issues seen after deploying the delete user feature.

It should fix the following problem:

  • email formatting
  • setting is_active to False on user deletion
  • hide deleted user on competition participant list + add an option to display them

Related PR

#1691 (original)
#1716 (for deployment)

Issues this PR resolves

#1159

Reminder on the hand testing checklist

  • Create a new user
  • Complete its profile with as much information as possible
  • Create an organization
  • Create a competition
  • Create a Queue
  • Add a submission
  • Make the competition and the submission public
  • Log out and log in with another user
  • Take a look at the new user profile
  • Log out and log in with the new user's account
  • Delete the account using the account view
  • Check your email (or the message in the console)
  • Click on the given link
  • Check that admins received an email with information on the user's deletion
  • Check the the deleted user's got a confirmation email
  • Try to log in with the deleted user's account. And fail
  • Log in as another user
  • Check that the competition and submissions of the removed user are still here but without personal data
  • Take a look at the removed user's profile and check that no personal information is displayed

Checklist

  • Code review by me
  • Hand tested by me
  • I'm proud of my work
  • Code review by reviewer
  • Hand tested by reviewer
  • CircleCi tests are passing
  • Ready to merge

@ObadaS
Copy link
Collaborator Author

ObadaS commented Jan 9, 2025

The email that is sent to administrators has the wrong username
image

@@ -140,8 +142,7 @@ def send_user_deletion_notice_to_admin(user):
queues = user.queues.all()
posts = Post.objects.filter(posted_by=user)

mail_subject = f'Notice: user {user.username} removed his account'
message = render_to_string('profiles/emails/template_delete_account_notice.html', {
context = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

To change the username display in the email you can play with this object. You might need to add the receiver name as a new key-value in this dict. Also don't forget to use it in the email template file located as indicated in the line above

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hello, thanks for the tip. I fixed it by adding a new 'deleted_user' key-value and making 'user' empty so that the email sent only says 'Hello'. I found it better since it sends the email to every admins at once.

@ObadaS
Copy link
Collaborator Author

ObadaS commented Jan 10, 2025

It seems like the link that the user gets to delete their accounts does not expire when clicked, which can then be used generate multiple emails sent to admins

@ihsaan-ullah
Copy link
Collaborator

The links in the deletion email are incomplete(e.g. competitions/345 instead of codabench.org/competitions/345)

Screenshot 2025-01-11 at 12 39 16 PM

@Didayolo
Copy link
Member

From Tristan:

Regarding the following points I have not developed them as it would require more time than I have as well as your opinions:

  • change the owner of a queue when the previous owner get his account removed
  • what to do regarding an organization when a user get his account removed. (in case he is the owner, in case he is the last member, in case he is part of an organization and in case the organization has made some submissions)

@OhMaley
Copy link
Collaborator

OhMaley commented Jan 14, 2025

It seems like the link that the user gets to delete their accounts does not expire when clicked, which can then be used generate multiple emails sent to admins

A very naive solution would be to add a check on the is_deleted attribute of the user in the /src/apps/profiles/views.py inside the delete method. This do not create the one time usage mechanism but it can avoid to trigger all the deletion mechanism.

If you want implement the one time usage feature it will probably requires a new model like TokenUsage whose going to be a oneToOne table between a user, a token with a is_used boolean. Some methods to generate a token and some lines of code to check and modify the is_used attribute. Something around those lines.

@OhMaley
Copy link
Collaborator

OhMaley commented Jan 14, 2025

The links in the deletion email are incomplete(e.g. competitions/345 instead of codabench.org/competitions/345)
Screenshot 2025-01-11 at 12 39 16 PM

Indeed it is missing the protocol and the domain in the url.

What we currently have: <a class="item" href="{% url 'profiles:organization_profile' pk=organization.id %}">
What we should have: <a class="item" href="{{ protocol }}://{{ domain }}{% url 'profiles:organization_profile' pk=organization.id %}">

We simply need to add protocol and doamin to the context passed down to the html template, as well as adding those in the html template

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.

4 participants