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

Pronouns #542

Merged
merged 8 commits into from
Oct 8, 2021
Merged

Pronouns #542

merged 8 commits into from
Oct 8, 2021

Conversation

Muirrum
Copy link
Member

@Muirrum Muirrum commented Sep 20, 2021

Add user-settable pronouns to the database User model. These then get concatenated onto the __str__ method of the User model in the same way as nicknames once set, except the pronouns are placed at the end inside of parentheses..

Copy link
Member

@tnurse18 tnurse18 left a comment

Choose a reason for hiding this comment

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

This PR contains several things besides what you mentioned in your comment. I'd recommend breaking this apart so that each change can be reviewed individually. It's typically good practice to keep PRs small and focused. The pronouns feature is also going to need a bit of work here.

@Muirrum
Copy link
Member Author

Muirrum commented Sep 27, 2021

Whoops, not sure how those other commits got in. Will rebase and fix

Allows the user to set their own pronouns and have them displayed across
the DB.
@coveralls
Copy link

coveralls commented Sep 27, 2021

Coverage Status

Coverage increased (+0.0005%) to 93.289% when pulling d1bb519 on Muirrum:pronouns into c9891fd on WPI-LNL:master.

@jmerdich
Copy link
Contributor

jmerdich commented Oct 6, 2021

So, haven't seen any code review in a week so I'll step in. This looks very good, but the main remaining problem here is that people aren't able to edit their own pronouns (permissions in forms are denied by default). See image below.

This can be confusing, as developers typically give themselves admin rights, which bypass all permission checks and therefore doesn't show it as greyed-out, but you definitely want folks to be able to edit this!

image

You'll need to add it here, probably to both "thisisme" and "hasperm". Officers have permission to edit all users' stuff like this, nominally in case of abuse1, but typically it's used because there's a typo or something and they're the one staring at it all the time.

enable=('email', 'first_name', 'last_name', 'addr', 'wpibox', 'phone', 'class_year', 'nickname', 'carrier')

Click to see steps to reproduce this image
  • python3 -m virtualenv env (first time)
  • source env/Scripts/activate (Linux) OR env/Scripts/activate.bat (Windows)
  • pip install -r requirements_debug.txt
  • python manage.py migrate (create database and run migrations)
  • python manage.py createsuperuser
  • python manage.py runserver
  • Login with your superuser account and use it to create a new account
  • Hit "Impersonate" and then try to edit their own account.

Footnotes

  1. Read: someone put an emoji in their nickname back when the database didn't support that and crashed things.

@Muirrum Muirrum requested a review from tnurse18 October 7, 2021 14:24
A difference between upstream and this branch caused two duplicate
migration IDs to be leafs of 0004.
- Remove pronouns from _str_() function
- Remove pronouns from admin panel
- Raised character limit from 13 to 32
Displays beneath group list

Also fix styling on modify/impersonate buttons
The only person who should be able to edit a user's pronouns is the user
themselves.
Copy link
Member

@tnurse18 tnurse18 left a comment

Choose a reason for hiding this comment

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

I think this looks great! Thanks for working on this!

@tnurse18 tnurse18 merged commit 5f459ca into WPI-LNL:master Oct 8, 2021
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