diff --git a/src/sentry/preprod/vcs/pr_comments/snapshot_templates.py b/src/sentry/preprod/vcs/pr_comments/snapshot_templates.py
index 37cb983b96e3..97ca30fb3f98 100644
--- a/src/sentry/preprod/vcs/pr_comments/snapshot_templates.py
+++ b/src/sentry/preprod/vcs/pr_comments/snapshot_templates.py
@@ -46,14 +46,24 @@ def format_snapshot_pr_comment(
total_errored = 0
for artifact in artifacts:
- name_cell = _name_cell(artifact, snapshot_metrics_map, base_artifact_map)
metrics = snapshot_metrics_map.get(artifact.id)
+ comparison = comparisons_map.get(metrics.id) if metrics else None
+ include_selected_types = comparison is not None and comparison.state not in (
+ PreprodSnapshotComparison.State.PENDING,
+ PreprodSnapshotComparison.State.PROCESSING,
+ PreprodSnapshotComparison.State.FAILED,
+ )
+ name_cell = _name_cell(
+ artifact,
+ snapshot_metrics_map,
+ base_artifact_map,
+ comparison=comparison if include_selected_types else None,
+ )
if not metrics:
table_rows.append(f"| {name_cell} | - | - | - | - | - | - | {PROCESSING_STATUS} |")
continue
- comparison = comparisons_map.get(metrics.id)
has_base = artifact.id in base_artifact_map
if not comparison and not has_base:
@@ -75,15 +85,6 @@ def format_snapshot_pr_comment(
elif comparison.state == PreprodSnapshotComparison.State.FAILED:
table_rows.append(f"| {name_cell} | - | - | - | - | - | - | ❌ Comparison failed |")
else:
- base_artifact = base_artifact_map.get(artifact.id)
- artifact_url = (
- get_preprod_artifact_comparison_url(
- artifact, base_artifact, comparison_type="snapshots"
- )
- if base_artifact
- else get_preprod_artifact_url(artifact, view_type="snapshots")
- )
-
has_changes = changes_map.get(artifact.id, False)
is_approved = approvals_map is not None and artifact.id in approvals_map
if has_changes and is_approved:
@@ -95,12 +96,12 @@ def format_snapshot_pr_comment(
table_rows.append(
f"| {name_cell}"
- f" | {_section_cell(comparison.images_added, 'added', artifact_url)}"
- f" | {_section_cell(comparison.images_removed, 'removed', artifact_url)}"
- f" | {_section_cell(comparison.images_changed, 'changed', artifact_url)}"
- f" | {_section_cell(comparison.images_renamed, 'renamed', artifact_url)}"
- f" | {_section_cell(comparison.images_unchanged, 'unchanged', artifact_url)}"
- f" | {_section_cell(comparison.images_skipped, 'skipped', artifact_url)}"
+ f" | {comparison.images_added}"
+ f" | {comparison.images_removed}"
+ f" | {comparison.images_changed}"
+ f" | {comparison.images_renamed}"
+ f" | {comparison.images_unchanged}"
+ f" | {comparison.images_skipped}"
f" | {status} |"
)
total_errored += comparison.images_errored
@@ -118,6 +119,7 @@ def _name_cell(
artifact: PreprodArtifact,
snapshot_metrics_map: dict[int, PreprodSnapshotMetrics],
base_artifact_map: dict[int, PreprodArtifact],
+ comparison: PreprodSnapshotComparison | None = None,
) -> str:
app_display, app_id = _app_display_info(artifact)
metrics = snapshot_metrics_map.get(artifact.id)
@@ -130,6 +132,9 @@ def _name_cell(
else:
artifact_url = get_preprod_artifact_url(artifact, view_type="snapshots")
+ if comparison is not None:
+ artifact_url += _selected_types_query(comparison)
+
return _format_name_cell(app_display, app_id, artifact_url)
@@ -147,10 +152,17 @@ def _format_name_cell(app_display: str, app_id: str, url: str) -> str:
return f"[{app_display}]({url})"
-def _section_cell(count: int, section: str, artifact_url: str) -> str:
- if count > 0:
- return f"[{count}]({artifact_url}?selectedTypes={section})"
- return str(count)
+def _selected_types_query(comparison: PreprodSnapshotComparison) -> str:
+ counts = (
+ ("added", comparison.images_added),
+ ("removed", comparison.images_removed),
+ ("changed", comparison.images_changed),
+ ("renamed", comparison.images_renamed),
+ )
+ selected = [name for name, count in counts if count > 0]
+ if not selected:
+ return ""
+ return f"?selectedTypes={','.join(selected)}"
def _format_settings_link(project: Project) -> str:
diff --git a/src/sentry/preprod/vcs/status_checks/snapshots/templates.py b/src/sentry/preprod/vcs/status_checks/snapshots/templates.py
index 1e87acb1d643..1c6e91f6c74f 100644
--- a/src/sentry/preprod/vcs/status_checks/snapshots/templates.py
+++ b/src/sentry/preprod/vcs/status_checks/snapshots/templates.py
@@ -7,14 +7,13 @@
from sentry.models.project import Project
from sentry.preprod.models import PreprodArtifact, PreprodComparisonApproval
from sentry.preprod.snapshots.models import PreprodSnapshotComparison, PreprodSnapshotMetrics
-from sentry.preprod.url_utils import get_preprod_artifact_comparison_url, get_preprod_artifact_url
+from sentry.preprod.url_utils import get_preprod_artifact_url
from sentry.preprod.vcs.pr_comments.snapshot_templates import (
COMPARISON_TABLE_HEADER,
PROCESSING_STATUS,
_app_display_info,
_format_name_cell,
_name_cell,
- _section_cell,
format_errored_note,
)
@@ -284,14 +283,23 @@ def _format_snapshot_summary(
table_rows = []
for artifact in artifacts:
- name = _name_cell(artifact, snapshot_metrics_map, base_artifact_map)
-
metrics = snapshot_metrics_map.get(artifact.id)
+ comparison = comparisons_map.get(metrics.id) if metrics else None
+ include_selected_types = comparison is not None and comparison.state not in (
+ PreprodSnapshotComparison.State.PENDING,
+ PreprodSnapshotComparison.State.PROCESSING,
+ )
+ name = _name_cell(
+ artifact,
+ snapshot_metrics_map,
+ base_artifact_map,
+ comparison=comparison if include_selected_types else None,
+ )
+
if not metrics:
table_rows.append(f"| {name} | - | - | - | - | - | - | {PROCESSING_STATUS} |")
continue
- comparison = comparisons_map.get(metrics.id)
if not comparison:
table_rows.append(f"| {name} | - | - | - | - | - | - | {PROCESSING_STATUS} |")
continue
@@ -302,15 +310,6 @@ def _format_snapshot_summary(
):
table_rows.append(f"| {name} | - | - | - | - | - | - | {PROCESSING_STATUS} |")
else:
- base_artifact = base_artifact_map.get(artifact.id)
- artifact_url = (
- get_preprod_artifact_comparison_url(
- artifact, base_artifact, comparison_type="snapshots"
- )
- if base_artifact
- else get_preprod_artifact_url(artifact, view_type="snapshots")
- )
-
has_changes = changes_map.get(artifact.id, False)
is_approved = approvals_map is not None and artifact.id in approvals_map
if has_changes and is_approved:
@@ -322,12 +321,12 @@ def _format_snapshot_summary(
table_rows.append(
f"| {name}"
- f" | {_section_cell(comparison.images_added, 'added', artifact_url)}"
- f" | {_section_cell(comparison.images_removed, 'removed', artifact_url)}"
- f" | {_section_cell(comparison.images_changed, 'changed', artifact_url)}"
- f" | {_section_cell(comparison.images_renamed, 'renamed', artifact_url)}"
- f" | {_section_cell(comparison.images_unchanged, 'unchanged', artifact_url)}"
- f" | {_section_cell(comparison.images_skipped, 'skipped', artifact_url)}"
+ f" | {comparison.images_added}"
+ f" | {comparison.images_removed}"
+ f" | {comparison.images_changed}"
+ f" | {comparison.images_renamed}"
+ f" | {comparison.images_unchanged}"
+ f" | {comparison.images_skipped}"
f" | {status} |"
)
diff --git a/tests/sentry/preprod/vcs/pr_comments/test_snapshot_templates.py b/tests/sentry/preprod/vcs/pr_comments/test_snapshot_templates.py
index d9545d5157d4..43f85fbedddc 100644
--- a/tests/sentry/preprod/vcs/pr_comments/test_snapshot_templates.py
+++ b/tests/sentry/preprod/vcs/pr_comments/test_snapshot_templates.py
@@ -1,10 +1,13 @@
from __future__ import annotations
+from types import SimpleNamespace
+
import pytest
from sentry.preprod.models import PreprodArtifact, PreprodComparisonApproval
from sentry.preprod.snapshots.models import PreprodSnapshotComparison, PreprodSnapshotMetrics
from sentry.preprod.vcs.pr_comments.snapshot_templates import (
+ _selected_types_query,
format_missing_base_snapshot_pr_comment,
format_snapshot_pr_comment,
format_solo_snapshot_pr_comment,
@@ -14,6 +17,32 @@
from sentry.testutils.silo import cell_silo_test
+def _comparison(
+ *, changed: int = 0, added: int = 0, removed: int = 0, renamed: int = 0
+) -> PreprodSnapshotComparison:
+ return SimpleNamespace(
+ images_changed=changed,
+ images_added=added,
+ images_removed=removed,
+ images_renamed=renamed,
+ ) # type: ignore[return-value]
+
+
+def test_selected_types_query_all_categories() -> None:
+ assert (
+ _selected_types_query(_comparison(changed=1, added=2, removed=3, renamed=4))
+ == "?selectedTypes=added,removed,changed,renamed"
+ )
+
+
+def test_selected_types_query_subset_preserves_order() -> None:
+ assert _selected_types_query(_comparison(changed=1, added=1)) == "?selectedTypes=added,changed"
+
+
+def test_selected_types_query_none_changed_is_empty() -> None:
+ assert _selected_types_query(_comparison()) == ""
+
+
class SnapshotPrCommentTestBase(TestCase):
def setUp(self) -> None:
super().setUp()
@@ -251,10 +280,9 @@ def test_no_changes_shows_unchanged(self) -> None:
assert "Unchanged" in result
assert "My App" in result
assert "`com.example.app`" in result
- # Zero counts are plain text, non-zero unchanged is linked
assert "| 0 | 0 | 0 | 0 |" in result
assert "| 0 | ✅ Unchanged |" in result
- assert "?selectedTypes=unchanged" in result
+ assert "?selectedTypes=" not in result
def test_changes_show_needs_approval(self) -> None:
head_artifact, head_metrics = self._create_artifact_with_metrics()
@@ -280,10 +308,7 @@ def test_changes_show_needs_approval(self) -> None:
)
assert "Needs approval" in result
- assert "?selectedTypes=added" in result
- assert "?selectedTypes=removed" in result
- assert "?selectedTypes=changed" in result
- assert "?selectedTypes=renamed" in result
+ assert "?selectedTypes=added,removed,changed,renamed" in result
def test_approved_shows_approved_status(self) -> None:
head_artifact, head_metrics = self._create_artifact_with_metrics()
diff --git a/tests/sentry/preprod/vcs/status_checks/snapshots/test_templates.py b/tests/sentry/preprod/vcs/status_checks/snapshots/test_templates.py
index 3e8f5a739017..e8a61f91747b 100644
--- a/tests/sentry/preprod/vcs/status_checks/snapshots/test_templates.py
+++ b/tests/sentry/preprod/vcs/status_checks/snapshots/test_templates.py
@@ -725,9 +725,7 @@ def test_full_success_summary_format(self) -> None:
"| Name | Added | Removed | Changed | Renamed | Unchanged | Skipped | Status |\n"
"| :--- | :---: | :---: | :---: | :---: | :---: | :---: | :---: |\n"
f"| [My App]({artifact_url})
`com.example.app`"
- f" | 0 | 0 | 0 | 0"
- f" | [{15}]({artifact_url}?selectedTypes=unchanged)"
- f" | 0"
+ f" | 0 | 0 | 0 | 0 | 15 | 0"
f" | ✅ Unchanged |"
f"\n\n{configure_link}"
)
@@ -771,13 +769,8 @@ def test_full_failure_summary_format(self) -> None:
expected = (
"| Name | Added | Removed | Changed | Renamed | Unchanged | Skipped | Status |\n"
"| :--- | :---: | :---: | :---: | :---: | :---: | :---: | :---: |\n"
- f"| [My App]({artifact_url})
`com.example.app`"
- f" | [{1}]({artifact_url}?selectedTypes=added)"
- f" | [{2}]({artifact_url}?selectedTypes=removed)"
- f" | [{3}]({artifact_url}?selectedTypes=changed)"
- f" | [{1}]({artifact_url}?selectedTypes=renamed)"
- f" | [{4}]({artifact_url}?selectedTypes=unchanged)"
- f" | 0"
+ f"| [My App]({artifact_url}?selectedTypes=added,removed,changed,renamed)
`com.example.app`"
+ f" | 1 | 2 | 3 | 1 | 4 | 0"
f" | ⏳ Needs approval |"
f"\n\n{configure_link}"
)
@@ -1143,13 +1136,8 @@ def test_approved_summary_table_format(self) -> None:
expected = (
"| Name | Added | Removed | Changed | Renamed | Unchanged | Skipped | Status |\n"
"| :--- | :---: | :---: | :---: | :---: | :---: | :---: | :---: |\n"
- f"| [My App]({artifact_url})
`com.example.app`"
- f" | [{1}]({artifact_url}?selectedTypes=added)"
- f" | [{2}]({artifact_url}?selectedTypes=removed)"
- f" | [{3}]({artifact_url}?selectedTypes=changed)"
- f" | [{1}]({artifact_url}?selectedTypes=renamed)"
- f" | [{4}]({artifact_url}?selectedTypes=unchanged)"
- f" | 0"
+ f"| [My App]({artifact_url}?selectedTypes=added,removed,changed,renamed)
`com.example.app`"
+ f" | 1 | 2 | 3 | 1 | 4 | 0"
f" | ✅ Approved |"
f"\n\n{configure_link}"
)