Conversation
✅ Multiple Profiles - Members Suggestion Dropdown
Description
As a user, on the BaseApp Profile,I would like to I want to add new
members to my organization, In order to have thse members help in the
organizations activites.
Original Story:
Acceptance Criteria
Context
This story is about creating the Add members button and the search
functionality to find users in the platform to invite them as members.
The suggestions dropdown should be generic so that in other projects the
queries could be changed. So that we can suggest other stuff that is not
users.
We are going to use the user query just for the sake of using an
example.
Sidebar with AA
Business Rules - Members Suggestion Dropdown
On this story we will work on the search functionality to add members,
but not the actual logic of the invitation.
Given a user wants to add a new member to the organization, when they
click the Add Member button, a modal should appear where they can input
the name or the user name.
The input box should suggest a list of users, based on what the user has
written in it.
The filtering criteria for this search will be name of the user.
We will show only 4 options max and then use an infinite scrolling
behavior to expand the initial results.
Given the I have found the corresponding user, when I click on the user
option, it will be added to the list of users to invite after
confirmation.
Given the user has clicked on Invite Button, the invited users should
now appear in the Members list page with the Label invited.
Demo: https://www.loom.com/share/d991c36180c847d2b34dd82cf116a22d
---------
Co-authored-by: Lúcio BJ <lb@tsl.io>
Co-authored-by: Alisson Patricio <eu@alisson.net>
Co-authored-by: Patrick Sluth <ps@tsl.io> Co-authored-by: Vitor <vitor@M4cb00k.local> Co-authored-by: Vitor Guimarães <80125714+vitorguima@users.noreply.github.com> Co-authored-by: Alisson Patricio <eu@alisson.net>
gabrielalmeida-tsl
requested review from
Hercilio1,
nossila and
vitorguima
as code owners
WalkthroughThis PR extends the user search filtering to support email and profile name lookups, and introduces three new GraphQL mutations for managing profile user roles (create, update, delete). It also updates the ProfileUserRole model to support email-based invitations and new status handling, with corresponding migrations and test coverage. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
baseapp_profiles/models.py (1)
235-240:⚠️ Potential issue | 🟠 Major
unique_togetherconstraint may not work as expected with nullableuser.The
unique_together = [("user", "profile")]constraint combined withuserbeing nullable could allow multipleProfileUserRolerecords withuser=Noneand the sameprofile, sinceNULLvalues are typically not considered equal in database unique constraints. This may be problematic for the email invitation flow when it's fully implemented.Additionally, the
__str__method on line 240 will display confusing output ifuserisNone(e.g., "None as Manager in ProfileName" in admin/logs).Since the email invitation feature is currently marked as TODO (line 174 in mutations.py), consider implementing email-based uniqueness validation (either at the database level with a partial unique constraint or via application-level validation) when completing the
emails_to_invitefunctionality. Also consider adding a fallback in__str__to display the email whenuserisNone.baseapp_auth/admin.py (1)
75-98:⚠️ Potential issue | 🟠 MajorGuard
add_is_password_expired()when expiration is disabled.
With interval<= 0,add_is_password_expired()still annotatesis_password_expiredasTruefor most users, andobj.password_expiredwill then return the annotation, making the admin show expired even when expiration is off.✅ Suggested fix
def get_queryset(self, request): qs = super().get_queryset(request) - return qs.add_is_password_expired() + if config.USER_PASSWORD_EXPIRATION_INTERVAL <= 0: + return qs + return qs.add_is_password_expired()
🤖 Fix all issues with AI agents
In `@baseapp_api_key/graphql/__init__.py`:
- Line 1: The file imports APIKeyAuthentication from .middleware and currently
uses a blanket noqa which RUF100 flags; remove the unused blanket `# noqa` and
either delete it entirely if no lint suppression is required or replace it with
a targeted code comment (e.g., a specific ruff/flake rule like `# noqa: F401`
only if the import must be kept for exports) so only the unnecessary blanket
suppression is removed while keeping the import of APIKeyAuthentication intact.
In `@baseapp_api_key/graphql/consumers.py`:
- Around line 39-54: The code unconditionally accesses api_key.user after
fetching api_key, which can be None or expired; update the logic in the block
handling unencrypted_api_key (where you call self.APIKeyModel.objects.encrypt
and await database_sync_to_async(...).first) to first check that api_key is not
None and that not api_key.is_expired before assigning user = api_key.user; if
either check fails set user = None (or return/continue) so you never dereference
a None api_key; ensure the checks occur immediately after fetching api_key and
remove the unconditional final assignment to api_key.user.
- Around line 61-63: The permission check in the conditional using
database_sync_to_async(user.has_perm)(...) is missing an await, so the coroutine
is not executed and the condition always truthy; update the conditional in the
block that references profile and database_sync_to_async(user.has_perm) to await
the coroutine (i.e., await
database_sync_to_async(user.has_perm)(f"{profile._meta.app_label}.use_profile",
profile)) so the permission is actually evaluated before allowing setting
current_profile.
In `@baseapp_api_key/graphql/middleware.py`:
- Around line 6-17: The class-level authenticated flag on APIKeyAuthentication
causes shared state across requests; change the logic in
APIKeyAuthentication.resolve to avoid a class attribute by using a
per-request/local marker (e.g., check/set a flag on info.context such as
info.context._api_key_authenticated or simply run authentication each call)
instead of the class attribute authenticated; ensure you still call
self.authenticate(info.context), assign info.context.user when
user.is_authenticated, and remove or stop modifying the class-level
authenticated property so middleware instances remain stateless across requests.
In `@baseapp_api_key/management/commands/api_key.py`:
- Around line 54-67: The code only processes the first tuple from
options.get("rotate_encryption_key") because it uses _args[0]; update the rotate
handling in the management command so all provided pairs are handled: either
iterate over the list returned by options.get("rotate_encryption_key") and call
APIKeyModel.objects.rotate_encryption_key(...) for each (keeping the same
masking and stdout output using _mask), or change the argparse setup to use
nargs=2 (remove action="append") if only a single rotation pair should be
supported; adjust references to _args and the processing block accordingly to
reflect the chosen approach.
In `@baseapp_api_key/README.md`:
- Around line 124-136: Update the README examples to replace the literal
token-like placeholder "BA-your-64-character-api-key-here" with a non-secret
environment-variable placeholder (e.g., use an env var name like $API_KEY or
{{API_KEY}}) in both the curl example and the Python requests headers; change
the curl command to read the header value from an env variable and change the
Python snippet to show reading the value from an environment variable (e.g., via
os.environ) so the examples in README.md no longer contain token-like secrets
and avoid secret-scanner false positives.
In `@baseapp_api_key/rest_framework/permissions.py`:
- Around line 15-29: The has_permission method on HasAPIKey is incorrectly
marked with `@abc.abstractmethod` despite providing a concrete implementation,
which prevents instantiation; remove the `@abc.abstractmethod` decorator from
has_permission in the HasAPIKey class so DRF can instantiate the permission
class, keeping the existing logic that reads settings.BA_API_KEY_REQUEST_HEADER
from request.META, uses APIKeyModel.objects.encrypt(unencrypted_value=...) and
then queries APIKeyModel.objects.filter(is_expired=False,
encrypted_api_key=...). Ensure no other abstract decorators remain on
has_permission.
In `@baseapp_auth/graphql/filters.py`:
- Around line 21-23: The method filter_by_q has an unused parameter name; rename
that parameter to _name to follow django-filters' required signature (self,
_name, value) and Python convention for unused variables. Update the method
definition for filter_by_q accordingly so linters stop flagging the unused
parameter while preserving the expected django-filters interface (no other logic
changes needed in filter_by_q).
In `@baseapp_core/tests/settings.py`:
- Line 217: Add the missing app entry to INSTALLED_APPS so the APIKey model and
authentication backend are available: in the settings where
DEFAULT_AUTHENTICATION_CLASSES includes
"baseapp_api_key.rest_framework.authentication.APIKeyAuthentication", add
"baseapp_api_key" to INSTALLED_APPS (ensure the string exactly matches the app
name used elsewhere) so imports and model lookups for APIKey and the
authentication class succeed during tests.
In `@baseapp_profiles/graphql/mutations.py`:
- Around line 178-183: The list comprehension building profile_user_roles can
raise a TypeError when users_ids is None; update the code that creates
profile_user_roles (the comprehension using ProfileUserRole and
get_pk_from_relay_id) to handle a missing users_ids by treating it as an empty
iterable (e.g., defaulting users_ids to [] or guarding with a conditional) so
the loop is safe when input.get("users_ids") returns None.
- Around line 244-247: ProfileUserRole.objects.get(...) will raise
ProfileUserRole.DoesNotExist before the current if check, so catch that
exception and convert it into the intended GraphQLError; specifically wrap the
call to ProfileUserRole.objects.get(user_id=user_pk, profile_id=profile_pk) in a
try/except catching ProfileUserRole.DoesNotExist and in the except raise
GraphQLError(str(_("User role not found"))), leaving successful retrieval to
assign obj as before.
In `@baseapp_profiles/tests/test_graphql_mutations_update.py`:
- Around line 340-361: The test function
test_user_with_permission_can_add_profile_user_role uses the wrong profile in
the GraphQL input: replace the incorrect user.profile.relay_id with the
profile.relay_id created by ProfileFactory (reference symbols:
test_user_with_permission_can_add_profile_user_role, ProfileFactory,
user.profile.relay_id, profile.relay_id) and ensure the subsequent
profile.refresh_from_db() refers to that same profile; alternatively, if the
intent was to test adding a role to the user's own profile, adjust the
ProfileFactory setup or the test name to reflect that intent and/or grant
explicit permission on profile before calling the mutation.
In `@testproject/settings.py`:
- Around line 171-173: Ruff flags env as undefined because it was brought in via
a star import; explicitly import the env symbol instead of relying on wildcard
imports so BA_API_KEY_REQUEST_HEADER and BA_API_KEY_ENCRYPTION_KEY can use it.
Add a direct import for env (the same provider used elsewhere in the project) at
the top of settings.py (so env is defined before BA_API_KEY_REQUEST_HEADER and
BA_API_KEY_ENCRYPTION_KEY are assigned).
🧹 Nitpick comments (19)
baseapp_profiles/graphql/mutations.py (2)
157-160: Use exception chaining for clearer tracebacks.Per static analysis (B904), when re-raising exceptions within an
exceptclause, useraise ... from errorraise ... from Noneto distinguish them from errors in exception handling.♻️ Proposed fix for exception chaining
try: profile = Profile.objects.get(pk=profile_pk) except Profile.DoesNotExist: - raise GraphQLError(str(_("Profile not found"))) + raise GraphQLError(str(_("Profile not found"))) from Nonetry: obj = ProfileUserRole.objects.get(user_id=user_pk, profile_id=profile_pk) except ProfileUserRole.DoesNotExist: - raise GraphQLError(str(_("Role not found"))) + raise GraphQLError(str(_("Role not found"))) from NoneAlso applies to: 211-214
169-172: Redundant condition check forrole_type.The
elif role_type and role_type not in ...condition is redundant because theelifalready impliesrole_typeis truthy (since theif not role_type:branch wasn't taken).♻️ Simplify the condition
if not role_type: role_type = ProfileUserRole.ProfileRoles.MANAGER - elif role_type and role_type not in ProfileUserRole.ProfileRoles.values: + elif role_type not in ProfileUserRole.ProfileRoles.values: raise GraphQLError(str(_("Invalid role type")))baseapp_profiles/tests/test_graphql_mutations_delete.py (1)
105-117: Unnecessary permission grant for owner test.The test is named
test_user_profile_owner_can_remove_profile_member, implying it tests that the profile owner can delete members. However, lines 106-113 grant thedelete_profileuserrolepermission to the user. Since the owner should already have this permission by virtue of being the owner (as perProfilesPermissionsBackend), the explicit permission grant is unnecessary and may obscure what the test is actually verifying.♻️ Proposed fix to remove unnecessary permission grant
def test_user_profile_owner_can_remove_profile_member(django_user_client, graphql_user_client): - perm = Permission.objects.get( - content_type__app_label=ProfileUserRole._meta.app_label, codename="delete_profileuserrole" - ) - user = django_user_client.user user_2 = UserFactory() - user.user_permissions.add(perm) profile = ProfileFactory(owner=user)baseapp_auth/tasks.py (1)
12-15: Usetimezone.now().date()to align with Django's__datelookup expectations.Django's
__datelookup is documented to accept a date value. While passingtimezone.now()(a datetime) will work because Django implicitly converts it viaDateField.to_python(), this conversion happens after timezone normalization, which can mask subtle timezone edge cases. Passing the date explicitly—as shown below—is the documented recommended approach and keeps the intent clear.Suggested change
- .filter(password_expiry_date__date=timezone.now()) + .filter(password_expiry_date__date=timezone.now().date())baseapp_api_key/tests/mixins.py (1)
1-1: Consider removing the unusednoqadirective forF401.Static analysis indicates that
F401is not enabled in your linter configuration, making thenoqa: F401directive unnecessary. You can simplify this to just# noqa: F403.🧹 Suggested cleanup
-from baseapp_core.tests.mixins import * # noqa: F403, F401 +from baseapp_core.tests.mixins import * # noqa: F403setup.cfg (1)
22-22: Exact version pins may limit flexibility.
django-pgtrigger == 4.14anddjango-pghistory == 3.5.5are pinned to exact versions. This ensures stability but prevents patch updates. If there are specific compatibility reasons, this is fine; otherwise, consider using compatible release specifiers (e.g.,~= 4.14or>= 4.14, < 5.0) to allow patch updates.Also applies to: 36-36
baseapp_auth/querysets.py (1)
1-35: Verifytyping.Selfcompatibility with supported Python versions.Without
from __future__ import annotations,typing.Selfonly exists at runtime on Python 3.11+. If older versions are supported, this will raise on import. Consider a forward reference ortyping_extensions.Self, or confirm the minimum Python version.🔧 Safer runtime-compatible annotation
-import typing @@ - def add_is_expired(self) -> typing.Self: + def add_is_expired(self) -> "BaseAPIKeyQuerySet":baseapp_api_key/tests/conftest.py (1)
5-7: Prefersys.path.insert(0, ...)to ensure local modules win.
This avoids accidentally importing an installed package version ahead of the repo copy.♻️ Suggested change
-# add the test root dir to python path -root_dir = d(d(abspath(__file__))) -sys.path.append(root_dir) +# add the test root dir to python path +root_dir = d(d(abspath(__file__))) +sys.path.insert(0, root_dir)baseapp_api_key/tests/helpers.py (2)
5-5: Remove unusednoqadirective.Per static analysis, the
F401suppression is not needed here since the wildcard import re-exports symbols (covered byF403).🧹 Proposed fix
-from baseapp_core.tests.helpers import * # noqa: F403, F401 +from baseapp_core.tests.helpers import * # noqa: F403
8-14: Consider adding error handling for missing factory configuration.If the configured
BASEAPP_AUTH_USER_FACTORYpath is invalid, this will raise a crypticModuleNotFoundErrororAttributeError. A clearer error message would improve debuggability.💡 Proposed improvement
def get_user_factory(): factory_class_path = getattr( settings, "BASEAPP_AUTH_USER_FACTORY", "apps.users.tests.factories.UserFactory" ) module_path = ".".join(factory_class_path.split(".")[:-1]) - module = importlib.import_module(module_path) - return getattr(module, factory_class_path.split(".")[-1]) + class_name = factory_class_path.split(".")[-1] + try: + module = importlib.import_module(module_path) + return getattr(module, class_name) + except (ImportError, AttributeError) as e: + raise ImportError( + f"Could not load user factory '{factory_class_path}'. " + f"Check BASEAPP_AUTH_USER_FACTORY setting. Original error: {e}" + ) from ebaseapp_api_key/rest_framework/permissions.py (2)
6-7: Minor: Incorrect type hints.
HttpRequestshould beRequest(DRF's request class) since this is DRF contextview: typing.Type[APIView]should beview: APIView(instance, not class)📝 Proposed fix
-from rest_framework.request import HttpRequest +from rest_framework.request import Request from rest_framework.views import APIView from baseapp_api_key.models import APIKey, BaseAPIKey class BaseHasAPIKey(permissions.BasePermission): APIKeyModel: typing.Type[BaseAPIKey] - def has_permission(self, request: HttpRequest, view: typing.Type[APIView]): + def has_permission(self, request: Request, view: APIView) -> bool:
1-1: Unused import:abcmodule.After removing
@abc.abstractmethod, theabcimport becomes unused.🧹 Proposed fix
-import abc import typingbaseapp_auth/tests/graphql/test_queries_user.py (2)
173-181: Unusedqueriesvariable.The
queriesvariable is unpacked but never used. Since this test doesn't verify query count (unliketest_anon_can_query_users_list_with_optimized_query), consider using_to indicate it's intentionally ignored.Proposed fix
- response, queries = graphql_client_with_queries( + response, _ = graphql_client_with_queries( QUERY_USERS_LIST_WITH_FILTERS, variables={"q": "test@test.com"} )
184-197: Same unusedqueriesvariable pattern.Same issue as above - the
queriesvariable is unpacked but unused.Proposed fix
- response, queries = graphql_client_with_queries( + response, _ = graphql_client_with_queries( QUERY_USERS_LIST_WITH_FILTERS, variables={"q": "test"} )baseapp_api_key/tests/factories.py (1)
3-6: Import ordering: moveuuid4import to top.The
from uuid import uuid4import on line 6 appears after code execution on line 5. While this works, it violates PEP 8 import ordering conventions.Proposed fix
import factory +from uuid import uuid4 import baseapp_auth.tests.helpers as h UserFactory = h.get_user_factory() -from uuid import uuid4baseapp_api_key/models.py (1)
14-14:default=Nonewithnull=Falsemay cause validation issues.Setting
default=Noneon aBinaryFieldwithnull=Falsecreates a potentially confusing situation. While this pattern can enforce that the field must be explicitly set (as done inadmin.pyand factories), attempting to save a model instance without settingencrypted_api_keywill raise anIntegrityError.Consider using a sentinel value or removing the default to make the requirement explicit:
Option 1: Remove default entirely (recommended)
- encrypted_api_key = models.BinaryField(null=False, blank=False, default=None) + encrypted_api_key = models.BinaryField(null=False, blank=False)Option 2: Use empty bytes as default if a default is truly needed
- encrypted_api_key = models.BinaryField(null=False, blank=False, default=None) + encrypted_api_key = models.BinaryField(null=False, blank=False, default=b"")baseapp_api_key/admin.py (1)
19-19: Consider removingencrypted_api_keyfromlist_display.Displaying
encrypted_api_key(binary/hex data) in the admin list view provides no useful information to administrators and clutters the interface. Consider showing a truncated version or a simple "Set" / "Not Set" indicator instead.Proposed fix - add a display method
class BaseAPIKeyAdmin(ModelAdmin): - list_display = ("id", "user", "name", "encrypted_api_key", "is_expired") + list_display = ("id", "user", "name", "has_api_key", "is_expired") def is_expired(self, obj): return obj.is_expired + def has_api_key(self, obj): + return bool(obj.encrypted_api_key) + has_api_key.boolean = True + has_api_key.short_description = "API Key Set"baseapp_api_key/management/commands/api_key.py (1)
44-48: UseExceptioninstead ofBaseExceptionand bareraise.
- Catching
BaseExceptionis overly broad and includesSystemExit,GeneratorExit, etc. UseExceptionfor general error handling.- Use bare
raiseto preserve the original traceback.Proposed fix
- except BaseException as e: + except Exception as e: self.stdout.write("\r\n") if isinstance(e, KeyboardInterrupt): return - raise e + raiseNote: With
Exception, you won't catchKeyboardInterrupt(which inherits fromBaseException), so you may want to handle it separately:Alternative with explicit KeyboardInterrupt handling
+ except KeyboardInterrupt: + self.stdout.write("\r\n") + return - except BaseException as e: + except Exception: self.stdout.write("\r\n") - if isinstance(e, KeyboardInterrupt): - return - raise e + raisebaseapp_api_key/managers.py (1)
11-11: Import the API-key queryset from the API-key app to avoid cross-app coupling.
This looks like a copy/paste frombaseapp_auth; prefer using the queryset inbaseapp_api_key.querysets.♻️ Suggested fix
-from baseapp_auth.querysets import BaseAPIKeyQuerySet +from baseapp_api_key.querysets import BaseAPIKeyQuerySet
| profile_user_roles = [ | ||
| ProfileUserRole( | ||
| user_id=get_pk_from_relay_id(user_id), profile_id=profile_pk, role=role_type | ||
| ) | ||
| for user_id in users_ids | ||
| ] |
There was a problem hiding this comment.
Potential TypeError when users_ids is None.
If users_ids is not provided in the input, input.get("users_ids") returns None, and iterating over None in the list comprehension will raise a TypeError.
🐛 Proposed fix to handle missing users_ids
profile_user_roles = [
ProfileUserRole(
user_id=get_pk_from_relay_id(user_id), profile_id=profile_pk, role=role_type
)
- for user_id in users_ids
+ for user_id in (users_ids or [])
]🤖 Prompt for AI Agents
In `@baseapp_profiles/graphql/mutations.py` around lines 178 - 183, The list
comprehension building profile_user_roles can raise a TypeError when users_ids
is None; update the code that creates profile_user_roles (the comprehension
using ProfileUserRole and get_pk_from_relay_id) to handle a missing users_ids by
treating it as an empty iterable (e.g., defaulting users_ids to [] or guarding
with a conditional) so the loop is safe when input.get("users_ids") returns
None.
Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: Patrick Sluth <ps@tsl.io> Co-authored-by: Vitor <vitor@M4cb00k.local> Co-authored-by: Vitor Guimarães <80125714+vitorguima@users.noreply.github.com> Co-authored-by: Alisson Patricio <eu@alisson.net> Co-authored-by: Mathieu Bouhelier <62220320+mathieubouhelier@users.noreply.github.com> Co-authored-by: Hercilio Martins Ortiz <herciliomartins@gmail.com> Co-authored-by: Hercilio Ortiz <ho@tsl.io> Co-authored-by: Ronan <62265281+Ronan-Fernandes@users.noreply.github.com> Co-authored-by: Rodean Fraser <rf2@tsl.io> Co-authored-by: davidbermudez-tsl <89879138+davidbermudez-tsl@users.noreply.github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Filipe de Castro <fc@tsl.io> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-authored-by: Lucas Caribé <lucasmoc47@gmail.com> Co-authored-by: Lúcio BJ <lbjunq@hotmail.com> Co-authored-by: Lúcio BJ <lb@tsl.io> Co-authored-by: Gabriel Leandro <gs@tsl.io> Co-authored-by: Gabriel Almeida <ga@tsl.io>
|
Hold this PR until #368 gets merged |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
baseapp_profiles/tests/test_graphql_mutations_update.py (1)
321-369: 🛠️ Refactor suggestion | 🟠 MajorDelete tests are duplicated — these also exist in
test_graphql_mutations_delete.py.
test_user_profile_owner_can_remove_profile_memberandtest_user_with_permission_can_remove_profile_memberare identical to their counterparts in the delete test file. Remove them from here to avoid maintenance burden and test confusion.
🤖 Fix all issues with AI agents
In `@baseapp_auth/tests/graphql/test_queries_user.py`:
- Around line 200-213: The test function
test_anon_can_query_users_list_with_filter_by_profile_name should reflect that
the filtering uses Django's __icontains substring lookup (so "test" matching
"testing" is correct and not full-text based); update the test
comment/assumption to remove any full-text/stemming concern and ensure the call
to graphql_client_with_queries(QUERY_USERS_LIST_WITH_FILTERS, variables={"q":
"test"}) remains as-is, and rename the unused variable queries returned from
graphql_client_with_queries to _queries to silence the linter while leaving
response handling and the final assertion unchanged.
In `@baseapp_profiles/tests/test_graphql_mutations_delete.py`:
- Around line 105-155: Two test functions are duplicated; remove the duplicate
definitions of test_user_profile_owner_can_remove_profile_member and
test_user_with_permission_can_remove_profile_member from the file that tests
updates (keep the versions in the delete tests file), ensuring only the deletion
tests remain; search for the function names to locate and delete the duplicate
blocks so the deletion-focused tests exist in one place.
In `@baseapp_profiles/tests/test_graphql_mutations_update.py`:
- Around line 372-396: Rename the misleading test function
test_user_without_permission_cant_remove_profile_member to reflect that it
asserts blocking creation of a profile role; update the function name to
something like test_user_without_permission_cant_create_profile_user_role (or
test_user_without_permission_cant_add_profile_member) and keep the existing test
body that uses PROFILE_USER_ROLE_CREATE_GRAPHQL and the create-style input
(usersIds, roleType) so the name matches the behavior being tested.
🧹 Nitpick comments (2)
baseapp_profiles/tests/test_graphql_mutations_delete.py (1)
27-33: Duplicate mutation constant:PROFILE_USER_ROLE_DELETE_GRAPHQLis defined in both this file andtest_graphql_mutations_update.py.This constant is identical in both files. Consider defining it once (e.g., in a shared test constants module or in this file) and importing it where needed.
baseapp_auth/tests/graphql/test_queries_user.py (1)
189-197: Unusedqueriesvariable — use_prefix or a plaingraphql_client.The
queriesbinding fromgraphql_client_with_queriesis never used. Either rename it to_queriesor use a simpler client fixture if you don't need query-count assertions.♻️ Suggested fix
def test_anon_can_query_users_list_with_filter_by_email(graphql_client_with_queries): UserFactory.create_batch(2) UserFactory(email="test@test.com") - response, queries = graphql_client_with_queries( + response, _queries = graphql_client_with_queries( QUERY_USERS_LIST_WITH_FILTERS, variables={"q": "test@test.com"} )
| def test_anon_can_query_users_list_with_filter_by_profile_name(graphql_client_with_queries): | ||
| UserFactory.create_batch(2) | ||
| user_1 = UserFactory() | ||
| user_1.profile.name = "test" | ||
| user_1.profile.save() | ||
| user_2 = UserFactory() | ||
| user_2.profile.name = "testing" | ||
| user_2.profile.save() | ||
| response, queries = graphql_client_with_queries( | ||
| QUERY_USERS_LIST_WITH_FILTERS, variables={"q": "test"} | ||
| ) | ||
| content = response.json() | ||
|
|
||
| assert len(content["data"]["users"]["edges"]) == 2 |
There was a problem hiding this comment.
Remove the full-text search concern; the filter uses __icontains for substring matching, not PostgreSQL full-text search. Rename unused variable queries to _queries.
The search for "test" matching "testing" is not fragile—the filter uses Django's __icontains lookup (case-insensitive substring matching), so this behavior is deterministic. The substring "test" is guaranteed to match "testing" regardless of configuration. The PostgreSQL full-text search stemming concern mentioned in the original comment does not apply here.
The queries variable is genuinely unused and should be renamed to _queries to suppress the linter warning.
♻️ Suggested fix for unused variable
- response, queries = graphql_client_with_queries(
+ response, _queries = graphql_client_with_queries(
QUERY_USERS_LIST_WITH_FILTERS, variables={"q": "test"}
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_anon_can_query_users_list_with_filter_by_profile_name(graphql_client_with_queries): | |
| UserFactory.create_batch(2) | |
| user_1 = UserFactory() | |
| user_1.profile.name = "test" | |
| user_1.profile.save() | |
| user_2 = UserFactory() | |
| user_2.profile.name = "testing" | |
| user_2.profile.save() | |
| response, queries = graphql_client_with_queries( | |
| QUERY_USERS_LIST_WITH_FILTERS, variables={"q": "test"} | |
| ) | |
| content = response.json() | |
| assert len(content["data"]["users"]["edges"]) == 2 | |
| def test_anon_can_query_users_list_with_filter_by_profile_name(graphql_client_with_queries): | |
| UserFactory.create_batch(2) | |
| user_1 = UserFactory() | |
| user_1.profile.name = "test" | |
| user_1.profile.save() | |
| user_2 = UserFactory() | |
| user_2.profile.name = "testing" | |
| user_2.profile.save() | |
| response, _queries = graphql_client_with_queries( | |
| QUERY_USERS_LIST_WITH_FILTERS, variables={"q": "test"} | |
| ) | |
| content = response.json() | |
| assert len(content["data"]["users"]["edges"]) == 2 |
🧰 Tools
🪛 Ruff (0.14.14)
[warning] 208-208: Unpacked variable queries is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
🤖 Prompt for AI Agents
In `@baseapp_auth/tests/graphql/test_queries_user.py` around lines 200 - 213, The
test function test_anon_can_query_users_list_with_filter_by_profile_name should
reflect that the filtering uses Django's __icontains substring lookup (so "test"
matching "testing" is correct and not full-text based); update the test
comment/assumption to remove any full-text/stemming concern and ensure the call
to graphql_client_with_queries(QUERY_USERS_LIST_WITH_FILTERS, variables={"q":
"test"}) remains as-is, and rename the unused variable queries returned from
graphql_client_with_queries to _queries to silence the linter while leaving
response handling and the final assertion unchanged.
| def test_user_profile_owner_can_remove_profile_member(django_user_client, graphql_user_client): | ||
| perm = Permission.objects.get( | ||
| content_type__app_label=ProfileUserRole._meta.app_label, codename="delete_profileuserrole" | ||
| ) | ||
|
|
||
| user = django_user_client.user | ||
| user_2 = UserFactory() | ||
|
|
||
| user.user_permissions.add(perm) | ||
| profile = ProfileFactory(owner=user) | ||
| profile_user_role = ProfileUserRoleFactory( | ||
| profile=profile, user=user_2, role=ProfileUserRole.ProfileRoles.MANAGER | ||
| ) | ||
|
|
||
| expected_relay_id = profile_user_role.relay_id | ||
|
|
||
| response = graphql_user_client( | ||
| PROFILE_USER_ROLE_DELETE_GRAPHQL, | ||
| variables={"input": {"userId": user_2.relay_id, "profileId": profile.relay_id}}, | ||
| ) | ||
| content = response.json() | ||
|
|
||
| assert content["data"]["profileUserRoleDelete"]["deletedId"] == expected_relay_id | ||
| assert not ProfileUserRole.objects.filter(id=profile_user_role.id).exists() | ||
|
|
||
|
|
||
| def test_user_with_permission_can_remove_profile_member(django_user_client, graphql_user_client): | ||
| perm = Permission.objects.get( | ||
| content_type__app_label=ProfileUserRole._meta.app_label, codename="delete_profileuserrole" | ||
| ) | ||
|
|
||
| user = django_user_client.user | ||
| user.user_permissions.add(perm) | ||
| user_2 = UserFactory() | ||
| user_3 = UserFactory() | ||
|
|
||
| profile = ProfileFactory(owner=user_2) | ||
| ProfileUserRoleFactory(profile=profile, user=user, role=ProfileUserRole.ProfileRoles.ADMIN) | ||
| profile_user_role = ProfileUserRoleFactory( | ||
| profile=profile, user=user_3, role=ProfileUserRole.ProfileRoles.MANAGER | ||
| ) | ||
|
|
||
| expected_relay_id = profile_user_role.relay_id | ||
|
|
||
| response = graphql_user_client( | ||
| PROFILE_USER_ROLE_DELETE_GRAPHQL, | ||
| variables={"input": {"userId": user_3.relay_id, "profileId": profile.relay_id}}, | ||
| ) | ||
| content = response.json() | ||
| assert content["data"]["profileUserRoleDelete"]["deletedId"] == expected_relay_id | ||
| assert not ProfileUserRole.objects.filter(id=profile_user_role.id).exists() |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
# First, let's verify the test functions exist in both files
echo "=== Finding test functions in delete file ==="
rg -n "def test_user.*remove_profile_member" baseapp_profiles/tests/test_graphql_mutations_delete.py -A 2
echo ""
echo "=== Finding test functions in update file ==="
rg -n "def test_user.*remove_profile_member" baseapp_profiles/tests/test_graphql_mutations_update.py -A 2Repository: silverlogic/baseapp-backend
Length of output: 1387
🏁 Script executed:
# Now let's extract and compare the full test content
echo "=== Full content of tests in delete file (lines 105-155) ==="
sed -n '105,155p' baseapp_profiles/tests/test_graphql_mutations_delete.py
echo ""
echo "=== Full content of tests in update file (lines 321-369) ==="
sed -n '321,369p' baseapp_profiles/tests/test_graphql_mutations_update.pyRepository: silverlogic/baseapp-backend
Length of output: 4219
🏁 Script executed:
# Let's also check the exact line numbers where these tests start and end
echo "=== Checking exact test boundaries in delete file ==="
grep -n "^def test_user.*remove_profile_member" baseapp_profiles/tests/test_graphql_mutations_delete.py
echo ""
echo "=== Checking exact test boundaries in update file ==="
grep -n "^def test_user.*remove_profile_member" baseapp_profiles/tests/test_graphql_mutations_update.pyRepository: silverlogic/baseapp-backend
Length of output: 713
Remove duplicate tests from test_graphql_mutations_update.py.
test_user_profile_owner_can_remove_profile_member and test_user_with_permission_can_remove_profile_member are functionally identical in both files (delete file lines 105–155 match update file lines 321–369). These tests belong only in test_graphql_mutations_delete.py since they test deletion behavior. Delete the duplicate versions from the update file.
🤖 Prompt for AI Agents
In `@baseapp_profiles/tests/test_graphql_mutations_delete.py` around lines 105 -
155, Two test functions are duplicated; remove the duplicate definitions of
test_user_profile_owner_can_remove_profile_member and
test_user_with_permission_can_remove_profile_member from the file that tests
updates (keep the versions in the delete tests file), ensuring only the deletion
tests remain; search for the function names to locate and delete the duplicate
blocks so the deletion-focused tests exist in one place.
| def test_user_without_permission_cant_remove_profile_member( | ||
| django_user_client, graphql_user_client | ||
| ): | ||
| perm = Permission.objects.get( | ||
| content_type__app_label=ProfileUserRole._meta.app_label, codename="add_profileuserrole" | ||
| ) | ||
|
|
||
| user = django_user_client.user | ||
| user.user_permissions.add(perm) | ||
| user_2 = UserFactory() | ||
| user_3 = UserFactory() | ||
|
|
||
| profile = ProfileFactory(owner=user_2) | ||
| ProfileUserRoleFactory(profile=profile, user=user, role=ProfileUserRole.ProfileRoles.MANAGER) | ||
| profile_user_role = ProfileUserRoleFactory( | ||
| profile=profile, user=user_3, role=ProfileUserRole.ProfileRoles.MANAGER | ||
|
|
||
| response = graphql_user_client( | ||
| PROFILE_USER_ROLE_CREATE_GRAPHQL, | ||
| variables={ | ||
| "input": { | ||
| "usersIds": [user_2.relay_id], | ||
| "profileId": profile.relay_id, | ||
| "roleType": "ADMIN", | ||
| } | ||
| }, | ||
| ) | ||
| content = response.json() | ||
| assert content["errors"][0]["message"] == "You don't have permission to perform this action" | ||
| profile.refresh_from_db() |
There was a problem hiding this comment.
Test name is misleading — it tests profile user role creation, not removal.
The function is named test_user_without_permission_cant_remove_profile_member but it uses PROFILE_USER_ROLE_CREATE_GRAPHQL with create-style input (usersIds, roleType). This appears to be testing that a non-owner can't create a role on someone else's profile, not that they can't remove a member.
🐛 Proposed rename
-def test_user_without_permission_cant_remove_profile_member(
+def test_user_without_permission_cant_add_profile_user_role(
django_user_client, graphql_user_client
):🤖 Prompt for AI Agents
In `@baseapp_profiles/tests/test_graphql_mutations_update.py` around lines 372 -
396, Rename the misleading test function
test_user_without_permission_cant_remove_profile_member to reflect that it
asserts blocking creation of a profile role; update the function name to
something like test_user_without_permission_cant_create_profile_user_role (or
test_user_without_permission_cant_add_profile_member) and keep the existing test
body that uses PROFILE_USER_ROLE_CREATE_GRAPHQL and the create-style input
(usersIds, roleType) so the name matches the behavior being tested.
<!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Profile invitation workflow: send, accept, decline, cancel, and resend invitations via GraphQL with token-based expiry, status transitions, and mobile deep-linking with web fallback. * Email invitations with HTML/text templates and configurable acceptance URLs; invitees matched to existing accounts or invited by email with role assignment. * **Tests** * Extensive test coverage for invitations: creation, tokens, expiry, state transitions, permissions, and email sending. * **Chores** * DB schema migrations and new expiration constant for invitation handling; deterministic test factory keys. <sub>✏️ Tip: You can customize this high-level summary in your review settings.</sub> <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Gabriel Almeida <ga@tsl.io> Co-authored-by: Alisson Patricio <eu@alisson.net>
3 participants
Summary by CodeRabbit
Release Notes
New Features
Tests