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
WalkthroughAdds invitation-driven profile user-role management: new GraphQL mutations and permission checks, invitation lifecycle (token, expiry) with email sending and templates, model fields and migrations for invitations, profile-member filtering by email/profile name, and extensive tests for invitations and mutations. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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>
…ic (#391) ## Summary - Handle email delivery failures gracefully in `SendInvitation` and `ResendInvitation` mutations using try/except with logging, so the invitation is still created/updated even if the email service fails - Add `emailSent` boolean field to both mutation responses, allowing the frontend to show appropriate feedback when email delivery fails - Add tests covering email failure scenarios for both send and resend flows ## Context This addresses the concern raised in [PR #368](#368 (comment)) comment about what happens when the email service fails to send an invitation. Previously, if `send_invitation_email()` raised an exception, the entire mutation would fail and the database transaction would roll back — leaving no record of the invitation attempt. Now the mutation succeeds regardless of email delivery, and the `emailSent` response field lets the frontend inform the user and offer a resend option.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
baseapp_profiles/graphql/mutations.py (2)
300-330:⚠️ Potential issue | 🟠 MajorHandle
ProfileUserRole.DoesNotExistinstead ofif not obj.
objects.get(...)will raise before theif not objcheck is reached.🐛 Robust lookup
- obj = ProfileUserRole.objects.get(user_id=user_pk, profile_id=profile_pk) - - if not obj: - raise GraphQLError(str(_("User role not found"))) + try: + obj = ProfileUserRole.objects.get(user_id=user_pk, profile_id=profile_pk) + except ProfileUserRole.DoesNotExist: + raise GraphQLError(str(_("User role not found"))) from None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@baseapp_profiles/graphql/mutations.py` around lines 300 - 330, The code calls ProfileUserRole.objects.get(...) and then checks if not obj, but get() raises ProfileUserRole.DoesNotExist; update ProfileUserRoleDelete.mutate_and_get_payload to wrap the lookup in a try/except catching ProfileUserRole.DoesNotExist and raise a GraphQLError("User role not found") (or localized equivalent) instead of the invalid if-not check, then proceed with the permission check on obj.profile, set id_to_return = obj.relay_id and delete obj as before.
209-262:⚠️ Potential issue | 🟠 MajorGuard
users_idswhen it’s omitted to avoidTypeError.
input.get("users_ids")can beNone, which makes the list comprehension fail.🐛 Safe guard
- profile_user_roles = [ + 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
Verify each finding against the current code and only fix it if needed. In `@baseapp_profiles/graphql/mutations.py` around lines 209 - 262, The list comprehension building profile_user_roles in ProfileUserRoleCreate.mutate_and_get_payload assumes users_ids is iterable but input.get("users_ids") can be None; guard it by normalizing users_ids to an empty list (or validate and raise a GraphQLError) before the comprehension so the loop over users_ids and the call to get_pk_from_relay_id(...) never receives None; update the users_ids handling right after reading input to either users_ids = users_ids or users_ids = [] (or raise) and then proceed to create ProfileUserRole instances and call ProfileUserRole.objects.bulk_create(...)
🧹 Nitpick comments (7)
baseapp_profiles/migrations/0010_alter_profileuserrole_status.py (1)
6-27: This migration exists solely to correct thestatusdefault set in0008— can be dropped if0008is updated.As noted in
0008, ifdefault=3is applied directly there, this migration becomes a no-op and the migration chain is cleaner. No independent issues otherwise.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@baseapp_profiles/migrations/0010_alter_profileuserrole_status.py` around lines 6 - 27, This migration (Migration class in 0010_alter_profileuserrole_status.py) only corrects the ProfileUserRole.status default to 3; either remove this migration from the migration list or instead set default=3 on the status field in the earlier migration 0008_profileuserrole...(the migration that introduced the status field) so that 0010 becomes a no-op and can be dropped—update the IntegerField choices/default in the migration that created/altered ProfileUserRole.status (matching model_name="profileuserrole", name="status") and then delete 0010 to keep the migration chain clean.baseapp_profiles/migrations/0008_profileuserrole_invitation_expires_at_and_more.py (1)
69-81: Unnecessary intermediatestatusdefault — consolidate migrations 0008 and 0010.This migration sets
statusdefault to2(pending), but0010immediately overwrites it to3(inactive). In a production deployment where migrations run sequentially, anyProfileUserRolecreated between 0008 and 0010 (or if 0010 is missed) will carry the wrong default. Thetestproject/0005migration goes directly todefault=3, which is the correct final state. Since 0008 has not yet been applied to production (this is an open PR), consider settingdefault=3here and dropping migration 0010 entirely.♻️ Proposed fix (set correct default directly in 0008)
field=models.IntegerField( choices=[ (1, "active"), (2, "pending"), (3, "inactive"), (4, "declined"), (5, "expired"), ], - default=2, + default=3, ),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@baseapp_profiles/migrations/0008_profileuserrole_invitation_expires_at_and_more.py` around lines 69 - 81, The migration alters ProfileUserRole.status to default=2 but a later migration (0010) changes it to 3; change the default in this migration's AlterField (model_name="profileuserrole", name="status") to default=3 so the correct final default is applied immediately, and remove or consolidate the subsequent migration 0010 to avoid an intermediate incorrect default being applied in production.baseapp_profiles/templates/profiles/emails/invitation-body.txt.j2 (1)
1-10: Consider adding a decline link, given the model supports an activedeclinedstatus.
ProfileUserRolehas adeclined(status=4) state and aresponded_atfield, implying the system supports explicit decline. Currently the only CTA isaccept_url; users who want to actively decline must find the flow elsewhere. A decline URL mirroringaccept_url(e.g.decline_url) would close this gap.Additionally, if
expiration_dateis passed as a rawdatetimeobject fromemails.py, the rendered format may be unreadable. Verify it is pre-formatted to a human-friendly string before being passed to the template context.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@baseapp_profiles/templates/profiles/emails/invitation-body.txt.j2` around lines 1 - 10, Add an explicit decline CTA to the invitation template and ensure the expiration date is pre-formatted before rendering: update invitation-body.txt.j2 to render a new decline_url alongside the existing accept_url (reference symbols: invitation-body.txt.j2, accept_url, decline_url, expiration_date) and extend the email generation in emails.py to build/pass a human-friendly expiration_date string (e.g., strftime or a localization helper) and a decline_url pointing to the endpoint that records ProfileUserRole declined/status=4 and sets responded_at (reference symbols: emails.py, ProfileUserRole, responded_at). Ensure the decline_url mirrors accept_url logic and that emails.py populates both URLs and a formatted expiration_date in the template context.baseapp_profiles/graphql/object_types.py (1)
38-53: Ruff RUF012: prefer tuples for class Meta attributes.
Using tuples avoids mutable class-attribute warnings without changing behavior.♻️ Proposed change
class Meta: model = ProfileUserRole - interfaces = [RelayNode] - fields = [ + interfaces = (RelayNode,) + fields = ( "id", "pk", "user", "role", "status", "created", "modified", "invited_email", "invited_at", "invitation_expires_at", "responded_at", - ] + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@baseapp_profiles/graphql/object_types.py` around lines 38 - 53, The Meta class for the GraphQL type uses mutable lists for class attributes; change interfaces and fields in the Meta of the ProfileUserRole-based type to use tuples instead of lists (e.g., replace interfaces = [RelayNode] and fields = [ "id", "pk", ... ] with interfaces = (RelayNode,) and fields = ("id", "pk", ...)) to satisfy Ruff RUF012 and avoid mutable class-attribute warnings.baseapp_profiles/emails.py (1)
17-25:inviteris unused; either use it or silence ARG001 explicitly.
If you’re keeping the parameter for API compatibility, consider marking it as intentionally unused.♻️ Minimal lint-safe tweak
-def create_invitation(profile, inviter, invited_email, role=ProfileUserRole.ProfileRoles.MANAGER): +def create_invitation(profile, inviter, invited_email, role=ProfileUserRole.ProfileRoles.MANAGER): # noqa: ARG001🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@baseapp_profiles/emails.py` around lines 17 - 25, The parameter inviter in create_invitation is declared but never used; either use it where needed or mark it intentionally unused to satisfy linters—e.g., rename the parameter to _inviter (or assign it to a short unused variable like _ = inviter) or add an explicit linter suppression comment (e.g., "# noqa: ARG001" or the project’s preferred suppression) on the function signature so create_invitation(profile, inviter, ...) becomes lint-clean without changing behavior.baseapp_profiles/models.py (1)
270-279: Ruff RUF012: use a tuple forMeta.constraints.
Prevents mutable class-attribute warnings with no functional change.♻️ Proposed change
class Meta: abstract = True unique_together = [("user", "profile")] - constraints = [ + constraints = ( models.UniqueConstraint( fields=["profile", "invited_email"], name="unique_profile_invited_email", condition=models.Q(invited_email__isnull=False), ), - ] + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@baseapp_profiles/models.py` around lines 270 - 279, Meta in the abstract model defines constraints as a list which triggers Ruff RUF012; change the mutable list to an immutable tuple by replacing the list assigned to Meta.constraints with a tuple containing the existing models.UniqueConstraint instance (i.e., update the class Meta in the abstract model that currently has unique_together and constraints to use constraints = ( models.UniqueConstraint(...), ) so the constraint remains the same but is an immutable tuple).baseapp_profiles/tests/test_invitations.py (1)
400-535: Use the patched mocks/fixtures (or assert calls) to avoid ARG002 noise.
A few tests patchsend_invitation_emailbut never reference the mock, and some acceptdjango_user_clientwithout use. If Ruff is enforced, this will warn and the intent isn’t asserted. Consider asserting the call or dropping the unused fixture.♻️ Suggested adjustments (apply similarly to other tests in these blocks)
def test_send_invitation_succeeds_even_if_email_fails( self, mock_send_email, django_user_client, graphql_user_client ): ... response = graphql_user_client(mutation, variables=variables) content = response.json() + assert mock_send_email.called -def test_accept_null_user_wrong_email_returns_wrong_user( - self, django_user_client, graphql_user_client -): +def test_accept_null_user_wrong_email_returns_wrong_user(self, graphql_user_client): ...Also applies to: 619-823
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@baseapp_profiles/tests/test_invitations.py` around lines 400 - 535, Several tests patch baseapp_profiles.emails.send_invitation_email but never use the mock or have unused fixtures (e.g., django_user_client) which triggers ARG002; in each affected test (e.g., test_send_invitation_succeeds_even_if_email_fails and test_resend_invitation_succeeds_even_if_email_fails) either remove unused fixture params or add assertions on the mock (e.g., mock_send_email.assert_called() / assert_called_once() or assert_called_with(...) using the invitation data) to demonstrate the patched function was exercised; update any similar tests in the same block (and lines 619-823) the same way so the mock is referenced and unused fixtures are removed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@baseapp_profiles/graphql/mutations.py`:
- Around line 435-521: The code calls Profile.objects.get(pk=profile_pk) which
can raise Profile.DoesNotExist and cause a 500; wrap the lookup (after
get_pk_from_relay_id(profile_id)) in a try/except catching Profile.DoesNotExist
and raise a GraphQLError with a clear message (e.g. "Profile not found") and an
extensions code like "not_found" so the mutation (ProfileSendInvitation) returns
a controlled GraphQL error instead of crashing.
In `@testproject/settings.py`:
- Line 300: FRONT_URL is referenced (for FRONT_ACCEPT_INVITATION_URL) but pulled
via a star import which Ruff flags as F405; add an explicit import for FRONT_URL
from baseapp_core.tests.settings (e.g. add from baseapp_core.tests.settings
import FRONT_URL) and update the other similar constants that rely on FRONT_URL
(the block around lines 295–299) to use the explicitly imported symbol so Ruff
no longer reports F405.
---
Duplicate comments:
In `@baseapp_profiles/graphql/mutations.py`:
- Around line 300-330: The code calls ProfileUserRole.objects.get(...) and then
checks if not obj, but get() raises ProfileUserRole.DoesNotExist; update
ProfileUserRoleDelete.mutate_and_get_payload to wrap the lookup in a try/except
catching ProfileUserRole.DoesNotExist and raise a GraphQLError("User role not
found") (or localized equivalent) instead of the invalid if-not check, then
proceed with the permission check on obj.profile, set id_to_return =
obj.relay_id and delete obj as before.
- Around line 209-262: The list comprehension building profile_user_roles in
ProfileUserRoleCreate.mutate_and_get_payload assumes users_ids is iterable but
input.get("users_ids") can be None; guard it by normalizing users_ids to an
empty list (or validate and raise a GraphQLError) before the comprehension so
the loop over users_ids and the call to get_pk_from_relay_id(...) never receives
None; update the users_ids handling right after reading input to either
users_ids = users_ids or users_ids = [] (or raise) and then proceed to create
ProfileUserRole instances and call ProfileUserRole.objects.bulk_create(...)
---
Nitpick comments:
In `@baseapp_profiles/emails.py`:
- Around line 17-25: The parameter inviter in create_invitation is declared but
never used; either use it where needed or mark it intentionally unused to
satisfy linters—e.g., rename the parameter to _inviter (or assign it to a short
unused variable like _ = inviter) or add an explicit linter suppression comment
(e.g., "# noqa: ARG001" or the project’s preferred suppression) on the function
signature so create_invitation(profile, inviter, ...) becomes lint-clean without
changing behavior.
In `@baseapp_profiles/graphql/object_types.py`:
- Around line 38-53: The Meta class for the GraphQL type uses mutable lists for
class attributes; change interfaces and fields in the Meta of the
ProfileUserRole-based type to use tuples instead of lists (e.g., replace
interfaces = [RelayNode] and fields = [ "id", "pk", ... ] with interfaces =
(RelayNode,) and fields = ("id", "pk", ...)) to satisfy Ruff RUF012 and avoid
mutable class-attribute warnings.
In
`@baseapp_profiles/migrations/0008_profileuserrole_invitation_expires_at_and_more.py`:
- Around line 69-81: The migration alters ProfileUserRole.status to default=2
but a later migration (0010) changes it to 3; change the default in this
migration's AlterField (model_name="profileuserrole", name="status") to
default=3 so the correct final default is applied immediately, and remove or
consolidate the subsequent migration 0010 to avoid an intermediate incorrect
default being applied in production.
In `@baseapp_profiles/migrations/0010_alter_profileuserrole_status.py`:
- Around line 6-27: This migration (Migration class in
0010_alter_profileuserrole_status.py) only corrects the ProfileUserRole.status
default to 3; either remove this migration from the migration list or instead
set default=3 on the status field in the earlier migration
0008_profileuserrole...(the migration that introduced the status field) so that
0010 becomes a no-op and can be dropped—update the IntegerField choices/default
in the migration that created/altered ProfileUserRole.status (matching
model_name="profileuserrole", name="status") and then delete 0010 to keep the
migration chain clean.
In `@baseapp_profiles/models.py`:
- Around line 270-279: Meta in the abstract model defines constraints as a list
which triggers Ruff RUF012; change the mutable list to an immutable tuple by
replacing the list assigned to Meta.constraints with a tuple containing the
existing models.UniqueConstraint instance (i.e., update the class Meta in the
abstract model that currently has unique_together and constraints to use
constraints = ( models.UniqueConstraint(...), ) so the constraint remains the
same but is an immutable tuple).
In `@baseapp_profiles/templates/profiles/emails/invitation-body.txt.j2`:
- Around line 1-10: Add an explicit decline CTA to the invitation template and
ensure the expiration date is pre-formatted before rendering: update
invitation-body.txt.j2 to render a new decline_url alongside the existing
accept_url (reference symbols: invitation-body.txt.j2, accept_url, decline_url,
expiration_date) and extend the email generation in emails.py to build/pass a
human-friendly expiration_date string (e.g., strftime or a localization helper)
and a decline_url pointing to the endpoint that records ProfileUserRole
declined/status=4 and sets responded_at (reference symbols: emails.py,
ProfileUserRole, responded_at). Ensure the decline_url mirrors accept_url logic
and that emails.py populates both URLs and a formatted expiration_date in the
template context.
In `@baseapp_profiles/tests/test_invitations.py`:
- Around line 400-535: Several tests patch
baseapp_profiles.emails.send_invitation_email but never use the mock or have
unused fixtures (e.g., django_user_client) which triggers ARG002; in each
affected test (e.g., test_send_invitation_succeeds_even_if_email_fails and
test_resend_invitation_succeeds_even_if_email_fails) either remove unused
fixture params or add assertions on the mock (e.g.,
mock_send_email.assert_called() / assert_called_once() or
assert_called_with(...) using the invitation data) to demonstrate the patched
function was exercised; update any similar tests in the same block (and lines
619-823) the same way so the mock is referenced and unused fixtures are removed.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
baseapp_profiles/constants.pybaseapp_profiles/emails.pybaseapp_profiles/graphql/mutations.pybaseapp_profiles/graphql/object_types.pybaseapp_profiles/migrations/0008_profileuserrole_invitation_expires_at_and_more.pybaseapp_profiles/migrations/0009_profileuserrole_unique_profile_invited_email.pybaseapp_profiles/migrations/0010_alter_profileuserrole_status.pybaseapp_profiles/models.pybaseapp_profiles/templates/profiles/emails/invitation-body.html.j2baseapp_profiles/templates/profiles/emails/invitation-body.txt.j2baseapp_profiles/templates/profiles/emails/invitation-subject.txt.j2baseapp_profiles/tests/test_invitations.pybaseapp_reports/tests/factories.pytestproject/profiles/migrations/0005_profileuserrole_invitation_expires_at_and_more.pytestproject/settings.py
✅ Files skipped from review due to trivial changes (1)
- baseapp_profiles/templates/profiles/emails/invitation-body.html.j2
| profile_id = input.get("profile_id") | ||
| email = input.get("email") | ||
| role = input.get("role") | ||
|
|
||
| profile_pk = get_pk_from_relay_id(profile_id) | ||
| profile = Profile.objects.get(pk=profile_pk) | ||
|
|
||
| if not info.context.user.has_perm( | ||
| f"{profile_user_role_app_label}.add_profileuserrole", profile | ||
| ): | ||
| raise GraphQLError( | ||
| str(_("You don't have permission to perform this action")), | ||
| extensions={"code": "permission_required"}, | ||
| ) | ||
|
|
||
| normalized_email = email.lower() | ||
| User = get_user_model() | ||
|
|
||
| try: | ||
| invited_user = User.objects.get(email__iexact=normalized_email) | ||
| except User.DoesNotExist: | ||
| invited_user = None | ||
|
|
||
| with transaction.atomic(): | ||
| existing_role = ( | ||
| ProfileUserRole.objects.select_for_update() | ||
| .filter(profile=profile, invited_email__iexact=normalized_email) | ||
| .first() | ||
| ) | ||
|
|
||
| if existing_role: | ||
| if existing_role.status == ProfileUserRole.ProfileRoleStatus.ACTIVE: | ||
| raise GraphQLError( | ||
| str(_("This user is already a member of this profile")), | ||
| extensions={"code": "already_member"}, | ||
| ) | ||
|
|
||
| if existing_role.status == ProfileUserRole.ProfileRoleStatus.PENDING: | ||
| if not existing_role.is_invitation_expired(): | ||
| raise GraphQLError( | ||
| str(_("An invitation has already been sent to this email")), | ||
| extensions={"code": "duplicate_invitation"}, | ||
| ) | ||
| existing_role.status = ProfileUserRole.ProfileRoleStatus.EXPIRED | ||
|
|
||
| if existing_role.status in [ | ||
| ProfileUserRole.ProfileRoleStatus.DECLINED, | ||
| ProfileUserRole.ProfileRoleStatus.INACTIVE, | ||
| ProfileUserRole.ProfileRoleStatus.EXPIRED, | ||
| ]: | ||
| _reset_invitation_for_send(existing_role, role=role, user=invited_user) | ||
| existing_role.save() | ||
| invitation = existing_role | ||
| else: | ||
| raise GraphQLError( | ||
| str(_("Cannot send invitation in current state")), | ||
| extensions={"code": "invalid_status"}, | ||
| ) | ||
| else: | ||
| try: | ||
| invitation = ProfileUserRole.objects.create( | ||
| profile=profile, | ||
| user=invited_user, | ||
| invited_email=normalized_email, | ||
| role=role, | ||
| status=ProfileUserRole.ProfileRoleStatus.PENDING, | ||
| invited_at=timezone.now(), | ||
| invitation_expires_at=( | ||
| timezone.now() + timedelta(days=INVITATION_EXPIRATION_DAYS) | ||
| ), | ||
| ) | ||
| invitation.generate_invitation_token() | ||
| invitation.save() | ||
| except IntegrityError: | ||
| raise GraphQLError( | ||
| str(_("An invitation has already been sent to this email")), | ||
| extensions={"code": "duplicate_invitation"}, | ||
| ) | ||
|
|
||
| email_sent = True | ||
| try: | ||
| send_invitation_email(invitation, info.context.user) | ||
| except Exception: | ||
| logger.exception("Failed to send invitation email to %s", normalized_email) | ||
| email_sent = False | ||
|
|
||
| return ProfileSendInvitation(profile_user_role=invitation, email_sent=email_sent) |
There was a problem hiding this comment.
Handle missing profiles in ProfileSendInvitation.
Profile.objects.get(...) can raise DoesNotExist and bubble up as a 500.
🐛 Suggested guard
- profile = Profile.objects.get(pk=profile_pk)
+ try:
+ profile = Profile.objects.get(pk=profile_pk)
+ except Profile.DoesNotExist:
+ raise GraphQLError(
+ str(_("Profile not found")),
+ extensions={"code": "not_found"},
+ ) from None🧰 Tools
🪛 Ruff (0.15.2)
[warning] 509-512: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@baseapp_profiles/graphql/mutations.py` around lines 435 - 521, The code calls
Profile.objects.get(pk=profile_pk) which can raise Profile.DoesNotExist and
cause a 500; wrap the lookup (after get_pk_from_relay_id(profile_id)) in a
try/except catching Profile.DoesNotExist and raise a GraphQLError with a clear
message (e.g. "Profile not found") and an extensions code like "not_found" so
the mutation (ProfileSendInvitation) returns a controlled GraphQL error instead
of crashing.
| FRONT_CHANGE_EMAIL_CONFIRM_URL = FRONT_URL + "/change-email/{id}/{token}" | ||
| FRONT_CHANGE_EMAIL_VERIFY_URL = FRONT_URL + "/change-email-verify/{id}/{token}" | ||
| FRONT_CHANGE_EXPIRED_PASSWORD_URL = FRONT_URL + "/change-expired-password/{token}" | ||
| FRONT_ACCEPT_INVITATION_URL = FRONT_URL + "/accept-invitation/{token}" |
There was a problem hiding this comment.
Ruff F405: FRONT_URL may be undefined due to star import.
FRONT_URL is brought in via from baseapp_core.tests.settings import * and is not explicitly imported, which Ruff flags as potentially undefined (F405). The same pattern exists on lines 295–299, so the cleanest fix is a single explicit import alongside the existing env fix suggested in a prior review.
🛠️ Suggested fix
-from baseapp_core.tests.settings import * # noqa
+from baseapp_core.tests.settings import * # noqa
+from baseapp_core.tests.settings import env, FRONT_URL # noqa: F401🧰 Tools
🪛 Ruff (0.15.2)
[error] 300-300: FRONT_URL may be undefined, or defined from star imports
(F405)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@testproject/settings.py` at line 300, FRONT_URL is referenced (for
FRONT_ACCEPT_INVITATION_URL) but pulled via a star import which Ruff flags as
F405; add an explicit import for FRONT_URL from baseapp_core.tests.settings
(e.g. add from baseapp_core.tests.settings import FRONT_URL) and update the
other similar constants that rely on FRONT_URL (the block around lines 295–299)
to use the explicitly imported symbol so Ruff no longer reports F405.
| @@ -0,0 +1 @@ | |||
| INVITATION_EXPIRATION_DAYS = 7 | |||
There was a problem hiding this comment.
can we move this to a django settings var instead? make it get from django settings, if it is not present, then use 7 as default
3 participants
Summary by CodeRabbit
New Features
Tests