Skip to content

Conversation

@cathteng
Copy link
Member

@cathteng cathteng commented Jan 4, 2023

Adds organization_slug to the url for the SharedGroupDetails endpoint. Keeps the original orgless URL for now.

Also fixes the GroupParticipants url with organization_slug to have the /organizations/{organization_slug}/... format

For HC-516

@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Jan 4, 2023
@cathteng cathteng marked this pull request as ready for review January 4, 2023 22:47
@cathteng cathteng requested a review from a team as a code owner January 4, 2023 22:47
@cathteng cathteng requested review from a team and RyanSkonnord January 4, 2023 22:47
name="sentry-api-0-shared-group-details",
),
url(
r"^shared/(?P<organization_slug>[^\/]+)/(?:issues|groups)/(?P<share_id>[^\/]+)/$",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a little concerned about placing the organization slug at various places inside the URL structure, rather than following the "REST-ful noun" pattern of organizations/(?P<organization_slug>[^\/]+) at the beginning. (I'm not sure how many other mappings are already doing this -- I noticed at least one, after I'd already hit "approve" on it the other day.)

In general, it seems like it's inviting ambiguity. In this case, it looks like we'd be relying on not having an org with "issues" or "groups" as its slug (I don't know if those words are already included in reserved slugs).

When adding org slugs to mappings, would it be plausible to just always follow the pattern of "organizations/ + org slug + old mapping"? Or am I assuming too much?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it should definitely be possible... i can follow up this PR with another one to modify the ones i've already changed

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see #42825

@vercel
Copy link

vercel bot commented Jan 5, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
sentry 🔄 Building (Inspect) Jan 5, 2023 at 10:55PM (UTC)
storybook 🔄 Building (Inspect) Jan 5, 2023 at 10:55PM (UTC)

Comment on lines -2323 to +2324
r"^issues/(?P<organization_slug>[^\/]+)/(?P<issue_id>[^\/]+)/participants/$",
r"^organizations/(?P<organization_slug>[^\/]+)/issues/(?P<issue_id>[^\/]+)/participants/$",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need the old URL for participants?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'm keeping it for now since it's getting some occasional hits

@cathteng cathteng merged commit bbe0605 into master Jan 6, 2023
@cathteng cathteng deleted the cathy/hc/convert-orgless-shared-group-details-api branch January 6, 2023 23:53
@github-actions github-actions bot locked and limited conversation to collaborators Jan 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Scope: Backend Automatically applied to PRs that change backend components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants