-
-
Notifications
You must be signed in to change notification settings - Fork 4.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(view-sharing): Add new endpoint to delete single view #86157
base: master
Are you sure you want to change the base?
Conversation
44c6ce3
to
4362f16
Compare
4362f16
to
300f021
Compare
tests/sentry/issues/endpoints/test_organization_group_search_views_details.py
Outdated
Show resolved
Hide resolved
except GroupSearchView.DoesNotExist: | ||
return Response(status=status.HTTP_404_NOT_FOUND) | ||
|
||
# Check if the view is starred by the current user |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the reasoning for not just letting them do this and deleting the star?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're moving the delete and duplicate options (the two options that alter the number of views) to within the all views page. The idea is that you should be more intentional about changing the state of your views (though this may change in the future).
This also saves some complexity wrt making the positions correct. We don't have to worry about that as much if the only entry point to creating/deleting a starred view is via the bulk update function, but if we allow someone to directly delete/create a starred view, then we'll need to also decrement/increment (respectively) the position of the views after it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm I'm not sure that this is necessary for deletion. We did discuss that we shouldn't allow deleting from the list of starred views, but I think you should still be able to delete a view without first unstarring it. The positions should be okay to remain the same since the ordering will be unchanged, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that's true, deleting a starred would always retain the correct ordering of the views. Though I think it may still be worth it to update the positions anyways to make sure they are always perfectly in sync with the positions on the frontend.
I'm afraid of a future where we forget that its possible for positions to be in a desynced state, and then we make a change that relies on them being in sync, causing the ordering to get all out of wack. I could totally be too paranoid, but that would be very messy situation to fix. I'm curious what y'all think though.
tests/sentry/issues/endpoints/test_organization_group_search_views_details.py
Outdated
Show resolved
Hide resolved
…iews_details.py Co-authored-by: Matt Duncan <[email protected]>
❌ 1 Tests Failed:
View the top 1 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
As part of the shared view project, we will now need to be able to delete a single view.
We can't use the old endpoint because that one doesn't contain a
viewId
url param, so we need a new endpoint specifically to access a single resource.