Feat/mark sentry app installed put route #14060
Conversation
src/sentry/api/bases/sentryapps.py
Outdated
| def has_permission(self, request, *args, **kwargs): | ||
| # To let the app mark the installation as installed, we don't care about permissions | ||
| if request.user.is_sentry_app: | ||
| if request.method == 'PUT': |
There was a problem hiding this comment.
Can probably combine the two above conditions into one.
| ), | ||
| ) | ||
| # convert status to str for comparison | ||
| last_status = SentryAppInstallationStatus.as_str(self.instance.status) |
There was a problem hiding this comment.
If this is used when creating a new SentryAppInstallation, self.instance is going to be None, so we should probably wrap this whole part in if self.instance.
| serializer = SentryAppInstallationSerializer( | ||
| installation, | ||
| data=request.data, | ||
| partial=True, |
There was a problem hiding this comment.
I don't think you need the partial=True here since we expect the status to be in the request.data and that's the only thing you are validating
There was a problem hiding this comment.
It might be good to keep it, though. Updating is almost always going to allow only a portion of the required data (opposed to requiring all fields).
| def validate_status(self, new_status): | ||
| # make sure the status in in our defined list | ||
| try: | ||
| SentryAppInstallationStatus.STATUS_MAP[new_status] |
There was a problem hiding this comment.
so I don't think we necessarily need the STATUS_MAP - if we only allow for updating to be installed we can probs just do that explicitly
if new_status != SentryAppInstallationStatus.INSTALLED:
raise ValidationError(u"Invalid value '{}' for status".format(new_status))That way we don't have to worry about the check below since we don't let anyone explicitly set an installation's status to pending
There was a problem hiding this comment.
@MeredithAnya I talked to @mnoble about this and he said we should let our APIs be idempotent (see https://restfulapi.net/idempotent-rest-apis/). In other words, if the installation is pending we shouldn't throw an error if the request sets it to pending.
There was a problem hiding this comment.
They can write client code in such a way that there can be duplicate requests as well.
To me that's not what we are saying here imo. It seems like pending is a state that is never set directly by the developer. Like they should never be trying to set the status to pending.
There was a problem hiding this comment.
@MeredithAnya I am OK with either approach but I'd like to know what @mnoble thinks before making the change
* master: ref(admin): Convert user edit page to react (#14074) ref: Remove unused Group.get_oldest_event and legacy events behavior (#14038) ref(api): Update DELETE users/ to support hard deleting (#14068) test(OrganizationDiscoverSavedQueryDetailTest): Stabilize put test (#14077) meta(readme): Sentry logo should link to sentry.io (#14076) ref: Remove duplicate column (#14073) App platform/update permissions token auth (#14046) feat: Support issue IDs as canonical parameters ref: Change to new traceparent header for Python SDK (#14070) feat: Use option to force-disable transaction events (#14056) feat(apm): Register option to force-disable transaction events (#14055) Feat/mark sentry app installed put route (#14060) ref: Remove unused Group.event_set property (#14036) fix: Filter out groups that are pending deletion/merge from `by_qualified_short_id` (SEN-849) fix(ui): Fix resolve/ignore actions for accounts without multi… (#14058) Fix: Remove extra $.param introduced in GH-14051 (#14061) feat: Use Snuba for Group.from_event_id (#14034) fix(ui) Display implicit default sort and default to descending (#14042) fix(github) Fix 404s not being handled in repository search (#14030) fix: Pass an empty array to $.param instead of an empty string when options.query is falsey (#14051) # Conflicts: # src/sentry/utils/sdk.py
This PR adds a route to mark a sentry app as installed after receiving an access token from the grant. Here's what the request object should look like in JS:
There is validation on the value of
status. It must either beinstalledorpending. And there is validation to make sure we never go frominstalledtopending.I had to make a hack in
has_permissioninSentryAppInstallationPermissionto ignore the usual permission checks for marking the sentry app as installed. This is because a sentry app that has no permissions should still be able to be marked asinstalledby the proxy hack.