feat: update maintainers from ignored packages in the view

Author: fricklerhandwerk

src/shared/github.py

@@ -68,32 +68,14 @@ def cvss_details() -> str:
             return ""
 
     def maintainers() -> str:
-        # Get all maintainer github_ids from currently active packages
-        active_package_maintainer_ids = {
-            maintainer["github_id"]
-            for package in cached_suggestion.payload["packages"].values()
-            for maintainer in package["maintainers"]
-            if "github_id" in maintainer
-        }
-
-        # Filter active maintainers to only those still in active packages
-        filtered_active_maintainers = [
-            maintainer
-            for maintainer in cached_suggestion.payload["categorized_maintainers"][
-                "active"
-            ]
-            if maintainer["github_id"] in active_package_maintainer_ids
-        ]
+        maintainers = cached_suggestion.payload["categorized_maintainers"]
 
         # We need to query for the latest username of each maintainer, because
         # those might have changed since they were written out in Nixpkgs; since
         # we have the user id (which is stable), we can ask the GitHub API
         maintainers_list = [
             get_maintainer_username(maintainer, github)
-            for maintainer in (
-                filtered_active_maintainers
-                + cached_suggestion.payload["categorized_maintainers"]["added"]
-            )
+            for maintainer in (maintainers["active"] + maintainers["added"])
             if "github_id" in maintainer and "github" in maintainer
         ]
 

src/shared/listeners/cache_suggestions.py

@@ -195,10 +195,6 @@ def cache_new_suggestions(suggestion: CVEDerivationClusterProposal) -> None:
     )
     package_edits = list(suggestion.package_edits.all())
     packages = apply_package_edits(original_packages, package_edits)
-    # FIXME(@fricklerhandwerk): We should just pass `packages`, but a tangled legacy view is still using this seemingly internal function and wants to pass a dict.
-    categorized_maintainers = categorize_maintainers(
-        original_packages, maintainers_edits
-    )
 
     only_relevant_data = CachedSuggestion(
         pk=suggestion.pk,
@@ -211,7 +207,7 @@ def cache_new_suggestions(suggestion: CVEDerivationClusterProposal) -> None:
         original_packages=packages,
         packages=packages,
         metrics=[to_dict(m) for m in prefetched_metrics],
-        categorized_maintainers=categorized_maintainers,
+        categorized_maintainers=categorize_maintainers(packages, maintainers_edits),
     )
 
     _, created = CachedSuggestions.objects.update_or_create(
@@ -425,19 +421,17 @@ def maintainers_list(packages: dict, edits: list[MaintainersEdit]) -> list[dict]
 
 
 def categorize_maintainers(
-    original_packages: dict[str, CachedSuggestion.Package],
+    packages: dict[str, CachedSuggestion.Package],
     maintainers_edits: list[MaintainersEdit],
 ) -> CachedSuggestion.CategorizedMaintainers:
     """
-    Categorize maintainers associated the packages of a suggestion.
+    Categorize maintainers associated to the packages of a suggestion.
     """
     # Collect all original maintainers from packages (deduplicated by github_id)
     original_maintainers_dict: dict[int, dict] = {}
-    for package in original_packages.values():
-        for maintainer_dict in package.maintainers:
-            github_id = maintainer_dict["github_id"]
-            if github_id not in original_maintainers_dict:
-                original_maintainers_dict[github_id] = maintainer_dict
+    for package in packages.values():
+        for maintainer in package.maintainers:
+            original_maintainers_dict[maintainer["github_id"]] = maintainer
 
     original_maintainers = list(original_maintainers_dict.values())
 

src/webview/suggestions/views/base.py

@@ -113,6 +113,9 @@ class ForbiddenOperationError(Exception):
         def __init__(self, response: HttpResponse) -> None:
             self.error = response
 
+    # FIXME(@fricklerhandwerk): This conflates access control with database queries and bypasses the standard Django mechanism of overriding the respective view methods.
+    # The main problem here is that it results in very inefficient queries, as we can't express fetching related data in one go.
+    # A minor problem for now is obscured and coarse-grained access control, but there's no user story at the moment for which it's in the way.
     def _check_access_rights_and_get_suggestion(
         self, request: HttpRequest, suggestion_id: int
     ) -> tuple[CVEDerivationClusterProposal, SuggestionContext]:

src/webview/suggestions/views/packages.py

@@ -3,12 +3,16 @@
 from django.db import transaction
 from django.http import HttpRequest, HttpResponse
 
-from shared.listeners.cache_suggestions import apply_package_edits
+from shared.listeners.cache_suggestions import (
+    CachedSuggestion,
+    apply_package_edits,
+    categorize_maintainers,
+)
 from shared.models.linkage import (
     CVEDerivationClusterProposal,
-    PackageEdit,
 )
 from webview.suggestions.context.builders import (
+    get_maintainer_list_context,
     get_package_list_context,
 )
 
@@ -40,11 +44,19 @@ def post(
         try:
             with transaction.atomic():
                 self._perform_operation(suggestion, package_attr)
-                new_active_packages = apply_package_edits(
+                suggestion.cached.payload["packages"] = apply_package_edits(
                     suggestion.cached.payload["original_packages"],
                     suggestion.package_edits.all(),
                 )
-                suggestion.cached.payload["packages"] = new_active_packages
+                suggestion.cached.payload["categorized_maintainers"] = (
+                    categorize_maintainers(
+                        {
+                            k: CachedSuggestion.Package.model_validate(v)
+                            for k, v in suggestion.cached.payload["packages"].items()
+                        },
+                        suggestion.maintainers_edits.all(),
+                    ).model_dump()
+                )
                 suggestion.cached.save()
         except Exception:
             return self._handle_error(
@@ -55,6 +67,9 @@ def post(
 
         # Refresh the package list context and activity log
         suggestion_context.package_list_context = get_package_list_context(suggestion)
+        suggestion_context.maintainer_list_context = get_maintainer_list_context(
+            suggestion
+        )
         suggestion_context.activity_log = fetch_activity_log(suggestion.pk)
 
         # Handle response based on request type

src/webview/templates/suggestions/components/maintainers_list.html

@@ -11,7 +11,6 @@
       <!-- Active maintainers -->
       <div class="row gap spread wrap baseline">
         <h2 class="heading dimmed">Package maintainers</h2>
-        {% if data.editable and user|is_maintainer_or_admin %}<p class="dimmed">Maintainers of ignored packages won't be notified.</p>{% endif %}
       </div>
       {% if data.active %}
         <ul id="maintainers-list-{{ data.suggestion_id }}-active" class="column gap-small">

src/webview/tests/test_maintainers.py

@@ -135,7 +135,6 @@ def test_add_maintainer_from_invalid_github_handle_returns_error(
     expect(error).to_be_visible()
 
 
-@pytest.mark.xfail(reason="Not implemented")
 @pytest.mark.parametrize(
     "ignore_maintainer",
     [True, False],