feat(shared-views): Add endpoint to star and unstar views #87463
Conversation
github-actions
bot
added
the
Scope: Backend
| try: | ||
| view = GroupSearchView.objects.get(id=view_id, organization=organization) | ||
| except GroupSearchView.DoesNotExist: | ||
| return Response(status=status.HTTP_404_NOT_FOUND) | ||
|
|
||
| if not ( | ||
| view.user_id == request.user.id | ||
| or view.visibility == GroupSearchViewVisibility.ORGANIZATION | ||
| ): | ||
| return Response(status=status.HTTP_404_NOT_FOUND) |
There was a problem hiding this comment.
should we create a OrganizationGroupSearchViewBase endpoint that validates this? (similar to how say how member or group endpoint have this)
There was a problem hiding this comment.
This is a great idea actually! Though this should probably be in its own PR, I’ll add it to my todo list
| @@ -48,6 +65,77 @@ def reorder_starred_views( | |||
| if views_to_update: | |||
| self.bulk_update(views_to_update, ["position"]) | |||
|
|
|||
| def insert_starred_view( | |||
There was a problem hiding this comment.
do we need to put this function in a db transaction? (can use transaction.atomic decorator if so).
There was a problem hiding this comment.
I don’t think this set of db updates is particularly high risk, but I suppose it can’t hurt. Is there any downside to running it in a transaction that I’m not thinking of?
There was a problem hiding this comment.
just thinking if two requests hit this endpoint simultaneously, could that put things into a weird state? if no, no need. i don't think there's a huge downside to putting it in a transaction at the scale of traffic this endpoint will likely get.
There was a problem hiding this comment.
hmm yeah It's possible if two endpoints try to star a view and add it in the same position at the same time, then the later request might hit a unique constraint violation. I imagine the odds of that are exceedingly small given the traffic for this endpoint, but better safe than sorry I suppose.
| ValueError: If the view is already starred | ||
| """ | ||
| if self.get_starred_view(organization, user_id, view): | ||
| raise ValueError("View is already starred.") |
There was a problem hiding this comment.
in both cases where we raise a ValueError we catch it then continue -- maybe we should just return instead?
can see chapter 10, define errors out of existence in "A Philosophy of Software Design" for an opinion ons on errors and interfaces https://milkov.tech/assets/psd.pdf
There was a problem hiding this comment.
I thought about that, and you're right that the endpoint in this PR happens to not care if it gets raised or not, but I figured that since function is on the model manager and is designed to be reusable, it would be better if it took the least opinionated/most verbose approach possible.
There was a problem hiding this comment.
i think it would be fair to return for now -- if you don't need it now, whoever is using the obj manager method in the future can add that error in and handle it. one other thing you could do is return a boolean saying whether or not the view was creator or not, and for now you don't need to use it.
relevant links for thought:
|
|
||
| def get_starred_view( | ||
| self, organization: Organization, user_id: int, view: GroupSearchView | ||
| ) -> "GroupSearchViewStarred | None": |
There was a problem hiding this comment.
if you put from __future__ import annotations at top of file i don't think you need the quotes around the return types?
This PR adds the
POST/group-search-views/:viewId/starredendpoint, which exposes the ability to star and unstar a view.At a high level the logic is as follows:
starredquery param istrue:a. Do nothing if the view was already starred.
b. If position is provided, insert at that position and increment any new views
c. if position is not provided, star the view and insert at the end of the list.
starredquery param isfalse, do nothing if its already unstarred, otherwise unstar the view.