From a55111ca74359b5019ff96fb656656049f7f2a70 Mon Sep 17 00:00:00 2001 From: Foo Bar Date: Mon, 3 Mar 2025 19:03:49 +0000 Subject: [PATCH] Use display_name for "username" instead of first_name, last_name (#1640) (#1638) --- core/githubhelper.py | 20 ++----- core/tests/test_githubhelper.py | 45 ++++++---------- libraries/api.py | 3 +- libraries/forms.py | 15 ------ libraries/github.py | 13 ++--- libraries/models.py | 16 ++++++ libraries/views.py | 3 +- mailing_list/managers.py | 13 ++--- mailing_list/models.py | 15 ++++++ news/tests/fixtures.py | 1 + news/tests/test_notifications.py | 3 +- news/tests/test_views.py | 17 +++--- news/views.py | 2 +- templates/admin/release_report_detail.html | 12 ++--- templates/homepage.html | 10 ++-- .../libraries/_library_grid_list_item.html | 2 +- .../includes/library_preferences.html | 5 +- templates/news/detail.html | 8 +-- templates/news/emails/needs_moderation.html | 2 +- templates/news/emails/needs_moderation.txt | 2 +- templates/news/list.html | 10 ++-- templates/users/profile_base.html | 2 +- users/admin.py | 8 ++- users/forms.py | 19 ++++++- .../0017_populate_users_display_name.py | 14 +++++ ...8_user_is_commit_author_name_overridden.py | 21 ++++++++ users/models.py | 49 +++++++---------- users/serializers.py | 12 ++--- users/signals.py | 21 +++++--- users/templatetags/avatar_tags.py | 36 ++++++++----- users/tests/fixtures.py | 10 ++-- users/tests/test_forms.py | 8 +-- users/tests/test_managers.py | 6 +-- users/tests/test_models.py | 54 ++++--------------- users/tests/test_views.py | 6 +-- 35 files changed, 237 insertions(+), 246 deletions(-) create mode 100644 users/migrations/0017_populate_users_display_name.py create mode 100644 users/migrations/0018_user_is_commit_author_name_overridden.py diff --git a/core/githubhelper.py b/core/githubhelper.py index a52feb59..282b62f9 100644 --- a/core/githubhelper.py +++ b/core/githubhelper.py @@ -646,8 +646,7 @@ class GithubDataParser: data["email"] = None data["valid_email"] = False - first_name, last_name = self.extract_names(contributor) - data["first_name"], data["last_name"] = first_name[:30], last_name[:30] + data["display_name"] = self.extract_name(contributor) return data @@ -681,22 +680,9 @@ class GithubDataParser: return return email - def extract_names(self, val: str) -> list: - """ - Returns a list of first, last names for the val argument. - - NOTE: This is an overly simplistic solution to importing names. - Names that don't conform neatly to "First Last" formats will need - to be cleaned up manually. - """ - # Strip the email, if present + def extract_name(self, val: str) -> str: email = re.search("<.+>", val) if email: val = val.replace(email.group(), "") - names = val.strip().rsplit(" ", 1) - - if len(names) == 1: - names.append("") - - return names + return val.strip() diff --git a/core/tests/test_githubhelper.py b/core/tests/test_githubhelper.py index 2b5673a9..d2719934 100644 --- a/core/tests/test_githubhelper.py +++ b/core/tests/test_githubhelper.py @@ -187,30 +187,18 @@ def test_parse_tag(): assert result == expected -def test_extract_names(): - sample = "Tester Testerson " - expected = ["Tester", "Testerson"] - result = GithubDataParser().extract_names(sample) - assert expected == result - - sample = "Tester Testerson" - expected = ["Tester", "Testerson"] - result = GithubDataParser().extract_names(sample) - assert expected == result - - sample = "Tester de Testerson " - expected = ["Tester de", "Testerson"] - result = GithubDataParser().extract_names(sample) - assert expected == result - - sample = "Tester de Testerson" - expected = ["Tester de", "Testerson"] - result = GithubDataParser().extract_names(sample) - assert expected == result - - sample = "Various" - expected = ["Various", ""] - result = GithubDataParser().extract_names(sample) +@pytest.mark.parametrize( + "sample, expected", + [ + ("Tester Testerson ", "Tester Testerson"), + ("Tester Testerson", "Tester Testerson"), + ("Tester de Testerson ", "Tester de Testerson"), + ("Tester de Testerson", "Tester de Testerson"), + ("Various", "Various"), + ], +) +def test_extract_name(sample, expected): + result = GithubDataParser().extract_name(sample) assert expected == result @@ -255,8 +243,7 @@ def test_extract_contributor_data(): expected = { "valid_email": True, "email": "tester@gmail.com", - "first_name": "Tester", - "last_name": "Testerson", + "display_name": "Tester Testerson", } result = GithubDataParser().extract_contributor_data(sample) assert expected == result @@ -264,11 +251,9 @@ def test_extract_contributor_data(): sample = "Tester Testerson" expected = { "valid_email": False, - "first_name": "Tester", - "last_name": "Testerson", + "display_name": "Tester Testerson", } result = GithubDataParser().extract_contributor_data(sample) assert expected["valid_email"] is False - assert expected["first_name"] == result["first_name"] - assert expected["last_name"] == result["last_name"] + assert expected["display_name"] == result["display_name"] assert "email" in result diff --git a/libraries/api.py b/libraries/api.py index a7ed7ce2..a7e1cc77 100644 --- a/libraries/api.py +++ b/libraries/api.py @@ -36,8 +36,7 @@ class LibrarySearchView(viewsets.ModelViewSet): Q(name__icontains=value) | Q(description__icontains=value) | Q(categories__name__icontains=value) - | Q(authors__first_name__icontains=value) - | Q(authors__last_name__icontains=value) + | Q(authors__display_name__icontains=value) ) return Library.objects.filter(f).distinct()[:5] diff --git a/libraries/forms.py b/libraries/forms.py index f4e53851..c25d08c3 100644 --- a/libraries/forms.py +++ b/libraries/forms.py @@ -154,13 +154,6 @@ class CreateReportFullForm(Form): commit__library_version__library_id=library_id ) .annotate(commit_count=Count("commit")) - .values( - "name", - "avatar_url", - "github_profile_url", - "commit_count", - "commit__library_version__library_id", - ) .order_by("-commit_count")[:10] ) return top_contributors_library @@ -273,7 +266,6 @@ class CreateReportForm(CreateReportFullForm): ), ) ) - .values("name", "avatar_url", "commit_count", "github_profile_url") .order_by("-commit_count")[:10] ) @@ -425,13 +417,6 @@ class CreateReportForm(CreateReportFullForm): ) ) .annotate(commit_count=Count("commit")) - .values( - "name", - "avatar_url", - "github_profile_url", - "commit_count", - "commit__library_version__library_id", - ) .order_by("-commit_count")[:10] ) return top_contributors_release diff --git a/libraries/github.py b/libraries/github.py index 89ec604f..279c3bdf 100644 --- a/libraries/github.py +++ b/libraries/github.py @@ -303,22 +303,18 @@ class LibraryUpdater: if isinstance(authors, str): authors = [authors] - for author in authors: person_data = self.parser.extract_contributor_data(author) email = person_data["email"] user = User.objects.find_contributor( email=person_data["email"], - first_name=person_data["first_name"], - last_name=person_data["last_name"], + display_name=person_data["display_name"], ) if not user: email = person_data.pop("email") if not email: - email = generate_fake_email( - f"{person_data['first_name']} {person_data['last_name']}" - ) + email = generate_fake_email(person_data["display_name"]) # With a new email, we may have a user record user = User.objects.find_contributor(email=email) @@ -346,13 +342,12 @@ class LibraryUpdater: person_data = self.parser.extract_contributor_data(maintainer) user = User.objects.find_contributor( email=person_data["email"], - first_name=person_data["first_name"], - last_name=person_data["last_name"], + display_name=person_data["display_name"], ) if not user: email = person_data.pop("email") or generate_fake_email( - f"{person_data['first_name']} {person_data['last_name']}" + person_data["display_name"] ) if not (user := User.objects.filter(email=email).first()): user = User.objects.create_stub_user(email.lower(), **person_data) diff --git a/libraries/models.py b/libraries/models.py index 3d5d4ffc..b745c9e6 100644 --- a/libraries/models.py +++ b/libraries/models.py @@ -2,6 +2,7 @@ import re from typing import Self from urllib.parse import urlparse +from django.contrib.auth import get_user_model from django.core.cache import caches from django.db import models, transaction from django.db.models import Sum @@ -48,6 +49,21 @@ class CommitAuthor(models.Model): avatar_url = models.URLField(null=True, max_length=100) github_profile_url = models.URLField(null=True, max_length=100) + @property + def display_name(self): + if ( + self.user + and self.user.is_commit_author_name_overridden + and self.user.display_name + ): + return self.user.display_name + return self.name + + @property + def user(self): + User = get_user_model() + return User.get_user_by_github_url(self.github_profile_url) + def __str__(self): return self.name diff --git a/libraries/views.py b/libraries/views.py index 7f4ca86d..0eb98042 100644 --- a/libraries/views.py +++ b/libraries/views.py @@ -312,7 +312,7 @@ class LibraryDetail(VersionAlertMixin, BoostVersionMixin, DetailView): def get_author_tag(self): """Format the authors for the author meta tag in the template.""" authors = self.object.authors.all() - author_names = [author.get_full_name() for author in authors] + author_names = [author.display_name for author in authors] if len(author_names) > 1: final_output = ", ".join(author_names[:-1]) + " and " + author_names[-1] else: @@ -374,6 +374,7 @@ class LibraryDetail(VersionAlertMixin, BoostVersionMixin, DetailView): user.commitauthor = SimpleNamespace( github_profile_url="", avatar_url="", + display_name="", ) return qs diff --git a/mailing_list/managers.py b/mailing_list/managers.py index e5749e0f..3368823a 100644 --- a/mailing_list/managers.py +++ b/mailing_list/managers.py @@ -5,14 +5,11 @@ from django.db.models import Sum, F class EmailDataQuerySet(models.QuerySet): def with_total_counts(self): """Annotate total post count per author.""" - return ( - self.annotate( - name=F("author__name"), - avatar_url=F("author__avatar_url"), - github_profile_url=F("author__avatar_url"), - ) - .values("name", "avatar_url", "github_profile_url", "author_id") - .annotate(total_count=Sum("count")) + return self.annotate( + name=F("author__name"), + avatar_url=F("author__avatar_url"), + github_profile_url=F("author__avatar_url"), + total_count=Sum("count"), ) diff --git a/mailing_list/models.py b/mailing_list/models.py index 9163f19b..5e666540 100644 --- a/mailing_list/models.py +++ b/mailing_list/models.py @@ -1,3 +1,4 @@ +from django.contrib.auth import get_user_model from django.db import models from mailing_list.managers import EmailDataManager @@ -8,6 +9,20 @@ class EmailData(models.Model): version = models.ForeignKey("versions.Version", on_delete=models.CASCADE) count = models.IntegerField() + @property + def display_name(self): + if ( + self.author.user + and self.author.user.is_commit_author_name_overridden + and self.author.user.display_name + ): + return self.author.user.display_name + + @property + def user(self): + User = get_user_model() + return User.get_user_by_github_url(self.author.github_profile_url) + objects = EmailDataManager() class Meta: diff --git a/news/tests/fixtures.py b/news/tests/fixtures.py index 2eae27df..3dafa92c 100644 --- a/news/tests/fixtures.py +++ b/news/tests/fixtures.py @@ -130,6 +130,7 @@ def regular_user(db, make_user): def superuser(db, make_user): return make_user( email="superuser@example.com", + display_name="Regular User", is_superuser=True, password="admin", groups={}, diff --git a/news/tests/test_notifications.py b/news/tests/test_notifications.py index 3a75f9cb..ca2838aa 100644 --- a/news/tests/test_notifications.py +++ b/news/tests/test_notifications.py @@ -107,7 +107,8 @@ def test_send_email_news_needs_moderation( assert "news entry needs moderation" in msg.subject.lower() assert entry.title in msg.body assert escape(entry.title) not in msg.body - assert entry.author.get_display_name in msg.body + if entry.author.display_name: + assert entry.author.display_name in msg.body assert entry.author.email in msg.body assert request.build_absolute_uri(entry.get_absolute_url()) in msg.body assert request.build_absolute_uri(reverse("news-moderate")) in msg.body diff --git a/news/tests/test_views.py b/news/tests/test_views.py index 98bf5259..f980a7bf 100644 --- a/news/tests/test_views.py +++ b/news/tests/test_views.py @@ -360,18 +360,16 @@ def test_news_create_multiplexer(tp, user_type, request): @pytest.mark.parametrize( - "has_image, has_first_name, has_last_name, should_redirect", + "has_image, has_display_name, should_redirect", [ - (True, True, False, False), # Has image and first name - (True, False, True, False), # Has image and last name - (True, True, True, False), # Has image, first name, and last name - (False, True, True, True), # Missing image - (True, False, False, True), # Missing names - (False, False, False, True), # Missing everything + (True, True, False), # Has image, display name + (False, True, True), # Missing image + (True, False, True), # Missing names + (False, False, True), # Missing everything ], ) def test_news_create_requirements( - tp, user, has_image, has_first_name, has_last_name, should_redirect + tp, user, has_image, has_display_name, should_redirect ): """Users must have a profile photo and at least one of the names: first or last.""" url_name = "news-create" @@ -389,8 +387,7 @@ def test_news_create_requirements( else: user.image = None - user.first_name = "Test" if has_first_name else "" - user.last_name = "User" if has_last_name else "" + user.display_name = "Test User" if has_display_name else "" user.save() with tp.login(user): diff --git a/news/views.py b/news/views.py index cd78a33d..8f9e0d57 100644 --- a/news/views.py +++ b/news/views.py @@ -268,7 +268,7 @@ class AllTypesCreateView(LoginRequiredMixin, TemplateView): if request.user.is_authenticated: missing_data = [] - if not request.user.first_name and not request.user.last_name: + if not request.user.display_name: missing_data.append("your name") if not request.user.image: diff --git a/templates/admin/release_report_detail.html b/templates/admin/release_report_detail.html index e743a379..c037cf6b 100644 --- a/templates/admin/release_report_detail.html +++ b/templates/admin/release_report_detail.html @@ -157,15 +157,15 @@ body {
- {% for user in committee_members|dictsort:"first_name" %} + {% for user in committee_members|dictsort:"display_name" %}
- + {% avatar user=user is_link=False image_size="w-32 h-32" icon_size="text-8xl" %} -
{{user.first_name}} {{user.last_name}}
+
{{user.display_name}}
{% endfor %}
@@ -189,7 +189,7 @@ body { {% avatar commitauthor=author %}
- {{ author.name }} + {{ author.display_name }}
{{ author.commit_count }} commit{{ author.commit_count|pluralize }}
@@ -217,7 +217,7 @@ body { {% avatar commitauthor=item %}
- {{ item.name }} + {{ item.display_name }}
{{ item.total_count }} post{{ item.total_count|pluralize }}
@@ -417,7 +417,7 @@ body { {% avatar commitauthor=author %}
- {{ author.name }} + {{ author.display_name }}
{{ author.commit_count }} commit{{ author.commit_count|pluralize }}
diff --git a/templates/homepage.html b/templates/homepage.html index 432f1073..a6099dc6 100644 --- a/templates/homepage.html +++ b/templates/homepage.html @@ -146,14 +146,14 @@
{% if author.image %} {{ author.get_full_name }} {% else %} - + {% endif %}
- {{ author.get_full_name }} + {{ author.display_name }} {% endfor %} @@ -230,7 +230,7 @@ -

Posted on {{ entry.publish_at|date:"M jS, Y" }} by {{ entry.author.get_full_name }}

+

Posted on {{ entry.publish_at|date:"M jS, Y" }} by {{ entry.author.display_name }}

{% if entry.content %}
{{ entry.content|urlize|url_target_blank:'text-sky-600 dark:text-sky-300 hover:text-orange dark:hover:text-orange'|linebreaksbr|multi_truncate_middle:30|truncatechars_html:500 }} diff --git a/templates/libraries/_library_grid_list_item.html b/templates/libraries/_library_grid_list_item.html index 38fa7d31..96b2b219 100644 --- a/templates/libraries/_library_grid_list_item.html +++ b/templates/libraries/_library_grid_list_item.html @@ -8,7 +8,7 @@ onclick="window.location='{% url 'library-detail' library_slug=library_version.l {{ library_version.library.name }} {% for author in library.authors.all %} {% if author.image %} - {{ author.get_display_name }} + {{ author.display_name }} {% endif %} {% endfor %} diff --git a/templates/libraries/includes/library_preferences.html b/templates/libraries/includes/library_preferences.html index 93327f34..f9b3ce4c 100644 --- a/templates/libraries/includes/library_preferences.html +++ b/templates/libraries/includes/library_preferences.html @@ -5,7 +5,10 @@
{# Search #} - {% comment %} {% include "libraries/includes/search_input.html" %} {% endcomment %} + {% comment %} + LibrarySearchView can be removed if this isn't ever used again + {% include "libraries/includes/search_input.html" %} + {% endcomment %} {# Display options #}
diff --git a/templates/news/detail.html b/templates/news/detail.html index ded0fb72..9bdc9fc3 100644 --- a/templates/news/detail.html +++ b/templates/news/detail.html @@ -54,16 +54,16 @@
{% if entry.author.image %} - {{ entry.author.get_full_name }} + {{ entry.author.display_name }} {% else %} - + {% endif %} - {% if entry.author.get_full_name %} + {% if entry.author.display_name %}
- {{ entry.author.get_full_name }}
+ {{ entry.author.display_name }}
{{ entry.publish_at|date:'M jS, Y' }}
{% endif %} diff --git a/templates/news/emails/needs_moderation.html b/templates/news/emails/needs_moderation.html index b22d440c..bc69cf48 100644 --- a/templates/news/emails/needs_moderation.html +++ b/templates/news/emails/needs_moderation.html @@ -74,7 +74,7 @@

Hello! You are receiving this email because you are a Boost news moderator.
- The user {{ entry.author.get_display_name|default:entry.author.email }} has submitted a new {{ entry.tag }} that requires moderation: + The user {{ entry.author.display_name|default:entry.author.email }} has submitted a new {{ entry.tag }} that requires moderation:

diff --git a/templates/news/emails/needs_moderation.txt b/templates/news/emails/needs_moderation.txt index f56e4cb3..fcbdbf28 100644 --- a/templates/news/emails/needs_moderation.txt +++ b/templates/news/emails/needs_moderation.txt @@ -2,7 +2,7 @@ Hello! You are receiving this email because you are a Boost news moderator. -The user {{ entry.author.get_display_name|default:entry.author.email }} has submitted a new {{ entry.tag }} that requires moderation: +The user {{ entry.author.display_name|default:entry.author.email }} has submitted a new {{ entry.tag }} that requires moderation: --- {% autoescape off %} diff --git a/templates/news/list.html b/templates/news/list.html index 3ea4af7e..081e25eb 100644 --- a/templates/news/list.html +++ b/templates/news/list.html @@ -116,7 +116,7 @@
{% can_edit news_item=entry as editable %} @@ -134,7 +134,7 @@
@@ -147,7 +147,7 @@
- {{ entry.author.get_full_name }}
+ {{ entry.author.display_name }}
{{ entry.display_publish_at }}
@@ -183,11 +183,11 @@
{% if entry.author.image %} - {{ entry.author.get_full_name }} + {{ entry.author.display_name }} {% else %} - + {% endif %}
diff --git a/templates/users/profile_base.html b/templates/users/profile_base.html index 026b42bb..2f835afa 100644 --- a/templates/users/profile_base.html +++ b/templates/users/profile_base.html @@ -6,7 +6,7 @@
{% avatar user=user image_size='h-[80px] w-[80px]' icon_size='text-7xl' %}
- {{ user.get_full_name }} + {{ user.display_name }} Joined {{ user.date_joined }}
diff --git a/users/admin.py b/users/admin.py index a07a063c..4b29da80 100644 --- a/users/admin.py +++ b/users/admin.py @@ -12,8 +12,7 @@ class EmailUserAdmin(UserAdmin): _("Personal info"), { "fields": ( - "first_name", - "last_name", + "display_name", "github_username", "valid_email", "claimed", @@ -54,13 +53,12 @@ class EmailUserAdmin(UserAdmin): ordering = ("email",) list_display = ( "email", - "first_name", - "last_name", + "display_name", "is_staff", "valid_email", "claimed", ) - search_fields = ("email", "first_name", "last_name") + search_fields = ("email", "display_name") admin.site.register(User, EmailUserAdmin) diff --git a/users/forms.py b/users/forms.py index 7db1c3f7..fbd2e0f0 100644 --- a/users/forms.py +++ b/users/forms.py @@ -88,7 +88,24 @@ class PreferencesForm(forms.ModelForm): class UserProfileForm(forms.ModelForm): class Meta: model = User - fields = ["email", "first_name", "last_name", "indicate_last_login_method"] + fields = [ + "email", + "display_name", + "indicate_last_login_method", + "is_commit_author_name_overridden", + ] + labels = { + "display_name": "Username", + "is_commit_author_name_overridden": "Override commit author name", + } + override_msg = ( + "Globally replaces your git commit author name with Username " + "value set above." + ) + help_texts = { + "display_name": "Your name as it will be displayed across the site.", + "is_commit_author_name_overridden": override_msg, + } class CustomClearableFileInput(forms.ClearableFileInput): diff --git a/users/migrations/0017_populate_users_display_name.py b/users/migrations/0017_populate_users_display_name.py new file mode 100644 index 00000000..11f04e79 --- /dev/null +++ b/users/migrations/0017_populate_users_display_name.py @@ -0,0 +1,14 @@ +# Generated by Django 4.2.16 on 2025-02-14 22:40 + +from django.db import migrations + + +class Migration(migrations.Migration): + + dependencies = [ + ("users", "0016_user_hq_image"), + ] + + operations = [ + migrations.RunSQL("UPDATE users_user SET display_name = CONCAT(first_name, ' ', last_name) WHERE display_name IS NULL;"), + ] diff --git a/users/migrations/0018_user_is_commit_author_name_overridden.py b/users/migrations/0018_user_is_commit_author_name_overridden.py new file mode 100644 index 00000000..f330d673 --- /dev/null +++ b/users/migrations/0018_user_is_commit_author_name_overridden.py @@ -0,0 +1,21 @@ +# Generated by Django 4.2.16 on 2025-02-25 19:09 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ("users", "0017_populate_users_display_name"), + ] + + operations = [ + migrations.AddField( + model_name="user", + name="is_commit_author_name_overridden", + field=models.BooleanField( + default=False, + help_text="Select to override the commit author with Username", + ), + ), + ] diff --git a/users/models.py b/users/models.py index fb2661df..fef5280f 100644 --- a/users/models.py +++ b/users/models.py @@ -73,7 +73,7 @@ class UserManager(BaseUserManager): logger.info("Creating stub user with email='%s'", email) return self._create_user(email, password, claimed=claimed, **extra_fields) - def find_contributor(self, email=None, first_name=None, last_name=None): + def find_contributor(self, email=None, display_name=None): """ Lazily finds a matching User record by email, or first name and last name. @@ -88,8 +88,7 @@ class UserManager(BaseUserManager): email (str, optional): The email address of the user to search for. Assumes the email address is legitimate, and is not one we generated as a placeholder. - first_name (str, optional): The first name of the user to search for. - last_name (str, optional): The last name of the user to search for. + display_name (str, optional): The display name of the user to search for. Returns: User object or None: If a user is found based on the provided criteria, the @@ -104,10 +103,8 @@ class UserManager(BaseUserManager): except self.model.DoesNotExist: pass - if not user and first_name and last_name: - users = self.filter( - first_name__iexact=first_name, last_name__iexact=last_name - ) + if not user and display_name: + users = self.filter(display_name__iexact=display_name) authors_or_maintainers = users.filter( models.Q(authors__isnull=False) | models.Q(maintainers__isnull=False) ).distinct() @@ -144,6 +141,7 @@ class BaseUser(AbstractBaseUser, PermissionsMixin): Our email for username user model """ + # todo: remove first_name, last_name after May 2025 first_name = models.CharField(_("first name"), max_length=30, blank=True) last_name = models.CharField(_("last name"), max_length=30, blank=True) email = models.EmailField(_("email address"), unique=True, db_index=True) @@ -175,17 +173,6 @@ class BaseUser(AbstractBaseUser, PermissionsMixin): swappable = "AUTH_USER_MODEL" abstract = True - def get_full_name(self): - """ - Returns the first_name plus the last_name, with a space in between. - """ - full_name = "%s %s" % (self.first_name, self.last_name) - return full_name.strip() - - def get_short_name(self): - """Returns the short name for the user.""" - return self.first_name - def email_user(self, subject, message, from_email=None, **kwargs): """ Sends an email to this User. @@ -212,7 +199,11 @@ class User(BaseUser): """ badges = models.ManyToManyField(Badge) + # todo: consider making this unique=True after checking user data for duplicates github_username = models.CharField(_("github username"), max_length=100, blank=True) + is_commit_author_name_overridden = models.BooleanField( + default=False, help_text="Select to override the commit author with Username" + ) image = models.FileField( upload_to="profile-images", null=True, @@ -269,7 +260,7 @@ class User(BaseUser): # elapsed. delete_permanently_at = models.DateTimeField(null=True, editable=False) - def save_image_from_github(self, avatar_url): + def save_image_from_provider(self, avatar_url): response = requests.get(avatar_url) filename = f"{self.profile_image_filename_root}.png" os.path.join(settings.MEDIA_ROOT, "media", "profile-images", filename) @@ -287,16 +278,6 @@ class User(BaseUser): Does not include the file extension.""" return f"profile-{self.pk}" - @property - def get_display_name(self): - """Returns the display name for the user.""" - if self.display_name: - return self.display_name - elif self.first_name and self.last_name: - return f"{self.first_name} {self.last_name}" - else: - return self.first_name or self.last_name - def claim(self): """Claim the user""" if not self.claimed: @@ -314,6 +295,13 @@ class User(BaseUser): return None return f"https://github.com/{self.github_username}" + @staticmethod + def get_user_by_github_url(url: str): + if not url: + return None + github_user = url.rstrip("/").split("/")[-1] + return User.objects.filter(github_username=github_user).first() + @transaction.atomic def delete_account(self): from . import tasks @@ -328,6 +316,7 @@ class User(BaseUser): self.display_name = "John Doe" self.first_name = "John" self.last_name = "Doe" + self.display_name = "John Doe" self.email = "deleted-{}@example.com".format(uuid.uuid4()) image = self.image transaction.on_commit(lambda: image.delete()) @@ -337,7 +326,7 @@ class User(BaseUser): self.save() def __str__(self): - return f"{self.first_name} {self.last_name} <{self.email}>" + return f"{self.display_name} <{self.email}>" class LastSeen(models.Model): diff --git a/users/serializers.py b/users/serializers.py index 79cf9284..994c8367 100644 --- a/users/serializers.py +++ b/users/serializers.py @@ -13,13 +13,11 @@ class UserSerializer(serializers.ModelSerializer): model = User fields = ( "id", - "first_name", - "last_name", + "display_name", ) read_only_fields = ( "id", - "first_name", - "last_name", + "display_name", ) @@ -33,8 +31,7 @@ class CurrentUserSerializer(serializers.ModelSerializer): fields = ( "id", "email", - "first_name", - "last_name", + "display_name", "image", "date_joined", "data", @@ -53,8 +50,7 @@ class FullUserSerializer(serializers.ModelSerializer): fields = ( "id", "email", - "first_name", - "last_name", + "display_name", "is_staff", "is_active", "is_superuser", diff --git a/users/signals.py b/users/signals.py index db98cbd1..bfd7b9d9 100644 --- a/users/signals.py +++ b/users/signals.py @@ -7,6 +7,7 @@ from allauth.socialaccount.models import SocialAccount from users.constants import LOGIN_METHOD_SESSION_FIELD_NAME GITHUB = "github" +GOOGLE = "google" @receiver(post_save, sender=SocialAccount) @@ -16,19 +17,25 @@ def import_social_profile_data(sender, instance, created, **kwargs): user's profile """ if not created: + # for display name to save this needs to be resaved after the redirect. + instance.user.save() return - if instance.provider != GITHUB: + if instance.provider not in [GITHUB, GOOGLE]: return - github_username = instance.extra_data.get("login") - avatar_url = instance.extra_data.get("avatar_url") - if github_username: - instance.user.github_username = github_username - instance.save() + avatar_url = None + if instance.provider == GITHUB: + instance.user.github_username = instance.extra_data.get("login") + avatar_url = instance.extra_data.get("avatar_url") + elif instance.provider == GOOGLE: + avatar_url = instance.extra_data.get("picture") if avatar_url: - instance.user.save_image_from_github(avatar_url) + instance.user.save_image_from_provider(avatar_url) + + instance.user.display_name = instance.extra_data.get("name") + instance.save() @receiver(user_logged_in) diff --git a/users/templatetags/avatar_tags.py b/users/templatetags/avatar_tags.py index 990a0068..a5075a49 100644 --- a/users/templatetags/avatar_tags.py +++ b/users/templatetags/avatar_tags.py @@ -45,6 +45,14 @@ def avatar( contributor_label=None, avatar_type=None, ): + def get_commit_author_attribute(commitauthor, attribute): + if isinstance(commitauthor, dict): + return commitauthor.get(attribute) + try: + return getattr(commitauthor, attribute, None) + except AttributeError: + return None + kwargs = { "is_link": is_link, "alt": alt, @@ -54,33 +62,35 @@ def avatar( "contributor_label": contributor_label, "avatar_type": avatar_type, } + if user and commitauthor: - image_url = user.get_thumbnail_url() or commitauthor.avatar_url - href = user.github_profile_url or commitauthor.github_profile_url + image_url = user.get_thumbnail_url() or get_commit_author_attribute( + commitauthor, "avatar_url" + ) + href = user.github_profile_url or get_commit_author_attribute( + commitauthor, "github_profile_url" + ) return base_avatar( - user.get_full_name(), + commitauthor.display_name, image_url, href, **kwargs, ) elif user: return base_avatar( - user.get_full_name(), + user.display_name, user.get_thumbnail_url(), user.github_profile_url, **kwargs, ) elif commitauthor: - if isinstance(commitauthor, dict): - name = commitauthor["name"] - avatar_url = commitauthor["avatar_url"] - github_profile_url = commitauthor["github_profile_url"] - else: - name = commitauthor.name - avatar_url = commitauthor.avatar_url - github_profile_url = commitauthor.github_profile_url + display_name = get_commit_author_attribute(commitauthor, "display_name") + avatar_url = get_commit_author_attribute(commitauthor, "avatar_url") + github_profile_url = get_commit_author_attribute( + commitauthor, "github_profile_url" + ) return base_avatar( - name, + display_name, avatar_url, github_profile_url, **kwargs, diff --git a/users/tests/fixtures.py b/users/tests/fixtures.py index 84c4c4c4..4fd12141 100644 --- a/users/tests/fixtures.py +++ b/users/tests/fixtures.py @@ -13,9 +13,9 @@ def user(db): user = baker.make( "users.User", email="user@example.com", - first_name="Regular", - last_name="User", + display_name="Regular User", indicate_last_login_method=False, + is_commit_author_name_overridden=False, last_login=timezone.now(), image=None, ) @@ -39,8 +39,7 @@ def staff_user(db): user = baker.make( "users.User", email="staff@example.com", - first_name="Staff", - last_name="User", + display_name="Staff User", last_login=timezone.now(), is_staff=True, image=None, @@ -65,8 +64,7 @@ def super_user(db): user = baker.make( "users.User", email="super@example.com", - first_name="Super", - last_name="User", + display_name="Super User", last_login=timezone.now(), is_staff=True, is_superuser=True, diff --git a/users/tests/test_forms.py b/users/tests/test_forms.py index e4c312fc..c5b831c2 100644 --- a/users/tests/test_forms.py +++ b/users/tests/test_forms.py @@ -157,16 +157,16 @@ def test_preferences_form_model_modifies_instance_empty_list_user_moderator( def test_user_profile_form(user): form = UserProfileForm(instance=user) assert set(form.fields.keys()) == { - "first_name", - "last_name", + "display_name", "email", "indicate_last_login_method", + "is_commit_author_name_overridden", } assert form.initial == { - "first_name": user.first_name, - "last_name": user.last_name, + "display_name": user.display_name, "email": user.email, "indicate_last_login_method": user.indicate_last_login_method, + "is_commit_author_name_overridden": user.is_commit_author_name_overridden, } form = UserProfileForm(instance=user, data={"email": "test@example.com"}) assert form.is_valid() diff --git a/users/tests/test_managers.py b/users/tests/test_managers.py index 092f11c1..1801f9ce 100644 --- a/users/tests/test_managers.py +++ b/users/tests/test_managers.py @@ -44,8 +44,7 @@ def test_create_stub_user(db): u = User.objects.create_stub_user( "t4@example.com", None, - first_name="Tester", - last_name="Testerson", + display_name="Tester Testerson", valid_email=False, ) assert u.claimed is False @@ -53,5 +52,4 @@ def test_create_stub_user(db): assert u.is_active is True assert u.is_staff is False assert u.is_superuser is False - assert u.first_name == "Tester" - assert u.last_name == "Testerson" + assert u.display_name == "Tester Testerson" diff --git a/users/tests/test_models.py b/users/tests/test_models.py index 6de845ae..1430005d 100644 --- a/users/tests/test_models.py +++ b/users/tests/test_models.py @@ -28,30 +28,6 @@ def test_super_user(super_user): assert super_user.is_superuser is True -def test_get_display_name(user): - # Test case 1: Display name is set - user.display_name = "Display Name" - user.save() - assert user.get_display_name == "Display Name" - - # Test case 2: First and last name are set, no display name - user.display_name = "" - user.save() - assert user.get_display_name == f"{user.first_name} {user.last_name}" - - # Test case 3: Only first name is set, no display name - user.first_name = "First" - user.last_name = "" - user.save() - assert user.get_display_name == "First" - - # Test case 4: Only last name is set, no display name - user.first_name = "" - user.last_name = "Last" - user.save() - assert user.get_display_name == "Last" - - def test_profile_image_filename_root(user): assert user.profile_image_filename_root == f"profile-{user.id}" @@ -122,29 +98,21 @@ def test_find_contributor_by_email_not_found(): def test_find_contributor_not_author_or_maintainer(user: User): - found_user = User.objects.find_contributor( - first_name=user.first_name, last_name=user.last_name - ) + found_user = User.objects.find_contributor(display_name=user.display_name) assert found_user is None -def test_find_contributor_by_first_and_last_name_not_found(): - non_existent_first_name = "Nonexistent" - non_existent_last_name = "User" - found_user = User.objects.find_contributor( - first_name=non_existent_first_name, last_name=non_existent_last_name - ) +def test_find_contributor_by_display_name_not_found(): + non_existent_name = "Nonexistent User" + found_user = User.objects.find_contributor(display_name=non_existent_name) assert found_user is None -def test_find_contributor_by_first_and_last_name_multiple_results(user, staff_user): - staff_user.first_name = user.first_name - staff_user.last_name = user.last_name +def test_find_contributor_by_display_name_multiple_results(user, staff_user): + staff_user.display_name = user.display_name staff_user.save() - found_user = User.objects.find_contributor( - first_name=user.first_name, last_name=user.last_name - ) + found_user = User.objects.find_contributor(display_name=user.display_name) assert found_user is None @@ -157,9 +125,7 @@ def test_find_contributor_is_author(user, library): library.authors.add(user) library.save() - found_user = User.objects.find_contributor( - first_name=user.first_name, last_name=user.last_name - ) + found_user = User.objects.find_contributor(display_name=user.display_name) assert found_user == user @@ -167,9 +133,7 @@ def test_find_contributor_is_maintainer(user, library_version): library_version.maintainers.add(user) library_version.save() - found_user = User.objects.find_contributor( - first_name=user.first_name, last_name=user.last_name - ) + found_user = User.objects.find_contributor(display_name=user.display_name) assert found_user == user diff --git a/users/tests/test_views.py b/users/tests/test_views.py index 043fcdd1..bb09d8da 100644 --- a/users/tests/test_views.py +++ b/users/tests/test_views.py @@ -96,16 +96,14 @@ def test_current_user_profile_view_update_name(user, tp): tp.reverse("profile-account"), data={ "email": user.email, - "first_name": new_first_name, - "last_name": new_last_name, + "display_name": f"{new_first_name} {new_last_name}", "update_profile": "", }, follow=True, ) assert response.status_code == 200 user.refresh_from_db() - assert user.first_name == new_first_name - assert user.last_name == new_last_name + assert user.display_name == f"{new_first_name} {new_last_name}" @pytest.mark.django_db