Skip to content
This repository was archived by the owner on Jun 13, 2025. It is now read-only.

fix: Remove ordering on account organizations list #900

Merged
merged 4 commits into from
Oct 18, 2024
Merged

fix: Remove ordering on account organizations list #900

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4a0bcd9
Remove ordering on organizations list, allow users of an account to s…
spalmurray Oct 17, 2024
2469ded
Fix test
spalmurray Oct 17, 2024
6168faa
Make the shared account or member decorator better
spalmurray Oct 17, 2024
22fe17c
Merge branch 'main' into spalmurray/account-api-tweaks
spalmurray-codecov Oct 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions graphql_api/helpers/mutation.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,5 +75,22 @@ def authenticated_resolver(queried_owner, info, *args, **kwargs):
return authenticated_resolver


def require_shared_account_or_part_of_org(resolver):
def authenticated_resolver(queried_owner, info, *args, **kwargs):
current_user = info.context["request"].user
if (
current_user
and current_user.is_authenticated
and queried_owner
and queried_owner.account
and current_user in queried_owner.account.users.all()
Copy link
Member

@nora-shap nora-shap Oct 17, 2024

Choose a reason for hiding this comment

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

💯 💯 💯
LMK if this is being slow, can optimize with

  • a prefetch for the Account and Users objs when you hit the db for the Owner obj
  • doing current_user in queried_owner.account.users.all() in a different way

Copy link
Member

@nora-shap nora-shap Oct 17, 2024

Choose a reason for hiding this comment

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

queried_owner is the OwnerOrg that the current_user is checking permissions for right? Not the current_user's OwnerUser object 🫠

Copy link
Contributor Author

@spalmurray-codecov spalmurray-codecov Oct 18, 2024

Choose a reason for hiding this comment

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

@nora-codecov Yes. Specifically this decorator should allow users to see fields on owners they share an account with. For example, I'm in codecov org and not sentry org. Since we share an Account, this decorator will let me see certain things on Sentry that an unrelated user could not see.

):
return resolver(queried_owner, info, *args, **kwargs)

return require_part_of_org(resolver)(queried_owner, info, *args, **kwargs)

return authenticated_resolver


def resolve_union_error_type(error, *_):
return error.get_graphql_type()
98 changes: 3 additions & 95 deletions graphql_api/tests/test_account.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,55 +126,9 @@ def test_fetch_organizations(self) -> None:
for node in result["owner"]["account"]["organizations"]["edges"]
]

assert orgs == ["owner-2", "owner-1", "owner-0"]

def test_fetch_organizations_order_by_activated_users_asc(self) -> None:
account = AccountFactory(name="account")
owner = OwnerFactory(
username="owner-0",
plan_activated_users=[],
account=account,
)
OwnerFactory(
username="owner-1",
plan_activated_users=[0],
account=account,
)
OwnerFactory(
username="owner-2",
plan_activated_users=[0, 1],
account=account,
)

query = """
query {
owner(username: "%s") {
account {
organizations(first: 20, ordering: ACTIVATED_USERS, orderingDirection: ASC) {
edges {
node {
username
activatedUserCount
}
}
}
}
}
}
""" % (owner.username)

result = self.gql_request(query, owner=owner)

assert "errors" not in result

orgs = [
node["node"]["username"]
for node in result["owner"]["account"]["organizations"]["edges"]
]

assert orgs == ["owner-0", "owner-1", "owner-2"]

def test_fetch_organizations_order_by_name(self) -> None:
def test_fetch_organizations_desc(self) -> None:
account = AccountFactory(name="account")
owner = OwnerFactory(
username="owner-0",
Expand All @@ -196,7 +150,7 @@ def test_fetch_organizations_order_by_name(self) -> None:
query {
owner(username: "%s") {
account {
organizations(first: 20, ordering: NAME) {
organizations(first: 20, orderingDirection: DESC) {
edges {
node {
username
Expand All @@ -220,52 +174,6 @@ def test_fetch_organizations_order_by_name(self) -> None:

assert orgs == ["owner-2", "owner-1", "owner-0"]

def test_fetch_organizations_order_by_name_asc(self) -> None:
account = AccountFactory(name="account")
owner = OwnerFactory(
username="owner-0",
plan_activated_users=[],
account=account,
)
OwnerFactory(
username="owner-1",
plan_activated_users=[0],
account=account,
)
OwnerFactory(
username="owner-2",
plan_activated_users=[0, 1],
account=account,
)

query = """
query {
owner(username: "%s") {
account {
organizations(first: 20, ordering: NAME, orderingDirection: ASC) {
edges {
node {
username
activatedUserCount
}
}
}
}
}
}
""" % (owner.username)

result = self.gql_request(query, owner=owner)

assert "errors" not in result

orgs = [
node["node"]["username"]
for node in result["owner"]["account"]["organizations"]["edges"]
]

assert orgs == ["owner-0", "owner-1", "owner-2"]

def test_fetch_organizations_pagination(self) -> None:
account = AccountFactory(name="account")
owner = OwnerFactory(
Expand Down Expand Up @@ -318,7 +226,7 @@ def test_fetch_organizations_pagination(self) -> None:
for node in result["owner"]["account"]["organizations"]["edges"]
]

assert orgs == ["owner-2", "owner-1"]
assert orgs == ["owner-0", "owner-1"]

hasNextPage = result["owner"]["account"]["organizations"]["pageInfo"][
"hasNextPage"
Expand Down
25 changes: 24 additions & 1 deletion graphql_api/tests/test_owner.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,10 @@
from freezegun import freeze_time
from graphql import GraphQLError
from prometheus_client import REGISTRY
from shared.django_apps.codecov_auth.tests.factories import OktaSettingsFactory
from shared.django_apps.codecov_auth.tests.factories import (
AccountsUsersFactory,
OktaSettingsFactory,
)
from shared.django_apps.reports.models import ReportType
from shared.upload.utils import UploaderType, insert_coverage_measurement

Expand Down Expand Up @@ -1027,3 +1030,23 @@ def test_fetch_activated_user_count_returns_null_if_not_in_org(self):
""" % (owner.username)
data = self.gql_request(query, owner=user)
assert data["owner"]["activatedUserCount"] is None

def test_fetch_activated_user_count_when_not_in_org_but_has_shared_account(self):
owner = OwnerFactory(username="sample-user")
AccountsUsersFactory(user=owner.user, account=self.account)
user2 = OwnerFactory(username="sample-user-2")
user3 = OwnerFactory(username="sample-user-3")
other_owner = OwnerFactory(
username="sample-org",
plan_activated_users=[user2.ownerid, user3.ownerid],
account=self.account,
)

query = """{
owner(username: "%s") {
activatedUserCount
}
}
""" % (other_owner.username)
data = self.gql_request(query, owner=owner)
assert data["owner"]["activatedUserCount"] == 2
1 change: 0 additions & 1 deletion graphql_api/types/account/account.graphql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ type Account {
totalSeatCount: Int!
activatedUserCount: Int!
organizations(
ordering: AccountOrganizationOrdering
orderingDirection: OrderingDirection
first: Int
after: String
Expand Down
7 changes: 3 additions & 4 deletions graphql_api/types/account/account.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
build_connection_graphql,
queryset_to_connection,
)
from graphql_api.types.enums.enums import AccountOrganizationOrdering, OrderingDirection
from graphql_api.types.enums.enums import OrderingDirection

account = ariadne_load_local_graphql(__file__, "account.graphql")
account = account + build_connection_graphql("AccountOrganizationConnection", "Owner")
Expand Down Expand Up @@ -43,13 +43,12 @@ def resolve_activated_user_count(account: Account, info: GraphQLResolveInfo) ->
def resolve_organizations(
account: Account,
info: GraphQLResolveInfo,
ordering=AccountOrganizationOrdering.ACTIVATED_USERS,
ordering_direction=OrderingDirection.DESC,
ordering_direction=OrderingDirection.ASC,
**kwargs,
):
return queryset_to_connection(
account.organizations,
ordering=(ordering,),
ordering=("username",),
ordering_direction=ordering_direction,
**kwargs,
)
4 changes: 0 additions & 4 deletions graphql_api/types/enums/account_organization_ordering.graphql
View file
Open in desktop

This file was deleted.

2 changes: 0 additions & 2 deletions graphql_api/types/enums/enum_types.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@
from timeseries.models import MeasurementName

from .enums import (
AccountOrganizationOrdering,
AssetOrdering,
BundleLoadTypes,
CoverageLine,
Expand Down Expand Up @@ -57,5 +56,4 @@
EnumType("TestResultsOrderingParameter", TestResultsOrderingParameter),
EnumType("TestResultsFilterParameter", TestResultsFilterParameter),
EnumType("AssetOrdering", AssetOrdering),
EnumType("AccountOrganizationOrdering", AccountOrganizationOrdering),
]
5 changes: 0 additions & 5 deletions graphql_api/types/enums/enums.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,11 +41,6 @@ class RepositoryOrdering(enum.Enum):
NAME = "name"


class AccountOrganizationOrdering(enum.Enum):
NAME = "username"
ACTIVATED_USERS = "plan_activated_users"


class OrderingDirection(enum.Enum):
ASC = "ascending"
DESC = "descending"
Expand Down
7 changes: 5 additions & 2 deletions graphql_api/types/owner/owner.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,10 @@
build_connection_graphql,
queryset_to_connection,
)
from graphql_api.helpers.mutation import require_part_of_org
from graphql_api.helpers.mutation import (
require_part_of_org,
require_shared_account_or_part_of_org,
)
from graphql_api.types.enums import OrderingDirection, RepositoryOrdering
from graphql_api.types.errors.errors import NotFoundError, OwnerNotActivatedError
from plan.constants import FREE_PLAN_REPRESENTATIONS, PlanData, PlanName
Expand Down Expand Up @@ -377,6 +380,6 @@ def resolve_upload_token_required(owner: Owner, info) -> bool | None:

@owner_bindable.field("activatedUserCount")
@sync_to_async
@require_part_of_org
@require_shared_account_or_part_of_org
def resolve_activated_user_count(owner: Owner, info: GraphQLResolveInfo) -> int:
return owner.activated_user_count
Loading