ref(teams): Use ProjectTeam relation instead of project.team #6807

macqueen · 2017-12-20T01:15:12Z

#6147 needs to be merged first

macqueen · 2017-12-20T01:21:43Z

src/sentry/models/deletedproject.py

not sure if these should just be removed and project/org is detailed enough?

Yeah, I think it'd be fair to drop this.

ok, will stop writing to it in this pr (a3e5145) and put up #6856 to remove it completely

macqueen · 2017-12-20T01:22:52Z

src/sentry/plugins/sentry_mail/activity/base.py

i'm not sure what the implications are of just removing these

ghost · 2017-12-20T01:37:14Z

	2 Warnings
⚠️	Changes require @getsentry/security sign-off
⚠️	You should update CHANGES due to the size of this PR

Security concerns found

src/sentry/tasks/email.py

Generated by 🚫 danger

dcramer · 2017-12-21T21:57:33Z

src/sentry/api/bases/project.py

TIL what prefetch_related does

macqueen · 2018-01-03T23:25:03Z

src/sentry/api/serializers/models/project.py

+            ).select_related('team')
+        )
+
+        # TODO(jess): decide if we should only include teams the user


gonna say it's ok to include teams a user doesn't have access to since we allow users to see team names and request access to teams they don't have access to

Yeah, I think that's reasonable. Teams aren't private, at least knowing they exist. Only the contents within.

macqueen · 2018-01-03T23:54:46Z

src/sentry/plugins/sentry_mail/activity/base.py

        group = self.group

        headers = {
-            'X-Sentry-Team': project.team.slug,


Talked to Ted about these. I tried to figure out how we'd include multiple values for the same header key without much luck. Since Ted discovered that gmail doesn't even allow filtering on custom headers, we figured it was probably not worth the hassle of customizing this for each recipient and decided to just remove these.

mattrobenolt

Overall, this looks fine from a high level. There's a lot of stuff going on and mass rewriting of every usage, so it's hard to get a grasp if this will 100% work, but this is best we can do without just trying it.

🦄

mattrobenolt · 2018-01-05T20:32:21Z

src/sentry/api/bases/project.py

+        if not result:
+            return result
+
+        return any([


nit: the [] bit isn't needed. You can just do return any(has_team_permission(request, team, self.scope_map) for team in project.teams.all())

There's also value in that wihtout the [] it's lazily evaluated as a generator and will return when the first one is truthy.

mattrobenolt · 2018-01-05T20:34:10Z

src/sentry/api/endpoints/project_search_details.py

            raise ResourceDoesNotExist

-        if request.access.has_team_scope(project.team, 'project:write'):
+        has_team_scope = any([


nit: same here with any()

mattrobenolt · 2018-01-05T20:35:57Z

src/sentry/api/serializers/models/project.py

+            ).select_related('team')
+        )
+
+        # TODO(jess): decide if we should only include teams the user


Yeah, I think that's reasonable. Teams aren't private, at least knowing they exist. Only the contents within.

mattrobenolt · 2018-01-05T20:37:05Z

src/sentry/api/serializers/models/team.py

    def get_attrs(self, item_list, user):
-        project_qs = list(
-            Project.objects.filter(
+        project_team_qs = list(


nit: This is named as _qs implying it's a QuerSet, and it's just a list. I know this preexisted, but might as well change it since you already did.

mattrobenolt · 2018-01-05T20:55:30Z

src/sentry/models/team.py

                key=lambda x: x.name.lower()
            )
+
+            teams_by_project = {p.id: set() for p in project_list}


nit: this could be replaced by defaultdict(set)

remove outdated todo fix tests add a few more todos, remove team slug from email headers update ProjectView to inherit from org view instead of team have project serializer use new relation fix mysql tests update test fixtures to use teams kwarg stop writing team to deleted project audit remove todo i handled in a different PR remove team x header make fixtures create_project backwards compatible for other repos matt pr feedback

macqueen added the WIP label Dec 20, 2017

macqueen requested review from a team and mattrobenolt December 20, 2017 01:15

macqueen added the wcgw label Dec 20, 2017

macqueen commented Dec 20, 2017

View reviewed changes

macqueen force-pushed the use-project-team branch from 2ae9b5e to 806a1a4 Compare December 20, 2017 01:34

macqueen force-pushed the use-project-team branch 2 times, most recently from f07fa2c to 78d3c8e Compare December 21, 2017 21:23

dcramer reviewed Dec 21, 2017

View reviewed changes

src/sentry/api/bases/project.py Outdated

Copy link

Member

dcramer Dec 21, 2017

Choose a reason for hiding this comment

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

TIL what prefetch_related does

dcramer approved these changes Dec 21, 2017

View reviewed changes

macqueen force-pushed the use-project-team branch from 5967658 to 7e1172a Compare December 22, 2017 23:20

macqueen mentioned this pull request Jan 3, 2018

ref(projects): Remove team data from deleted project audit #6856

Merged

macqueen force-pushed the use-project-team branch from 7e1172a to a3e5145 Compare January 3, 2018 00:09

macqueen commented Jan 3, 2018

View reviewed changes

macqueen removed the WIP label Jan 3, 2018

macqueen commented Jan 3, 2018

View reviewed changes

macqueen mentioned this pull request Jan 4, 2018

ref(projects): Use ProjectTeam relation instead of project.team getsentry/sentry-plugins#297

Merged

macqueen force-pushed the use-project-team branch from 92552a7 to 8700242 Compare January 5, 2018 18:53

mattrobenolt approved these changes Jan 5, 2018

View reviewed changes

macqueen force-pushed the use-project-team branch from c241a7e to 7ab0201 Compare January 8, 2018 19:23

macqueen force-pushed the use-project-team branch from 7ab0201 to 1c8730a Compare January 11, 2018 00:54

macqueen merged commit 6a49f5e into master Jan 11, 2018

macqueen deleted the use-project-team branch January 11, 2018 20:21

github-actions bot locked and limited conversation to collaborators Dec 22, 2020

Uh oh!

ref(teams): Use ProjectTeam relation instead of project.team #6807

ref(teams): Use ProjectTeam relation instead of project.team #6807

Uh oh!

Conversation

macqueen commented Dec 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost commented Dec 20, 2017 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Security concerns found

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattrobenolt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

macqueen commented Dec 20, 2017 •

edited

Loading

ghost commented Dec 20, 2017 •

edited by ghost

Loading