Skip to content

Commit ee3e0be

Browse files
DominikB2014claude
andauthored
ref(dashboards): Use favorited column for favorite status instead of row existence (#110204)
Use the `favorited` boolean column on `DashboardFavoriteUser` to determine favorite status, rather than checking for existence of a row in the table. Previously, unfavoriting a dashboard deleted the `DashboardFavoriteUser` row entirely. Now it sets `favorited=False` and clears the position, preserving the row for potential re-favoriting. This avoids row churn and makes the favorite state explicit. Changes: - All manager queries (`get_favorite_dashboards`, `get_favorite_dashboard`, `get_last_position`, `reorder_favorite_dashboards`) now filter by `favorited=True` - `insert_favorite_dashboard` reactivates existing unfavorited entries instead of always creating new rows - Renamed `delete_favorite_dashboard` → `unfavorite_dashboard` to reflect that the row is no longer deleted - Updated serializer, list endpoint filters, sort, and pin-by-favorites queries to scope by `favorited=True` - Fixed race condition in `insert_favorite_dashboard` using `select_for_update()` to prevent concurrent calls from causing `IntegrityError` - Fixed position assignment bug where re-favoriting the first dashboard after unfavoriting all others assigned position 1 instead of 0 due to a global unscoped count check Refs DAIN-1287 --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
1 parent 775f265 commit ee3e0be

File tree

5 files changed

+100
-38
lines changed

5 files changed

+100
-38
lines changed

src/sentry/api/serializers/models/dashboard.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -522,7 +522,7 @@ def get_attrs(self, item_list, user, **kwargs):
522522

523523
favorited_dashboard_ids = set(
524524
DashboardFavoriteUser.objects.filter(
525-
user_id=user.id, dashboard_id__in=item_dict.keys()
525+
user_id=user.id, dashboard_id__in=item_dict.keys(), favorited=True
526526
).values_list("dashboard_id", flat=True)
527527
)
528528

src/sentry/dashboards/endpoints/organization_dashboard_details.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -296,7 +296,7 @@ def put(
296296
dashboard=dashboard,
297297
)
298298
else:
299-
DashboardFavoriteUser.objects.delete_favorite_dashboard(
299+
DashboardFavoriteUser.objects.unfavorite_dashboard(
300300
organization=organization,
301301
user_id=request.user.id,
302302
dashboard=dashboard,

src/sentry/dashboards/endpoints/organization_dashboards.py

Lines changed: 15 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313
IntegerField,
1414
OrderBy,
1515
OuterRef,
16+
Q,
1617
Subquery,
1718
Value,
1819
When,
@@ -374,9 +375,15 @@ def get(self, request: Request, organization: Organization) -> Response:
374375
dashboards = Dashboard.objects.filter(organization_id=organization.id)
375376
for f in filters:
376377
if f == "onlyFavorites":
377-
dashboards = dashboards.filter(dashboardfavoriteuser__user_id=request.user.id)
378+
dashboards = dashboards.filter(
379+
dashboardfavoriteuser__user_id=request.user.id,
380+
dashboardfavoriteuser__favorited=True,
381+
)
378382
elif f == "excludeFavorites":
379-
dashboards = dashboards.exclude(dashboardfavoriteuser__user_id=request.user.id)
383+
dashboards = dashboards.exclude(
384+
dashboardfavoriteuser__user_id=request.user.id,
385+
dashboardfavoriteuser__favorited=True,
386+
)
380387
elif f == "owned":
381388
dashboards = dashboards.filter(created_by_id=request.user.id)
382389
elif f == "shared":
@@ -493,7 +500,11 @@ def get(self, request: Request, organization: Organization) -> Response:
493500
"organizations:dashboards-starred-reordering", organization, actor=request.user
494501
):
495502
dashboards = dashboards.annotate(
496-
favorites_count=Count("dashboardfavoriteuser", distinct=True)
503+
favorites_count=Count(
504+
"dashboardfavoriteuser",
505+
filter=Q(dashboardfavoriteuser__favorited=True),
506+
distinct=True,
507+
)
497508
)
498509
order_by = [
499510
"favorites_count" if desc else "-favorites_count",
@@ -506,7 +517,7 @@ def get(self, request: Request, organization: Organization) -> Response:
506517
pin_by = request.query_params.get("pin")
507518
if pin_by == "favorites":
508519
favorited_by_subquery = DashboardFavoriteUser.objects.filter(
509-
dashboard=OuterRef("pk"), user_id=request.user.id
520+
dashboard=OuterRef("pk"), user_id=request.user.id, favorited=True
510521
)
511522

512523
order_by_favorites = [

src/sentry/models/dashboard.py

Lines changed: 76 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,7 @@ def get_last_position(self, organization: Organization, user_id: int) -> int:
4545
self.filter(
4646
organization=organization,
4747
user_id=user_id,
48+
favorited=True,
4849
position__isnull=False,
4950
)
5051
.order_by("-position")
@@ -60,7 +61,7 @@ def get_favorite_dashboards(
6061
"""
6162
Returns all favorited dashboards for a user in an organization.
6263
"""
63-
return self.filter(organization=organization, user_id=user_id).order_by(
64+
return self.filter(organization=organization, user_id=user_id, favorited=True).order_by(
6465
"position", "dashboard__title"
6566
)
6667

@@ -70,7 +71,9 @@ def get_favorite_dashboard(
7071
"""
7172
Returns the favorite dashboard if it exists, otherwise None.
7273
"""
73-
return self.filter(organization=organization, user_id=user_id, dashboard=dashboard).first()
74+
return self.filter(
75+
organization=organization, user_id=user_id, dashboard=dashboard, favorited=True
76+
).first()
7477

7578
def reorder_favorite_dashboards(
7679
self, organization: Organization, user_id: int, new_dashboard_positions: list[int]
@@ -90,6 +93,7 @@ def reorder_favorite_dashboards(
9093
existing_favorite_dashboards = self.filter(
9194
organization=organization,
9295
user_id=user_id,
96+
favorited=True,
9397
)
9498

9599
existing_dashboard_ids = {
@@ -141,33 +145,52 @@ def insert_favorite_dashboard(
141145
True if the dashboard was favorited, False if the dashboard was already favorited
142146
"""
143147
with transaction.atomic(using=router.db_for_write(DashboardFavoriteUser)):
144-
if self.get_favorite_dashboard(organization, user_id, dashboard):
148+
# Lock any existing row to prevent concurrent insert races
149+
existing = (
150+
self.filter(
151+
organization=organization,
152+
user_id=user_id,
153+
dashboard=dashboard,
154+
)
155+
.select_for_update()
156+
.first()
157+
)
158+
159+
if existing and existing.favorited:
145160
return False
146161

147-
if self.count() == 0:
162+
existing_favorites = self.filter(
163+
organization=organization, user_id=user_id, favorited=True
164+
)
165+
if not existing_favorites.exists():
148166
position = 0
149167
else:
150168
position = self.get_last_position(organization, user_id) + 1
151169

152-
self.create(
153-
organization=organization,
154-
user_id=user_id,
155-
dashboard=dashboard,
156-
position=position,
157-
)
170+
if existing:
171+
existing.favorited = True
172+
existing.position = position
173+
existing.save(update_fields=["favorited", "position"])
174+
else:
175+
self.create(
176+
organization=organization,
177+
user_id=user_id,
178+
dashboard=dashboard,
179+
position=position,
180+
)
158181
return True
159182

160-
def delete_favorite_dashboard(
183+
def unfavorite_dashboard(
161184
self, organization: Organization, user_id: int, dashboard: Dashboard
162185
) -> bool:
163186
"""
164-
Deletes a favorited dashboard from the list.
165-
Decrements the position of all dashboards after the deletion point.
187+
Unfavorites a dashboard by setting favorited=False and clearing its position.
188+
Decrements the position of all remaining favorited dashboards after the removal point.
166189
167190
Args:
168191
organization: The organization the dashboards belong to
169192
user_id: The ID of the user whose favorited dashboards are being updated
170-
dashboard: The dashboard to delete
193+
dashboard: The dashboard to unfavorite
171194
172195
Returns:
173196
True if the dashboard was unfavorited, False if the dashboard was already unfavorited
@@ -177,10 +200,15 @@ def delete_favorite_dashboard(
177200
return False
178201

179202
deleted_position = favorite.position
180-
favorite.delete()
203+
favorite.favorited = False
204+
favorite.position = None
205+
favorite.save(update_fields=["favorited", "position"])
181206

182207
self.filter(
183-
organization=organization, user_id=user_id, position__gt=deleted_position
208+
organization=organization,
209+
user_id=user_id,
210+
favorited=True,
211+
position__gt=deleted_position,
184212
).update(position=models.F("position") - 1)
185213
return True
186214

@@ -265,7 +293,7 @@ def favorited_by(self):
265293
"""
266294
@deprecated Use the DashboardFavoriteUser object manager instead.
267295
"""
268-
user_ids = DashboardFavoriteUser.objects.filter(dashboard=self).values_list(
296+
user_ids = DashboardFavoriteUser.objects.filter(dashboard=self, favorited=True).values_list(
269297
"user_id", flat=True
270298
)
271299
return user_ids
@@ -277,20 +305,38 @@ def favorited_by(self, user_ids):
277305
"""
278306
from django.db import router, transaction
279307

280-
existing_user_ids = DashboardFavoriteUser.objects.filter(dashboard=self).values_list(
281-
"user_id", flat=True
282-
)
308+
existing_favorites = DashboardFavoriteUser.objects.filter(dashboard=self)
309+
existing_user_ids = set(existing_favorites.values_list("user_id", flat=True))
310+
new_user_ids = set(user_ids)
311+
283312
with transaction.atomic(using=router.db_for_write(DashboardFavoriteUser)):
284-
newly_favourited = [
285-
DashboardFavoriteUser(
286-
dashboard=self, user_id=user_id, organization=self.organization
287-
)
288-
for user_id in set(user_ids) - set(existing_user_ids)
289-
]
290-
DashboardFavoriteUser.objects.filter(
291-
dashboard=self, user_id__in=set(existing_user_ids) - set(user_ids)
292-
).delete()
293-
DashboardFavoriteUser.objects.bulk_create(newly_favourited)
313+
existing_favorites.filter(
314+
user_id__in=existing_user_ids - new_user_ids, favorited=True
315+
).update(favorited=False, position=None)
316+
317+
users_to_add = new_user_ids - existing_user_ids
318+
users_to_refavorite = new_user_ids & existing_user_ids
319+
320+
for user_id in users_to_refavorite:
321+
fav = existing_favorites.filter(user_id=user_id, favorited=False).first()
322+
if fav:
323+
last_position = DashboardFavoriteUser.objects.get_last_position(
324+
self.organization, user_id
325+
)
326+
has_any = DashboardFavoriteUser.objects.filter(
327+
organization=self.organization, user_id=user_id, favorited=True
328+
).exists()
329+
fav.favorited = True
330+
fav.position = 0 if not has_any else last_position + 1
331+
fav.save(update_fields=["favorited", "position"])
332+
DashboardFavoriteUser.objects.bulk_create(
333+
[
334+
DashboardFavoriteUser(
335+
dashboard=self, user_id=user_id, organization=self.organization
336+
)
337+
for user_id in users_to_add
338+
]
339+
)
294340

295341
@staticmethod
296342
def get_prebuilt_list(organization, user, title_query=None):

tests/sentry/models/test_dashboard.py

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -172,13 +172,18 @@ def test_deletes_and_increments_existing_positions(self) -> None:
172172

173173
assert DashboardFavoriteUser.objects.count() == 2
174174

175-
DashboardFavoriteUser.objects.delete_favorite_dashboard(
175+
DashboardFavoriteUser.objects.unfavorite_dashboard(
176176
self.organization, self.user.id, first_dashboard
177177
)
178178

179179
dashboard = DashboardFavoriteUser.objects.get_favorite_dashboard(
180180
self.organization, self.user.id, second_dashboard
181181
)
182-
assert DashboardFavoriteUser.objects.count() == 1
182+
# Row still exists but with favorited=False
183+
assert DashboardFavoriteUser.objects.count() == 2
184+
assert DashboardFavoriteUser.objects.filter(favorited=True).count() == 1
185+
unfavorited = DashboardFavoriteUser.objects.get(dashboard=first_dashboard)
186+
assert unfavorited.favorited is False
187+
assert unfavorited.position is None
183188
assert dashboard is not None
184189
assert dashboard.position == 0

0 commit comments

Comments
 (0)