From f483ab299c6ade537a0df35b1c27ad14941b6934 Mon Sep 17 00:00:00 2001 From: sgfost Date: Mon, 13 Nov 2023 09:54:42 -0700 Subject: [PATCH 1/5] fix: contributor name logic, list user's models in desc order - clean up contributor get_full_name + correctly resolve memberprofile.name when needed - show last modified date on unpublished models ref comses/planning#192 --- django/core/views.py | 1 + .../jinja2/library/codebases/macros.jinja | 2 +- django/library/models.py | 34 ++++++++----------- 3 files changed, 16 insertions(+), 21 deletions(-) diff --git a/django/core/views.py b/django/core/views.py index bb99eb091..cdc11c95e 100644 --- a/django/core/views.py +++ b/django/core/views.py @@ -349,6 +349,7 @@ def get_retrieve_context(self, instance): .filter_by_contributor(instance.user) .with_tags() .with_featured_images() + .order_by("-last_modified") ) add_change_delete_perms(instance, context, accessing_user) return context diff --git a/django/library/jinja2/library/codebases/macros.jinja b/django/library/jinja2/library/codebases/macros.jinja index e005de299..9e65024d5 100644 --- a/django/library/jinja2/library/codebases/macros.jinja +++ b/django/library/jinja2/library/codebases/macros.jinja @@ -91,7 +91,7 @@ | Last modified {{ format_date(codebase.last_published_on) }} {% endif %} {% else %} - Unpublished + Last modified {{ format_date(codebase.last_modified) }} {% endif %} diff --git a/django/library/models.py b/django/library/models.py index 3f9e9c3ea..5e3e557da 100644 --- a/django/library/models.py +++ b/django/library/models.py @@ -218,28 +218,22 @@ def has_name(self): return any([self.given_name, self.family_name]) or self.user def get_full_name(self, family_name_first=False): - full_name = "" - # Bah. Horrid name logic if self.type == "person": - if self.has_name: - if family_name_first: - full_name = ( - f"{self.family_name}, {self.given_name} {self.middle_name}" - ) - elif self.middle_name: - full_name = ( - f"{self.given_name} {self.middle_name} {self.family_name}" - ) - else: - full_name = f"{self.given_name} {self.family_name}" - elif self.user: - full_name = self.user.member_profile.name - else: - logger.exception("No usable name found for contributor %s", self.pk) + return self._get_person_full_name(family_name_first) + else: + # organizations only use given_name + return self.given_name + + def _get_person_full_name(self, family_name_first=False): + if not self.has_name: + logger.exception("No usable name found for contributor %s", self.pk) + return "" + if self.user and not any([self.given_name, self.family_name]): + return self.user.member_profile.name + if family_name_first: + return f"{self.family_name}, {self.given_name} {self.middle_name}" else: - # organizations only have given_name - full_name = self.given_name - return " ".join(full_name.split()) + return f"{self.given_name} {self.middle_name} {self.family_name}" @property def formatted_affiliations(self): From 3b9ec7a5a26affcb06d935dac8ed01ccc2e8873d Mon Sep 17 00:00:00 2001 From: sgfost Date: Mon, 13 Nov 2023 11:13:59 -0700 Subject: [PATCH 2/5] fix: curator statistics releases query filter by a list of ids instead of trying to combine two querysets resolves comses/planning#190 --- django/library/models.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/django/library/models.py b/django/library/models.py index 5e3e557da..ab2a243d6 100644 --- a/django/library/models.py +++ b/django/library/models.py @@ -459,7 +459,8 @@ def updated_after(self, start_date, end_date=None, **kwargs): updated_codebases = updated_codebases.difference(new_codebases) releases = CodebaseRelease.objects.filter( id__in=( - new_codebases.values("releases") | updated_codebases.values("releases") + list(new_codebases.values_list("releases", flat=True)) + + list(updated_codebases.values_list("releases", flat=True)) ) ) return new_codebases, updated_codebases, releases From 95721e256a8fabaec2a251e89e2a21a9d30acb53 Mon Sep 17 00:00:00 2001 From: sgfost Date: Wed, 15 Nov 2023 11:31:21 -0700 Subject: [PATCH 3/5] fix: always include self-submitted codebases in profile models tab prevents models that are submitted by a user who is not a contributor from being inaccessible - refactor the contributor filtering qs method(s) --- django/core/views.py | 5 +---- django/library/models.py | 23 +++++++++-------------- 2 files changed, 10 insertions(+), 18 deletions(-) diff --git a/django/core/views.py b/django/core/views.py index cdc11c95e..991e585e7 100644 --- a/django/core/views.py +++ b/django/core/views.py @@ -340,13 +340,10 @@ def get_queryset(self): def get_retrieve_context(self, instance): context = super().get_retrieve_context(instance) accessing_user = self.request.user - # FIXME: ideally this functionality should be in the library app, though its not clear - # whether this can be done without a much more complicated workaround or a major - # re-organization of apps logger.debug("Finding models for user %s", instance.user) context["codebases"] = ( Codebase.objects.accessible(accessing_user) - .filter_by_contributor(instance.user) + .filter_by_contributor_or_submitter(instance.user) .with_tags() .with_featured_images() .order_by("-last_modified") diff --git a/django/library/models.py b/django/library/models.py index ab2a243d6..3d77f9840 100644 --- a/django/library/models.py +++ b/django/library/models.py @@ -347,21 +347,16 @@ def accessible(self, user): user=user, queryset=self.with_viewable_releases(user=user) ) + def get_contributor_q(self, user): + if Contributor.objects.filter(user=user).count() > 1: + logger.warning("User %s has multiple contributors", user) + return Q(releases__contributors__user=user) + def filter_by_contributor(self, user): - # FIXME: query could likely be more efficient - # find all codebase releases with this user marked as a ReleaseContributor - contributors = Contributor.objects.filter(user=user) - if contributors.exists(): - if contributors.count() > 1: - logger.warning("User %s has multiple contributors", user) - releases = CodebaseRelease.objects.filter( - pk__in=ReleaseContributor.objects.filter( - contributor__in=contributors - ).values_list("release", flat=True) - ) - return self.filter(releases__in=releases).distinct() - else: - return self.filter(submitter=user) + return self.filter(self.get_contributor_q(user)).distinct() + + def filter_by_contributor_or_submitter(self, user): + return self.filter(Q(submitter=user) | self.get_contributor_q(user)).distinct() def with_contributors(self, release_contributor_qs=None, user=None, **kwargs): if user is not None: From 29e9d7eac831e23da3388589aaada2ce6650f4c7 Mon Sep 17 00:00:00 2001 From: Allen Lee Date: Thu, 16 Nov 2023 14:31:26 -0700 Subject: [PATCH 4/5] fix: guard against empty middle names whitespace non-issue for HTML rendering as HTML doesn't care much about consecutive whitespace https://developer.mozilla.org/en-US/docs/Web/API/Document_Object_Model/Whitespace possibly-important for codemeta + other metadata dissemination though - switch from logger.exception to logger.warning - keep Unpublished in the display of unpublished codebases --- django/library/jinja2/library/codebases/macros.jinja | 4 ++-- django/library/models.py | 9 ++++++--- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/django/library/jinja2/library/codebases/macros.jinja b/django/library/jinja2/library/codebases/macros.jinja index 9e65024d5..42065a912 100644 --- a/django/library/jinja2/library/codebases/macros.jinja +++ b/django/library/jinja2/library/codebases/macros.jinja @@ -81,7 +81,7 @@ {% endif %} {% else %} - {# FIXME: this is a degenerate case, there should always be at least 1 contributor (the submitter) #} + {# FIXME: degenerate data guard, there should always be at least 1 contributor #} no contributors listed {% endfor %} | @@ -91,7 +91,7 @@ | Last modified {{ format_date(codebase.last_published_on) }} {% endif %} {% else %} - Last modified {{ format_date(codebase.last_modified) }} + Unpublished | Last modified {{ format_date(codebase.last_modified) }} {% endif %} diff --git a/django/library/models.py b/django/library/models.py index 3d77f9840..6fa51397a 100644 --- a/django/library/models.py +++ b/django/library/models.py @@ -226,14 +226,17 @@ def get_full_name(self, family_name_first=False): def _get_person_full_name(self, family_name_first=False): if not self.has_name: - logger.exception("No usable name found for contributor %s", self.pk) + logger.warning("No usable name found for contributor %s", self.pk) return "" if self.user and not any([self.given_name, self.family_name]): return self.user.member_profile.name if family_name_first: - return f"{self.family_name}, {self.given_name} {self.middle_name}" + return f"{self.family_name}, {self.given_name} {self.middle_name}".strip() else: - return f"{self.given_name} {self.middle_name} {self.family_name}" + return ( + f"{self.given_name} {self.middle_name}".strip() + + f" {self.family_name}".rstrip() + ) @property def formatted_affiliations(self): From 6f4e6dd816e9a4480aa34a089d1c52326ceebcc9 Mon Sep 17 00:00:00 2001 From: Allen Lee Date: Thu, 16 Nov 2023 14:53:31 -0700 Subject: [PATCH 5/5] refactor: use union instead of materializing lists https://docs.djangoproject.com/en/4.2/ref/models/querysets/#union --- django/library/models.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/django/library/models.py b/django/library/models.py index 6fa51397a..40a15e162 100644 --- a/django/library/models.py +++ b/django/library/models.py @@ -457,8 +457,9 @@ def updated_after(self, start_date, end_date=None, **kwargs): updated_codebases = updated_codebases.difference(new_codebases) releases = CodebaseRelease.objects.filter( id__in=( - list(new_codebases.values_list("releases", flat=True)) - + list(updated_codebases.values_list("releases", flat=True)) + new_codebases.values_list("releases", flat=True).union( + updated_codebases.values_list("releases", flat=True) + ) ) ) return new_codebases, updated_codebases, releases @@ -2024,7 +2025,7 @@ def pending(self, **kwargs): def candidate_reviewers(self, **kwargs): # FIXME: fairly horribly inefficient return MemberProfile.objects.filter( - pk__in=self.values_list("candidate_reviewer", flat=True) + id__in=self.values_list("candidate_reviewer", flat=True) ) def with_reviewer_statistics(self):