Repository navigation
Link IndividualMember to user profile page #2277
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
Open
ertgl
wants to merge
63
commits into
django:main
Choose a base branch
from
ertgl:issue/1738
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
63 commits
Select commit
Hold shift + click to select a range
bb10b9c
Add `bio` field to `accounts.Profile` model
ertgl 2c29158
Apply black to migrations
ertgl 093ed87
Add a nullable one-to-one `user` field linking `IndividualMember` to …
ertgl 8b918c0
Display `IndividualMember` names as links to their profiles when asso…
ertgl 6e90cd5
Fix vertical split of profile name caused by floated image
ertgl 6a2322d
Make the `user` field on `IndividualMemberAdmin` auto-completable
ertgl a55a0f5
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] 9a180f1
Update `UserProfileTests.test_username_is_page_title` to reflect temp…
ertgl a352eda
Add profile link tests for current and former individual members
ertgl 36b3f90
Add admin action to send account invite mail to individual members
ertgl b7ad21f
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] da706a4
Fix Flake8 E501 (line too long) errors
ertgl d4d8873
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] b918122
Introduce several improvements (please see the commit message for det…
ertgl e46a504
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] f1a4781
Fix `ngettext` usages to correctly handle singular forms for all lang…
ertgl 20117b6
Add management command `send_individual_member_account_invite_mails`
ertgl 84c0826
Add management command `link_individual_members_to_users_by_email`
ertgl bc0467e
Add migration that links individual members to users by email
ertgl 7365e8b
Add tests for displaying user bio in profile
ertgl 46cb8cd
Convert `contrib.django.forms` module into an app for testing
ertgl a69e9e1
Add note to describe why we need value normalization in the `BoundFie…
ertgl 85c211d
Add tests for `BoundFieldWithCharacterCounter` class
ertgl 65950f7
Add tests for `edit_profile` view
ertgl 9fc7fd1
Add test for `IndividualMember.match_and_set_users_by_email` classmethod
ertgl bb963f6
Add test for `IndividualMember.send_account_invite_mails` classmethod
ertgl 2588d94
Add test for `IndividualMember.send_account_invite_mail` method
ertgl 94ba067
Fix Flake8 F541 (f-string is missing placeholders) error
ertgl 81d7137
Add test to verify `IndividualMember.send_account_invite_mails` preve…
ertgl db9a7dd
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] aa9d1cb
Reduce resource usage of some tests
ertgl f37e3f8
Add note to account-invite mail about matching GitHub username for Tr…
ertgl ef54d5b
Update subject of Individual Membership account-invite mail
ertgl b956bfb
Introduce `get_trac_username` function (please see the commit message…
ertgl f922710
Rename `get_trac_username` to `get_user_trac_username`
ertgl a0f2d47
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] b91a2a0
Fix Flake8 F401 (imported but unused) error
ertgl 26eed00
Reduce resource usage of `IndividualMemberTransactionTests.test_send_…
ertgl c9cc83e
Add tests to ensure overriding user's Trac username works for every t…
ertgl a8984d5
Remove the 'noreply' sender address from the individual member accoun…
ertgl 4725cfa
Format code
ertgl 5b67032
Add tests for management command `send_individual_member_account_invi…
ertgl a42b481
Remove unnecessary test `test_trac_username_overrides_user_username`
ertgl 58ed3bc
Fix test `BoundFieldWithCharacterCounterTests.test_characters_remaini…
ertgl 29c77ed
Prevent rendering Trac stats for a user when the username is used by …
ertgl e13bd18
Fix Flake8 F401 (imported but unused) error
ertgl f314007
Rename `check_if_trac_username_is_overridden_for_another_user` to `ch…
ertgl 86d44c4
Move code block inside `if` to prevent unnecessary execution
ertgl c3ae853
Add tests for `ProfileAdminForm`
ertgl 38f3ca3
Prevent unnecessary decrease of test coverage rate in the `Individual…
ertgl ea1ee73
Add tests for the management command `send_individual_member_account_…
ertgl 6b0d2c3
Add tests for `IndividualMemberAdmin`
ertgl dd4e5d8
Add temporary tests for the migration `members.0012`
ertgl 511c922
Add tests for the management command `link_individual_members_to_user…
ertgl 32a44ed
Improve tests for the management command `send_individual_member_acco…
ertgl e77cf23
Use fixed db records number instead of randint in some tests
ertgl c50371e
Add note that the migration test can be removed after deployment
ertgl d104d56
Improve `accounts.forms.ProfileForm.bio` input placeholder
ertgl 9ce978b
Remove unnecessary whitespaces from `UserProfileUpdateFormTests.test_…
ertgl 9c78453
Remove the spacing workaround from the `.user-info .avatar` CSS class
ertgl 7402d6b
Genericize 'profile edit' microcopy
ertgl af5df59
Harden fragile test `IndividualMemberTransactionTests.test_send_accou…
ertgl bf4504c
Introduce `TracAccount` model and support multi-account Trac stats
ertgl File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,52 @@ | ||
| from django import forms | ||
| from django.contrib import admin | ||
|
|
||
| from .forms import ProfileForm | ||
| from .models import Profile, TracAccount | ||
|
|
||
|
|
||
| class ProfileAdminForm(forms.ModelForm): | ||
| class Meta: | ||
| model = Profile | ||
| fields = "__all__" | ||
|
|
||
| def __init__(self, *args, **kwargs): | ||
| super().__init__(*args, **kwargs) | ||
| self.fields["bio"].widget.attrs["maxlength"] = ProfileForm.base_fields[ | ||
| "bio" | ||
| ].max_length | ||
| self.fields["bio"].help_text = ProfileForm.base_fields["bio"].help_text | ||
|
|
||
|
|
||
| @admin.register(Profile) | ||
| class ProfileAdmin(admin.ModelAdmin): | ||
| list_display = [ | ||
| "user__username", | ||
| "name", | ||
| ] | ||
| list_select_related = ["user"] | ||
| search_fields = ["user__username", "name"] | ||
| form = ProfileAdminForm | ||
| autocomplete_fields = ["user"] | ||
|
|
||
|
|
||
| class TracAccountAdminForm(forms.ModelForm): | ||
| class Meta: | ||
| model = TracAccount | ||
| fields = "__all__" | ||
|
|
||
| def __init__(self, *args, **kwargs): | ||
| super().__init__(*args, **kwargs) | ||
| self.fields["username"].widget.attrs["autocomplete"] = "off" | ||
|
|
||
|
|
||
| @admin.register(TracAccount) | ||
| class TracAccountAdmin(admin.ModelAdmin): | ||
| list_display = [ | ||
| "user__username", | ||
| "username", | ||
| ] | ||
| list_select_related = ["user"] | ||
| form = TracAccountAdminForm | ||
| search_fields = ["user__username", "username"] | ||
| autocomplete_fields = ["user"] |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| # Generated by Django 5.2.7 on 2025-10-19 01:39 | ||
|
|
||
| from django.db import migrations, models | ||
|
|
||
|
|
||
| class Migration(migrations.Migration): | ||
|
|
||
| dependencies = [ | ||
| ("accounts", "0002_migrate_sha1_passwords"), | ||
| ] | ||
|
|
||
| operations = [ | ||
| migrations.AddField( | ||
| model_name="profile", | ||
| name="bio", | ||
| field=models.TextField(blank=True), | ||
| ), | ||
| ] |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,20 @@ | ||
| # Generated by Django 5.2.7 on 2025-10-22 23:07 | ||
|
|
||
| from django.db import migrations, models | ||
|
|
||
|
|
||
| class Migration(migrations.Migration): | ||
|
|
||
| dependencies = [ | ||
| ("accounts", "0003_profile_bio"), | ||
| ] | ||
|
|
||
| operations = [ | ||
| migrations.AddField( | ||
| model_name="profile", | ||
| name="trac_username", | ||
| field=models.CharField( | ||
| blank=True, db_index=True, default="", max_length=150 | ||
| ), | ||
| ), | ||
| ] |
52 changes: 52 additions & 0 deletions
52
accounts/migrations/0005_remove_profile_trac_username_tracaccount.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,52 @@ | ||
| # Generated by Django 5.2.7 on 2026-10-04 16:59 | ||
|
|
||
| import django.db.models.deletion | ||
| from django.conf import settings | ||
| from django.db import migrations, models | ||
|
|
||
|
|
||
| class Migration(migrations.Migration): | ||
|
|
||
| dependencies = [ | ||
| ("accounts", "0004_profile_trac_username"), | ||
| migrations.swappable_dependency(settings.AUTH_USER_MODEL), | ||
| ] | ||
|
|
||
| operations = [ | ||
| migrations.RemoveField( | ||
| model_name="profile", | ||
| name="trac_username", | ||
| ), | ||
| migrations.CreateModel( | ||
| name="TracAccount", | ||
| fields=[ | ||
| ( | ||
| "id", | ||
| models.AutoField( | ||
| auto_created=True, | ||
| primary_key=True, | ||
| serialize=False, | ||
| verbose_name="ID", | ||
| ), | ||
| ), | ||
| ( | ||
| "username", | ||
| models.CharField( | ||
| db_index=True, | ||
| help_text="<p>⚠️ The username on Trac. <b>Setting this verifies ownership.</b></p><p>While a Trac account can be linked to multiple users, once claimed, any user with a matching <code>djangoproject.com</code> username who lacks their own verified Trac account will no longer be able to display Trac stats on their profile.</p>", | ||
| max_length=150, | ||
| ), | ||
| ), | ||
| ( | ||
| "user", | ||
| models.ForeignKey( | ||
| on_delete=django.db.models.deletion.CASCADE, | ||
| to=settings.AUTH_USER_MODEL, | ||
| ), | ||
| ), | ||
| ], | ||
| options={ | ||
| "unique_together": {("user", "username")}, | ||
| }, | ||
| ), | ||
| ] |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,10 +1,44 @@ | ||
| from django.contrib.auth.models import User | ||
| from django.db import models | ||
| from django.utils.safestring import mark_safe | ||
| from django.utils.translation import gettext_lazy as _ | ||
|
|
||
|
|
||
| class Profile(models.Model): | ||
| user = models.OneToOneField(User, on_delete=models.CASCADE) | ||
| name = models.CharField(max_length=200, blank=True) | ||
| bio = models.TextField(blank=True) | ||
|
|
||
| def __str__(self): | ||
| return self.name or str(self.user) | ||
|
|
||
|
|
||
| class TracAccountQuerySet(models.QuerySet): | ||
| def is_username_verified_for_another_user(self, user, username=None): | ||
| return self.exclude(user=user).filter(username=username or user.username).exists() | ||
|
|
||
| class TracAccount(models.Model): | ||
| user = models.ForeignKey(User, on_delete=models.CASCADE) | ||
| username = models.CharField( | ||
| max_length=150, | ||
| db_index=True, | ||
| help_text=mark_safe( | ||
| _( | ||
| "<p>⚠️ The username on Trac. <b>Setting this verifies ownership.</b></p>" | ||
| "<p>" | ||
| "While a Trac account can be linked to multiple users, once claimed, any user " | ||
| "with a matching <code>djangoproject.com</code> username who lacks their own " | ||
| "verified Trac account will no longer be able to display Trac stats on their profile." | ||
| "</p>", | ||
| ), | ||
| ), | ||
| ) | ||
| objects = TracAccountQuerySet.as_manager() | ||
|
|
||
| class Meta: | ||
| unique_together = [ | ||
| ("user", "username"), | ||
| ] | ||
|
|
||
| def __str__(self): | ||
| return f"Trac account for {self.user}: {self.username}" | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| from django.test import TestCase | ||
| from django.utils.functional import Promise | ||
|
|
||
| from .admin import ProfileAdminForm | ||
|
|
||
|
|
||
| class ProfileAdminFormTests(TestCase): | ||
| def test_bio_field_has_max_length(self): | ||
| form = ProfileAdminForm() | ||
| self.assertIn("bio", form.fields) | ||
| self.assertIn("maxlength", form.fields["bio"].widget.attrs) | ||
| self.assertIsInstance(form.fields["bio"].widget.attrs["maxlength"], int) | ||
|
|
||
| def test_bio_field_has_help_text(self): | ||
| form = ProfileAdminForm() | ||
| self.assertIn("bio", form.fields) | ||
| self.assertIsInstance(form.fields["bio"].help_text, (str, Promise)) | ||
| self.assertGreater(len(form.fields["bio"].help_text), 0) |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: Should this be a
UniqueConstraintinstead? Alternatively, should we make aTracAccounta separate model with a FK toProfileto allow a djangoproject account to have multiple trac accounts?It's probably that I don't have a good understanding of the underlying data model, but I believe I have two different accounts on Trac. It'd be cool to count both of them, but not necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A new detail, good to know!
Here, I was trying not to be inclined to block scenarios I'm not aware of that might occur in daily life; just like your case of having multiple Trac accounts now. But if we add a
TracAccountmodel, that's mentally different and clearer, so I would expect itsusernamefield to have aUniqueConstraint.It's still not clear to me if we should set one for the relationship itself, other than
(user_id, trac_username).Under this context, I think this would be the most correct path to follow.
Question: Should we point that FK to the
AUTH_USER_MODELinstead? I'm not sure which one is more central in general use across this project.I'd love to make it possible.
Question: With the current design, we'd need to call the
get_user_statsfunction as many times as the number of linked Trac accounts when the profile page loads without a warm cache. In practice, sincenofO(n)here will be around~2for relatively fewer people, and the results are cached, I don't think optimizing it would be profitable right now. What do you think?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, that's a good point. It probably should go to the user model.
Agreed. Since we're not using
get_user_statsin a bulk operation, I'm less concerned about it.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And I'm leaning towards the
TracAccountapproach as well.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the
tracdbapp is a reflection of an external system's database, where would you prefer me to create theTracAccountmodel? I'd suggest creating a new app,trac_integrationfor hygiene reasons, and astracdbfeels too narrow by definition. But maybe that's just me.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think I'm going to be able to make that call here. I don't regularly maintain this project. I would argue
accountsstill makes the most sense. I thinktrac_integrationassumes we're going to build out a deeper integration with Trac, but I don't think that's really likely.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't think that naming necessarily implies a deep integration, but I see your point. I will continue with your suggestion,
accounts. Thank you.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tim-schilling, sorry for the ping. I've noticed that the project still uses
unique_togetherinstead ofUniqueConstraint. I'm going to useunique_together. Let me know if there's an incremental update plan, so I can convert it toUniqueConstraint.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Image 14: Trac account - admin page
Image 15: Multiple Trac accounts
The "Trac Account" headings are displayed only when a user has multiple Trac accounts or when their linked Trac username differs from their `djangoproject.com` username.I've added more tests for this and improved the names of the previous ones.