feat(dashboards): add new endpoint for favourite dashboards #81438
Conversation
github-actions
bot
added
the
Scope: Backend
| @@ -252,6 +254,7 @@ def serialize(self, obj, attrs, user, **kwargs) -> DashboardListResponse: | |||
| "widgetDisplay": attrs.get("widget_display", []), | |||
| "widgetPreview": attrs.get("widget_preview", []), | |||
| "permissions": serialize(obj.permissions) if hasattr(obj, "permissions") else None, | |||
| "isFavourited": user.id in obj.favourited_by, | |||
There was a problem hiding this comment.
note to self: what happens when favourited_by doesn't exist in dashboard obj
This comment was marked as outdated.
This comment was marked as outdated.
| if is_favourited and request.user.id not in current_favourites: | ||
| current_favourites.add(request.user.id) # Add user to the set | ||
| elif not is_favourited and request.user.id in current_favourites: | ||
| current_favourites.remove(request.user.id) | ||
| else: | ||
| Response(status=204) |
There was a problem hiding this comment.
conditions:
not fav, not in list -> do nothing
not fav, in lits -> remove frm list
in fav, in list -> do nothing
in fav, not in list -> add to list
| "GET": ApiPublishStatus.PRIVATE, | ||
| } | ||
|
|
||
| def get(self, request: Request, organization, dashboard) -> Response: |
There was a problem hiding this comment.
Do we need to support GET on this endpoint? I thought we wanted to add isFavourited to the dashboard list endpoints, that way we get the value in one call. If we expect to call this endpoint to get the favourite status, we'll make one call for each dashboard result which is an N+1 performance issue
There was a problem hiding this comment.
If this is just to support test validation, we can just check the database values in our tests and once we get support in the dashboard serializers, we can mock the value and show that it's exposed properly
There was a problem hiding this comment.
My idea was to use this when displaying the favourite status for an individual dashboard endpoint which will be used for the star here:
And the PUT request is used when clicking on the star
There was a problem hiding this comment.
Instead, should I just add it to the GET of the dashboard details endpoint?
There was a problem hiding this comment.
Yeah, I think we should just include it in the GET for dashboard details so we can cut down on the number of requests (we're already querying the dashboard with that request anyways). It'll just be ignored in the endpoint when there are updates
| if isinstance(dashboard, dict): | ||
| return Response(status=204) |
There was a problem hiding this comment.
Is this to handle prebuilt dashboards? Our implementation of prebuilt dashboard is a little wonky on the backend, and it doesn't seem like this block actually sets up any relationships. Can we make it so prebuilt ones aren't favouritable, or otherwise we'll have to do something to actually handle favouriting those
There was a problem hiding this comment.
yup, it was for the pre-built dashboards. Would it make sense to keep this check in the GET and PUT methods, and not render the favourite option in the frontend? So, favouriting pre-built dashboards would just do nothing
There was a problem hiding this comment.
We should check with Ale how we expect to handle the prebuilt dashboard, but I'm okay with leaving it in until we figure out what to do with it. If we don't want to let people favourite prebuilt dashboards (we only have one) then I think we should just 400 the request or something
tests/sentry/api/endpoints/test_organization_dashboard_details.py
Outdated
Show resolved
Hide resolved
Co-authored-by: Nar Saynorath <[email protected]>
| Toggle favorite status for current user by adding or removing | ||
| current user from dashboard favorites | ||
| """ | ||
| if not features.has(EDIT_FEATURE, organization, actor=request.user): |
There was a problem hiding this comment.
Can we also add a check for the feature flag so this 404s if they don't have the favourite rollout?
harshithadurai
deleted the
harshi/feat/add-favourite-dashboards-endpoint
branch
Add 'return' to endpoint response for POST request. Should have added it in this PR: #81438
PUTreq)isFavouritefield inDashboardDetailsendpoint'sGETrequest#78023