fix: Sync plan_activated_users during sync_teams #902

suejung-sentry · 2024-11-18T21:51:54Z

sync_teams runs to take a user (e.g., suejung-sentry) and update its organizations.
When it does this, it should also remove from that organization (e.g., codecov)'s plan_activated_users, so the 2 places remain consistent.
That is, owners table row for owner suejung-sentry has owners.organizations=['codecov','other-org','other-org-2'] and owners table row for owner codecov has owners.plan_activated_users=['suejung-sentry','other-user','other-user-2'] and we need both those updated.

Special handling for "outside collaborators" - There may be "outside collaborators" that are not members of the org but may still be activated seats in codecov, so does keeping the 2 lists consistent make sense? Per here, I think we don't expressly support this feature and can employ a manual workaround in the infrequent cases a particular customer requires it.

sync_teams job is triggered from here

Here's another place it can be triggered by https://github.com/codecov/codecov-api/blob/354b64076170dd4115a4c83fbda63ba4478a23d7/services/refresh.py#L38
https://github.com/codecov/codecov-api/blob/354b64076170dd4115a4c83fbda63ba4478a23d7/graphql_api/types/mutation/sync_with_git_provider/sync_with_git_provider.py#L12 (graphql api resolve_sync_with_git_provider)
where it's called in gazebo
https://github.com/codecov/gazebo/blob/26d07c7dd517ae4a45751fa5b2d45785a264f98f/src/services/user/useResyncUser.ts#L33
Can't find your repo resync button (https://github.com/codecov/gazebo/blob/26d07c7dd517ae4a45751fa5b2d45785a264f98f/src/shared/ListRepo/OrgControlTable/RepoOrgNotFound/RepoOrgNotFound.tsx#L61)

Closes https://github.com/codecov/internal-issues/issues/1085

codecov · 2024-11-18T21:58:03Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Please upload report for BASE (main@5e92d54). Learn more about missing BASE report.
Report is 118 commits behind head on main.

❗ Current head 8774da8 differs from pull request most recent head a001d68

Please upload reports for the commit a001d68 to get more accurate results.

✅ All tests successful. No failed tests found.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #902   +/-   ##
=======================================
  Coverage        ?   98.06%           
=======================================
  Files           ?      444           
  Lines           ?    35474           
  Branches        ?        0           
=======================================
  Hits            ?    34786           
  Misses          ?      688           
  Partials        ?        0

Flag	Coverage Δ
integration	`41.93% <ø> (?)`
unit	`90.87% <ø> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

codecov-notifications · 2024-11-18T21:58:10Z

Codecov Report

Attention: Patch coverage is 75.00000% with 2 lines in your changes missing coverage. Please review.

✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
tasks/sync_teams.py	60.00%	2 Missing ⚠️

📢 Thoughts on this report? Let us know!

github-actions · 2024-11-18T21:58:28Z

✅ All tests successful. No failed tests were found.

📣 Thoughts on this report? Let Codecov know! | Powered by Codecov

codecov-qa · 2025-01-29T23:23:17Z

❌ 1 Tests Failed:

Tests completed	Failed	Passed	Skipped
1793	1	1792	9

View the top 1 failed tests by shortest run time

tasks/tests/unit/test_sync_teams_task.py::TestSyncTeamsTaskUnit::test_team_removed

Stack Traces | 0.048s run time

self = <worker.tasks.tests.unit.test_sync_teams_task.TestSyncTeamsTaskUnit object at 0x7f18e8d64180>
mocker = <pytest_mock.plugin.MockFixture object at 0x7f18d39b2f90>
mock_configuration = <shared.config.ConfigHelper object at 0x7f18d1f5bd70>
dbsession = <sqlalchemy.orm.session.Session object at 0x7f18d25dee40>
codecov_vcr = <vcr.cassette.Cassette object at 0x7f18d240b4d0>

    def test_team_removed(self, mocker, mock_configuration, dbsession, codecov_vcr):
        token = "testv2ztxs03zwys22v36ama292esl13swroe6dj"
        prev_team = OwnerFactory.create(service="github", username="Evil_Corp")
        dbsession.add(prev_team)
        user = OwnerFactory.create(
            organizations=[prev_team.ownerid],
            service="github",
            unencrypted_oauth_token=token,
        )
        prev_team.plan_activated_users = [user.ownerid]
        dbsession.add(user)
        dbsession.flush()
>       SyncTeamsTask().run_impl(dbsession, user.ownerid, using_integration=False)

.../tests/unit/test_sync_teams_task.py:43: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = <@task: app.tasks.sync_teams.SyncTeams of tasks at 0x7f18e540f8c0>
db_session = <sqlalchemy.orm.session.Session object at 0x7f18d25dee40>
ownerid = 202, username = None, kwargs = {'using_integration': False}
owner = Owner<202@service<github>>, service = 'github'
git = <github slug=None ownerid=202 repoid=None>, teams = [], updated_teams = []

    def run_impl(self, db_session, ownerid, *, username=None, **kwargs):
        log.info("Sync teams", extra=dict(ownerid=ownerid, username=username))
        owner = db_session.query(Owner).filter(Owner.ownerid == ownerid).first()
    
        assert owner, "Owner not found"
        service = owner.service
    
        git = get_owner_provider_service(owner, ignore_installation=True)
    
        # get list of teams with username, name, email, id (service_id), etc
        teams = async_to_sync(git.list_teams)()
    
        updated_teams = []
    
        for team in teams:
            team_data = dict(
                username=team["username"],
                name=team["name"],
                email=team.get("email"),
                avatar_url=team.get("avatar_url"),
                parent_service_id=team.get("parent_id"),
            )
            team_ownerid = self.upsert_team(
                db_session, service, str(team["id"]), team_data
            )
            team_data["ownerid"] = team_ownerid
            updated_teams.append(team_data)
    
        team_ids = [team["ownerid"] for team in updated_teams]
    
        removed_orgs = set(owner.organizations or []) - set(team_ids)
        if removed_orgs:
            log.warning(
                "Owner had access to organization that are being removed",
                extra=dict(
                    old_orgs=owner.organizations,
                    new_orgs=team_ids,
                    removed_orgs=sorted(removed_orgs),
                    ownerid=ownerid,
                ),
            )
            for org in removed_orgs:
>               org.plan_activated_users.remove(ownerid)
E               AttributeError: 'NoneType' object has no attribute 'plan_activated_users'

tasks/sync_teams.py:62: AttributeError

To view more test analytics, go to the Test Analytics Dashboard
📢 Thoughts on this report? Let us know!

codecov-public-qa · 2025-01-29T23:23:26Z

❌ 1 Tests Failed:

Tests completed	Failed	Passed	Skipped
1793	1	1792	9

View the top 1 failed tests by shortest run time

tasks/tests/unit/test_sync_teams_task.py::TestSyncTeamsTaskUnit::test_team_removed

Stack Traces | 0.048s run time

self = <worker.tasks.tests.unit.test_sync_teams_task.TestSyncTeamsTaskUnit object at 0x7f18e8d64180>
mocker = <pytest_mock.plugin.MockFixture object at 0x7f18d39b2f90>
mock_configuration = <shared.config.ConfigHelper object at 0x7f18d1f5bd70>
dbsession = <sqlalchemy.orm.session.Session object at 0x7f18d25dee40>
codecov_vcr = <vcr.cassette.Cassette object at 0x7f18d240b4d0>

    def test_team_removed(self, mocker, mock_configuration, dbsession, codecov_vcr):
        token = "testv2ztxs03zwys22v36ama292esl13swroe6dj"
        prev_team = OwnerFactory.create(service="github", username="Evil_Corp")
        dbsession.add(prev_team)
        user = OwnerFactory.create(
            organizations=[prev_team.ownerid],
            service="github",
            unencrypted_oauth_token=token,
        )
        prev_team.plan_activated_users = [user.ownerid]
        dbsession.add(user)
        dbsession.flush()
>       SyncTeamsTask().run_impl(dbsession, user.ownerid, using_integration=False)

.../tests/unit/test_sync_teams_task.py:43: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = <@task: app.tasks.sync_teams.SyncTeams of tasks at 0x7f18e540f8c0>
db_session = <sqlalchemy.orm.session.Session object at 0x7f18d25dee40>
ownerid = 202, username = None, kwargs = {'using_integration': False}
owner = Owner<202@service<github>>, service = 'github'
git = <github slug=None ownerid=202 repoid=None>, teams = [], updated_teams = []

    def run_impl(self, db_session, ownerid, *, username=None, **kwargs):
        log.info("Sync teams", extra=dict(ownerid=ownerid, username=username))
        owner = db_session.query(Owner).filter(Owner.ownerid == ownerid).first()
    
        assert owner, "Owner not found"
        service = owner.service
    
        git = get_owner_provider_service(owner, ignore_installation=True)
    
        # get list of teams with username, name, email, id (service_id), etc
        teams = async_to_sync(git.list_teams)()
    
        updated_teams = []
    
        for team in teams:
            team_data = dict(
                username=team["username"],
                name=team["name"],
                email=team.get("email"),
                avatar_url=team.get("avatar_url"),
                parent_service_id=team.get("parent_id"),
            )
            team_ownerid = self.upsert_team(
                db_session, service, str(team["id"]), team_data
            )
            team_data["ownerid"] = team_ownerid
            updated_teams.append(team_data)
    
        team_ids = [team["ownerid"] for team in updated_teams]
    
        removed_orgs = set(owner.organizations or []) - set(team_ids)
        if removed_orgs:
            log.warning(
                "Owner had access to organization that are being removed",
                extra=dict(
                    old_orgs=owner.organizations,
                    new_orgs=team_ids,
                    removed_orgs=sorted(removed_orgs),
                    ownerid=ownerid,
                ),
            )
            for org in removed_orgs:
>               org.plan_activated_users.remove(ownerid)
E               AttributeError: 'NoneType' object has no attribute 'plan_activated_users'

tasks/sync_teams.py:62: AttributeError

To view more test analytics, go to the Test Analytics Dashboard
📢 Thoughts on this report? Let us know!

suejung-sentry · 2025-01-30T00:14:48Z

Actually it looks like this logic is handled upstream by the place this job is called
https://github.com/codecov/codecov-api/blob/ae83c2fa69b4d572b0096c54803d8068a8f13ebe/webhook_handlers/views/github.py#L611C13-L616C21

EDIT - see pr description - this is only 1 of many places this can be called

suejung-sentry · 2025-03-24T22:28:20Z

closing because this logic already takes place here - https://github.com/codecov/codecov-api/blob/ae83c2fa69b4d572b0096c54803d8068a8f13ebe/webhook_handlers/views/github.py#L611C13-L616C21

fix: Remove from plan activated users when removing from org

3b28bbb

suejung-sentry force-pushed the sshin/fix/sync-teams branch from 8774da8 to 3b28bbb Compare January 29, 2025 23:16

fix test

f1eddb4

suejung-sentry force-pushed the sshin/fix/sync-teams branch from 7e09bf5 to f1eddb4 Compare January 29, 2025 23:54

suejung-sentry marked this pull request as ready for review January 30, 2025 00:02

cleanup

a001d68

suejung-sentry force-pushed the sshin/fix/sync-teams branch from cfbad10 to a001d68 Compare January 30, 2025 00:07

suejung-sentry closed this Mar 24, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix: Sync plan_activated_users during sync_teams #902

fix: Sync plan_activated_users during sync_teams #902

Uh oh!

suejung-sentry commented Nov 18, 2024 •

edited

Loading

Uh oh!

codecov bot commented Nov 18, 2024 •

edited

Loading

Uh oh!

codecov-notifications bot commented Nov 18, 2024 •

edited

Loading

Uh oh!

github-actions bot commented Nov 18, 2024

Uh oh!

codecov-qa bot commented Jan 29, 2025 •

edited

Loading

Uh oh!

codecov-public-qa bot commented Jan 29, 2025 •

edited

Loading

Uh oh!

suejung-sentry commented Jan 30, 2025 •

edited

Loading

Uh oh!

suejung-sentry commented Mar 24, 2025

Uh oh!

Uh oh!

fix: Sync plan_activated_users during sync_teams #902

fix: Sync plan_activated_users during sync_teams #902

Uh oh!

Conversation

suejung-sentry commented Nov 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Nov 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

codecov-notifications bot commented Nov 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

github-actions bot commented Nov 18, 2024

Uh oh!

codecov-qa bot commented Jan 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

❌ 1 Tests Failed:

Uh oh!

codecov-public-qa bot commented Jan 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

❌ 1 Tests Failed:

Uh oh!

suejung-sentry commented Jan 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

suejung-sentry commented Mar 24, 2025

Uh oh!

Uh oh!

suejung-sentry commented Nov 18, 2024 •

edited

Loading

codecov bot commented Nov 18, 2024 •

edited

Loading

codecov-notifications bot commented Nov 18, 2024 •

edited

Loading

codecov-qa bot commented Jan 29, 2025 •

edited

Loading

codecov-public-qa bot commented Jan 29, 2025 •

edited

Loading

suejung-sentry commented Jan 30, 2025 •

edited

Loading