Improve accessy syncing performance and improve code-reuse. #586
Conversation
WalkthroughThe pull request introduces significant modifications to the Changes
Sequence DiagramsequenceDiagram
participant Sync as Sync Module
participant AccessySession as Accessy Session
participant Member as AccessyMember
alt Sync Specific Member
Sync->>Sync: Get member details
Sync->>AccessySession: Remove/Add to groups
AccessySession->>Member: Perform operations
else Sync All Members
Sync->>Sync: Fetch all members
loop For Each Member
Sync->>AccessySession: Sync member access
end
end
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
api/src/multiaccessy/invite.py (1)
66-66: Add exception handling for thesyncfunction callThe
syncfunction may raise exceptions due to network issues or data inconsistencies. To enhance robustness, consider wrapping thesync(member_id)call in a try-except block to handle potential exceptions gracefully.Apply this diff to add exception handling:
if not summary.labaccess_active and not summary.special_labaccess_active: raise AccessyInvitePreconditionFailed("ingen aktiv labaccess") + try: sync(member_id=member_id) + except Exception as e: + logger.error(f"Failed to sync member {member_id}: {e}") + raiseapi/src/multiaccessy/sync.py (2)
33-33: Raise more specific exceptions instead of genericExceptionIn lines 33 and 120, raising a generic
Exceptionlacks specificity and can make error handling less precise. Use a more specific exception likeValueErrorto provide clearer context about the error.Apply this diff to improve exception handling:
Line 33: - raise Exception("Member does not exist") + raise ValueError(f"Member with ID {member_id} does not exist") Line 120: - raise Exception("Member does not exist") + raise ValueError(f"Member with ID {member_id} does not exist")Also applies to: 120-120
121-122: Add logging when skipping a member due to missing phone numberCurrently, if a member's phone number is
None, the function silently returns without logging any information. Adding a log entry will improve traceability and debugging.Apply this diff to add a log message:
if member.phone is None: + logger.info(f"Skipping sync for member {member_id} because phone number is missing") returnapi/src/multiaccessy/accessy.py (1)
274-275: Ensuremembership_idis populated before API callsIn
remove_from_organdremove_from_group,member.membership_idis used. Ensure thatmembership_idis notNoneto prevent API errors. If necessary, add checks or raise exceptions when it's missing.Consider adding an assertion:
def remove_from_org(self, member: AccessyMember) -> None: + assert member.membership_id is not None, "membership_id is required" self.__delete(f"/org/admin/organization/{self.organization_id()}/user/{member.user_id}")Also applies to: 277-278
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
api/src/multiaccessy/accessy.py(4 hunks)api/src/multiaccessy/invite.py(2 hunks)api/src/multiaccessy/sync.py(3 hunks)api/src/systest/api/accessy_sync_test.py(1 hunks)
🔇 Additional comments (5)
api/src/multiaccessy/invite.py (1)
16-17: Import statement is appropriate and maintains module dependencies
The import of sync from .sync ensures that the function is available for use in this module. There are no issues with circular dependencies or module resolution.
api/src/multiaccessy/sync.py (1)
107-164: Modifications enhance targeted synchronization and maintain correctness
The updates to sync function support syncing a specific member when member_id is provided, improving flexibility. The changes are well-integrated and maintain the overall functionality.
api/src/systest/api/accessy_sync_test.py (2)
98-107: Verify inclusion of members without lab access agreement
In the test, a member without a labaccess_agreement_at is included in the wanted access list. Please verify if this behavior aligns with the intended access policies, as it may allow access without signed agreements.
49-154: Updates to test cases reflect new member-span relationships
The test cases have been correctly updated to pass the member parameter to create_span, ensuring spans are associated with the correct members. Assertions have been adjusted appropriately.
api/src/multiaccessy/accessy.py (1)
184-184: Updates correctly handle optional msisdn values
Changing msisdn to be optional and adjusting the handling ensures that users without a phone number are managed appropriately, enhancing robustness.
Also applies to: 526-528
| def get_wanted_access(today: date, member_id: Optional[int] = None) -> dict[PHONE, AccessyMember]: | ||
| if member_id is not None: | ||
| member = db_session.query(Member).get(member_id) | ||
| if member is None: | ||
| raise Exception("Member does not exist") | ||
| members = [member] | ||
| summaries = [get_membership_summary(member_id, at_date=today)] | ||
| else: | ||
| members, summaries = get_members_and_membership(at_date=today) | ||
|
|
||
| return { | ||
| m.phone: AccessyMember( | ||
| phone=m.phone, | ||
| name=f"{m.firstname} {m.lastname}", | ||
| groups={GROUPS[span.type] for span in m.spans}, | ||
| member_id=m.member_id, | ||
| member_number=m.member_number, | ||
| member.phone: AccessyMember( | ||
| phone=member.phone, | ||
| name=f"{member.firstname} {member.lastname}", | ||
| groups={ | ||
| group | ||
| for group, enabled in { | ||
| ACCESSY_SPECIAL_LABACCESS_GROUP: membership.special_labaccess_active, | ||
| ACCESSY_LABACCESS_GROUP: membership.labaccess_active, | ||
| }.items() | ||
| if enabled | ||
| }, | ||
| member_id=member.member_id, | ||
| member_number=member.member_number, | ||
| ) | ||
| for m in query | ||
| for member, membership in zip(members, summaries) | ||
| if member.phone is not None and membership.effective_labaccess_active |
There was a problem hiding this comment.
Critical issue: Missing check for lab access agreement
The get_wanted_access function does not verify if member.labaccess_agreement_at is set. Including members who haven't signed the lab access agreement might violate policy or introduce security risks.
Consider updating the filtering condition to ensure only members who have signed the lab access agreement are included:
for member, membership in zip(members, summaries)
- if member.phone is not None and membership.effective_labaccess_active
+ if (
+ member.phone is not None
+ and membership.effective_labaccess_active
+ and member.labaccess_agreement_at is not None
+ )
}📝 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 get_wanted_access(today: date, member_id: Optional[int] = None) -> dict[PHONE, AccessyMember]: | |
| if member_id is not None: | |
| member = db_session.query(Member).get(member_id) | |
| if member is None: | |
| raise Exception("Member does not exist") | |
| members = [member] | |
| summaries = [get_membership_summary(member_id, at_date=today)] | |
| else: | |
| members, summaries = get_members_and_membership(at_date=today) | |
| return { | |
| m.phone: AccessyMember( | |
| phone=m.phone, | |
| name=f"{m.firstname} {m.lastname}", | |
| groups={GROUPS[span.type] for span in m.spans}, | |
| member_id=m.member_id, | |
| member_number=m.member_number, | |
| member.phone: AccessyMember( | |
| phone=member.phone, | |
| name=f"{member.firstname} {member.lastname}", | |
| groups={ | |
| group | |
| for group, enabled in { | |
| ACCESSY_SPECIAL_LABACCESS_GROUP: membership.special_labaccess_active, | |
| ACCESSY_LABACCESS_GROUP: membership.labaccess_active, | |
| }.items() | |
| if enabled | |
| }, | |
| member_id=member.member_id, | |
| member_number=member.member_number, | |
| ) | |
| for m in query | |
| for member, membership in zip(members, summaries) | |
| if member.phone is not None and membership.effective_labaccess_active | |
| def get_wanted_access(today: date, member_id: Optional[int] = None) -> dict[PHONE, AccessyMember]: | |
| if member_id is not None: | |
| member = db_session.query(Member).get(member_id) | |
| if member is None: | |
| raise Exception("Member does not exist") | |
| members = [member] | |
| summaries = [get_membership_summary(member_id, at_date=today)] | |
| else: | |
| members, summaries = get_members_and_membership(at_date=today) | |
| return { | |
| member.phone: AccessyMember( | |
| phone=member.phone, | |
| name=f"{member.firstname} {member.lastname}", | |
| groups={ | |
| group | |
| for group, enabled in { | |
| ACCESSY_SPECIAL_LABACCESS_GROUP: membership.special_labaccess_active, | |
| ACCESSY_LABACCESS_GROUP: membership.labaccess_active, | |
| }.items() | |
| if enabled | |
| }, | |
| member_id=member.member_id, | |
| member_number=member.member_number, | |
| ) | |
| for member, membership in zip(members, summaries) | |
| if ( | |
| member.phone is not None | |
| and membership.effective_labaccess_active | |
| and member.labaccess_agreement_at is not None | |
| ) |
HalfVoxel
force-pushed
the
aron/improve_accessy
branch
from
aa830a4 to
4c66716
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
api/src/multiaccessy/sync.py (1)
23-49:⚠️ Potential issueCritical: Lab access agreement verification still missing
The function allows access for members even without a signed lab access agreement, which was flagged in a previous review.
Consider adding the lab access agreement check:
return { member.phone: AccessyMember( phone=member.phone, name=f"{member.firstname} {member.lastname}", groups={ group for group, enabled in { ACCESSY_SPECIAL_LABACCESS_GROUP: membership.special_labaccess_active, ACCESSY_LABACCESS_GROUP: membership.labaccess_active, }.items() if enabled }, member_id=member.member_id, member_number=member.member_number, ) for member, membership in zip(members, summaries) - if member.phone is not None and membership.effective_labaccess_active + if member.phone is not None and membership.effective_labaccess_active and member.labaccess_agreement_at is not None }
🧹 Nitpick comments (2)
api/src/systest/api/accessy_sync_test.py (1)
98-107: Add test cases for edge casesThe test for unsigned lab access agreement should include more scenarios, such as expired agreements or pending agreements.
Consider adding these test cases:
# Test expired agreement expired_agreement_member = self.db.create_member( phone=random_phone_number(), labaccess_agreement_at=self.date(-30), # 30 days ago ) self.db.create_span( startdate=self.date(0), enddate=self.date(0), type=Span.LABACCESS, member=expired_agreement_member, ) # Test pending agreement pending_agreement_member = self.db.create_member( phone=random_phone_number(), labaccess_agreement_at=self.date(1), # Future date ) self.db.create_span( startdate=self.date(0), enddate=self.date(0), type=Span.LABACCESS, member=pending_agreement_member, )api/src/membership/membership.py (1)
Line range hint
1-240: Consider optimizing database queries for better performance.Since this PR focuses on improving syncing performance, consider these optimizations:
- The similar SQL queries for
labaccess_active,membership_active, andspecial_labaccess_activecould be combined into a single query using a UNION or conditional aggregation.- Add database indices for frequently queried columns (
member_id,type,startdate,enddate).- Consider caching membership summaries for frequently accessed members.
Would you like me to provide a specific implementation for any of these optimizations?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
api/src/membership/membership.py(1 hunks)api/src/multiaccessy/accessy.py(4 hunks)api/src/multiaccessy/invite.py(2 hunks)api/src/multiaccessy/sync.py(3 hunks)api/src/systest/api/accessy_sync_test.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- api/src/multiaccessy/accessy.py
🔇 Additional comments (4)
api/src/multiaccessy/invite.py (2)
16-16: LGTM: Good modularization of sync functionality
The import of the sync function aligns well with the PR's objective of improving code reuse.
66-66: Verify edge cases in the sync function
While the refactoring improves code reuse, we should verify that all edge cases previously handled in this function are properly covered by the sync function.
✅ Verification successful
Edge cases are properly handled in the sync function
The verification shows that the sync function adequately handles all relevant edge cases:
- Member existence check: Both functions verify if the member exists and raise appropriate exceptions
- Phone number validation: Both handle cases where phone numbers are missing
- Access validation: The invite.py's precondition check for active lab access is maintained as it runs before calling sync()
- Configuration checks: The sync function includes proper validation for Accessy configuration (ACCESSY_LABACCESS_GROUP, ACCESSY_SPECIAL_LABACCESS_GROUP, accessy_session)
- Pending invites handling: The sync function includes additional logic to prevent duplicate invites
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for edge case handling in the old vs new implementation
# Search for error handling and edge cases in the old implementation
git log -p HEAD^..HEAD -- api/src/multiaccessy/invite.py | grep -A 5 "raise\|except\|if\|else"
# Search for corresponding error handling in the new sync implementation
rg "raise\|except\|if\|else" api/src/multiaccessy/sync.py
Length of output: 59
Script:
#!/bin/bash
# Let's try a different approach to analyze the changes and edge cases
# First, let's see the actual changes in invite.py
rg -A 5 -B 5 "sync\(member_id=" api/src/multiaccessy/invite.py
# Then check the sync function implementation
fd sync.py$ --exec cat {}
# Look for error handling patterns in both files
rg "try|except|raise|if\s+not|if\s+\w+\s+is\s+None" api/src/multiaccessy/{invite,sync}.py
Length of output: 7591
api/src/systest/api/accessy_sync_test.py (1)
135-146: LGTM: Good null safety checks
The added assertions for phone number presence improve the robustness of the tests.
api/src/membership/membership.py (1)
148-152: Documentation clearly explains the access control logic.
The added comments effectively document that lab access is determined solely by the presence of spans, independent of membership agreement status. This aligns with the PR objectives.
However, let's verify this security-relevant change:
✅ Verification successful
Based on the evidence from the shell scripts, I can now generate the final response:
The documented behavior of granting lab access based on spans is correct and verified by tests.
The codebase confirms this behavior through:
- Test cases in
api/src/systest/api/accessy_sync_test.pyexplicitly verify that lab access is granted based on spans, including a test caseincluded_even_if_labaccess_agreement_not_signed - The implementation in
api/src/membership/membership.pyshows thateffective_labaccess_activeis determined solely by the presence of either regular lab access spans or special lab access spans - The comment accurately reflects that while the agreement is typically required for adding spans (through order actions), the access control itself only depends on the spans
This is an intentional security model where:
- Spans are the source of truth for access control
- The membership agreement acts as a prerequisite for obtaining spans through normal processes
- Special access can be granted via spans even without an agreement when needed
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the intentional decoupling of lab access from membership agreement
# by checking for any related security discussions or documentation.
# Check for related security discussions in issues/PRs
gh search issues "membership agreement lab access spans" --repo makerspace/makeradmin
# Look for related security documentation
rg -g "!{node_modules,dist,build}/*" -A 5 "lab.*access.*agreement|membership.*agreement.*access"
# Check for any security-related tests
rg -g "*test*.py" -A 5 "test.*lab.*access.*agreement|test.*membership.*agreement"
Length of output: 18130
Script:
#!/bin/bash
# Let's verify the behavior by checking the implementation and tests
# Check the effective_labaccess_active implementation
ast-grep --pattern 'def effective_labaccess_active($$$)'
# Check for test cases specifically about lab access spans
rg -g "*test*.py" -A 10 "test.*labaccess.*span|test.*effective_labaccess"
# Check for any Accessy-related security checks
rg -g "!{node_modules,dist,build}/*" -A 5 "effective_labaccess.*active|labaccess.*span"
Length of output: 9255
| def sync(today: Optional[date] = None, member_id: Optional[int] = None) -> None: | ||
| if accessy_session is None: | ||
| logger.info(f"accessy sync skipped, accessy not configured.") | ||
| return | ||
|
|
||
| if not today: | ||
| today = date.today() | ||
|
|
||
| actual_members = accessy_session.get_all_members() | ||
| # If a specific member is given, sync only that member, | ||
| # otherwise sync all members | ||
| if member_id is not None: | ||
| member = db_session.query(Member).get(member_id) | ||
| if member is None: | ||
| raise Exception("Member does not exist") | ||
| if member.phone is None: | ||
| return | ||
| accessy_member = accessy_session._get_org_user_from_phone(member.phone) | ||
| actual_members = [accessy_member] if accessy_member is not None else [] | ||
| else: | ||
| actual_members = accessy_session.get_all_members() | ||
|
|
||
| pending_invites = accessy_session.get_pending_invitations(after_date=today - timedelta(days=7)) | ||
| wanted_members = get_wanted_access(today) | ||
| wanted_members = get_wanted_access(today, member_id=member_id) | ||
|
|
||
| actual_members_by_phone = {} | ||
| members_to_discard: List[AccessyMember] = [] | ||
| for m in actual_members: | ||
| if m.phone: | ||
| actual_members_by_phone[m.phone] = m | ||
| else: | ||
| logger.warning( | ||
| f"accessy sync got member %s from accessy without phone number, skipping in calculation, will probably cause extra invite or delayed org remove", | ||
| m, | ||
| ) | ||
| # Members with no phone numbers are probably accounts that have gotten reset | ||
| members_to_discard.append(m) | ||
|
|
||
| diff = calculate_diff(actual_members_by_phone, wanted_members) | ||
|
|
||
| for member in diff.invites: | ||
| if member.phone in pending_invites: | ||
| logger.info(f"accessy sync skipping, invite already pending: {member}") | ||
| for accessy_member in diff.invites: | ||
| assert accessy_member.phone is not None | ||
| if accessy_member.phone in pending_invites: | ||
| logger.info(f"accessy sync skipping, invite already pending: {accessy_member}") | ||
| continue | ||
| logger.info(f"accessy sync inviting: {member}") | ||
| accessy_session.invite_phone_to_org_and_groups([member.phone], member.groups) | ||
| logger.info(f"accessy sync inviting: {accessy_member}") | ||
| accessy_session.invite_phone_to_org_and_groups([accessy_member.phone], accessy_member.groups) | ||
|
|
||
| for op in diff.group_adds: | ||
| logger.info(f"accessy sync adding to group: {op}") | ||
| accessy_session.add_to_group(op.member.phone, op.group) | ||
| accessy_session.add_to_group(op.member, op.group) | ||
|
|
||
| for op in diff.group_removes: | ||
| logger.info(f"accessy sync removing from group: {op}") | ||
| accessy_session.remove_from_group(op.member.phone, op.group) | ||
| accessy_session.remove_from_group(op.member, op.group) | ||
|
|
||
| for accessy_member in diff.org_removes: | ||
| logger.info(f"accessy sync removing from org: {accessy_member}") | ||
| accessy_session.remove_from_org(accessy_member) | ||
|
|
||
| for member in diff.org_removes: | ||
| logger.info(f"accessy sync removing from org: {member}") | ||
| accessy_session.remove_from_org(member.phone) | ||
| for accessy_member in members_to_discard: | ||
| logger.info(f"accessy sync removing from org: {accessy_member}, because phone number is missing") | ||
| accessy_session.remove_from_org(accessy_member) |
There was a problem hiding this comment.
Improve error handling for API operations
The sync function performs multiple API operations without proper error handling. Network operations should be wrapped in try-except blocks.
Consider adding error handling:
for accessy_member in diff.invites:
assert accessy_member.phone is not None
if accessy_member.phone in pending_invites:
logger.info(f"accessy sync skipping, invite already pending: {accessy_member}")
continue
- logger.info(f"accessy sync inviting: {accessy_member}")
- accessy_session.invite_phone_to_org_and_groups([accessy_member.phone], accessy_member.groups)
+ try:
+ logger.info(f"accessy sync inviting: {accessy_member}")
+ accessy_session.invite_phone_to_org_and_groups([accessy_member.phone], accessy_member.groups)
+ except Exception as e:
+ logger.error(f"Failed to invite member {accessy_member}: {e}")Similar error handling should be added for other API operations.
Committable suggestion skipped: line range outside the PR's diff.
- Reduced the number of calls we make to the AccessyAPI during syncs. - Re-use our existing code for getting membership summaries, instead of doing custom database calls. - Use the syncing code for when updating a single member as well, to ensure there are no differences in behaviour.
HalfVoxel
force-pushed
the
aron/improve_accessy
branch
from
4dee06a to
b0c3110
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
api/src/multiaccessy/sync.py (1)
23-49: Improve type safety in get_wanted_access functionThe function handles optional member_id well, but could benefit from explicit type hints for the return dictionary values.
Consider this improvement:
-def get_wanted_access(today: date, member_id: Optional[int] = None) -> dict[PHONE, AccessyMember]: +def get_wanted_access(today: date, member_id: Optional[int] = None) -> Dict[PHONE, AccessyMember]:Also, consider adding error handling for the database query:
if member_id is not None: - member = db_session.query(Member).get(member_id) + member = db_session.query(Member).get(member_id) + if member is None: + logger.error(f"Member {member_id} not found") + raise ValueError(f"Member {member_id} not found")api/src/systest/api/accessy_sync_test.py (1)
98-107: Add test case for member-specific syncWhile the tests cover the general sync cases well, we should add a test for the new member-specific sync functionality.
Consider adding this test case:
def test_get_wanted_access_single_member(self) -> None: # Create a member with spans member = self.db.create_member( phone=random_phone_number(), labaccess_agreement_at=date.today(), ) self.db.create_span( startdate=self.date(0), enddate=self.date(0), type=Span.LABACCESS, member=member, ) # Test member-specific sync wanted = get_wanted_access(self.date(), member_id=member.member_id) self.assertEqual(len(wanted), 1) self.assertIn(member.phone, wanted) # Test with non-existent member with self.assertRaises(ValueError): get_wanted_access(self.date(), member_id=99999)Also applies to: 153-154
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
api/src/membership/membership.py(1 hunks)api/src/multiaccessy/accessy.py(4 hunks)api/src/multiaccessy/invite.py(2 hunks)api/src/multiaccessy/sync.py(3 hunks)api/src/systest/api/accessy_sync_test.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- api/src/membership/membership.py
- api/src/multiaccessy/accessy.py
🔇 Additional comments (5)
api/src/multiaccessy/invite.py (2)
16-16: LGTM: Import aligns with architectural changes
The new import of the sync function supports the refactoring to improve code reuse.
66-66: Verify error propagation from sync function
The function now delegates to sync(member_id) after precondition checks. While this improves code reuse, we should ensure error handling is consistent.
Run this script to analyze error handling:
✅ Verification successful
Let me gather more information about the sync function and its error handling.
Let me try a different approach to find the sync function.
Let me check the full implementation of the sync function to understand its error handling.
Let me check the invite function to understand how it handles errors from sync.
Let me check the error handling in the base HTTP methods of the Accessy client.
Let me check the request function implementation to complete the error handling analysis.
Based on the analysis of the codebase, I can now provide the final response regarding error propagation.
Error handling is properly propagated through the call chain
The sync function and its dependencies have consistent error handling:
- The sync function raises explicit exceptions for invalid member IDs:
raise Exception("Member does not exist") - All HTTP operations are handled through the Accessy client which raises
AccessyErrorfor API failures - The underlying HTTP operations (
__post,__put,__delete) properly propagate errors through the request function - Errors from the Accessy API client (like "too many requests", invalid API key, etc.) are properly raised as
AccessyError
The error handling chain is complete from the sync function through to the HTTP layer, ensuring that any failures will be properly propagated to the caller.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check error handling patterns in sync and invite functions
rg -A 5 "raise\s+\w+Error" api/src/multiaccessy/
Length of output: 2296
Script:
#!/bin/bash
# Check sync function implementation and error handling
ast-grep --pattern 'def sync($$$) { $$$ }'
Length of output: 44
Script:
#!/bin/bash
# Find sync function definition and its context
rg -B 2 -A 10 "^def sync" api/src/multiaccessy/
Length of output: 798
Script:
#!/bin/bash
# Get the full content of sync.py to analyze error handling
cat api/src/multiaccessy/sync.py
Length of output: 5865
Script:
#!/bin/bash
# Find the invite function implementation
rg -B 5 -A 10 "def invite" api/src/multiaccessy/
Length of output: 1331
Script:
#!/bin/bash
# Find the HTTP methods implementation in accessy.py
rg -B 2 -A 10 "def __(?:get|post|put|delete)" api/src/multiaccessy/accessy.py
Length of output: 1553
Script:
#!/bin/bash
# Find the request function implementation
rg -B 5 -A 15 "def request" api/src/multiaccessy/accessy.py
Length of output: 744
api/src/multiaccessy/sync.py (1)
126-132: LGTM: Improved handling of members without phone numbers
Good addition of handling for members with missing phone numbers, likely due to account resets.
api/src/systest/api/accessy_sync_test.py (2)
49-51: LGTM: Improved test structure with explicit member associations
The test setup is now more explicit with member associations in span creation.
Also applies to: 57-62, 68-79
135-140: LGTM: Robust phone number assertions
Good addition of explicit phone number assertions to catch potential null pointer issues early.
Also applies to: 145-148
| # If a specific member is given, sync only that member, | ||
| # otherwise sync all members | ||
| if member_id is not None: | ||
| member = db_session.query(Member).get(member_id) | ||
| if member is None: | ||
| raise Exception("Member does not exist") | ||
| if member.phone is None: | ||
| return | ||
| accessy_member = accessy_session._get_org_user_from_phone(member.phone) | ||
| actual_members = [accessy_member] if accessy_member is not None else [] | ||
| else: |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling for member-specific sync
The member-specific sync path needs more robust error handling and logging.
Consider these improvements:
if member_id is not None:
member = db_session.query(Member).get(member_id)
if member is None:
- raise Exception("Member does not exist")
+ logger.error(f"Cannot sync non-existent member {member_id}")
+ raise ValueError(f"Member {member_id} does not exist")
if member.phone is None:
+ logger.info(f"Skipping sync for member {member_id} without phone number")
return
accessy_member = accessy_session._get_org_user_from_phone(member.phone)
+ if accessy_member is None:
+ logger.info(f"Member {member_id} not found in Accessy")
actual_members = [accessy_member] if accessy_member is not None else []📝 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.
| # If a specific member is given, sync only that member, | |
| # otherwise sync all members | |
| if member_id is not None: | |
| member = db_session.query(Member).get(member_id) | |
| if member is None: | |
| raise Exception("Member does not exist") | |
| if member.phone is None: | |
| return | |
| accessy_member = accessy_session._get_org_user_from_phone(member.phone) | |
| actual_members = [accessy_member] if accessy_member is not None else [] | |
| else: | |
| # If a specific member is given, sync only that member, | |
| # otherwise sync all members | |
| if member_id is not None: | |
| member = db_session.query(Member).get(member_id) | |
| if member is None: | |
| logger.error(f"Cannot sync non-existent member {member_id}") | |
| raise ValueError(f"Member {member_id} does not exist") | |
| if member.phone is None: | |
| logger.info(f"Skipping sync for member {member_id} without phone number") | |
| return | |
| accessy_member = accessy_session._get_org_user_from_phone(member.phone) | |
| if accessy_member is None: | |
| logger.info(f"Member {member_id} not found in Accessy") | |
| actual_members = [accessy_member] if accessy_member is not None else [] | |
| else: |
| for accessy_member in diff.invites: | ||
| assert accessy_member.phone is not None | ||
| if accessy_member.phone in pending_invites: | ||
| logger.info(f"accessy sync skipping, invite already pending: {accessy_member}") | ||
| continue | ||
| logger.info(f"accessy sync inviting: {member}") | ||
| accessy_session.invite_phone_to_org_and_groups([member.phone], member.groups) | ||
| logger.info(f"accessy sync inviting: {accessy_member}") | ||
| accessy_session.invite_phone_to_org_and_groups([accessy_member.phone], accessy_member.groups) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add retry mechanism for API operations
The invite operation could fail due to network issues or API rate limits.
Consider adding a retry mechanism:
for accessy_member in diff.invites:
assert accessy_member.phone is not None
if accessy_member.phone in pending_invites:
logger.info(f"accessy sync skipping, invite already pending: {accessy_member}")
continue
- logger.info(f"accessy sync inviting: {accessy_member}")
- accessy_session.invite_phone_to_org_and_groups([accessy_member.phone], accessy_member.groups)
+ max_retries = 3
+ for attempt in range(max_retries):
+ try:
+ logger.info(f"accessy sync inviting: {accessy_member} (attempt {attempt + 1}/{max_retries})")
+ accessy_session.invite_phone_to_org_and_groups([accessy_member.phone], accessy_member.groups)
+ break
+ except Exception as e:
+ if attempt == max_retries - 1:
+ logger.error(f"Failed to invite {accessy_member} after {max_retries} attempts: {e}")
+ raise
+ logger.warning(f"Failed to invite {accessy_member} (attempt {attempt + 1}/{max_retries}): {e}")📝 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.
| for accessy_member in diff.invites: | |
| assert accessy_member.phone is not None | |
| if accessy_member.phone in pending_invites: | |
| logger.info(f"accessy sync skipping, invite already pending: {accessy_member}") | |
| continue | |
| logger.info(f"accessy sync inviting: {member}") | |
| accessy_session.invite_phone_to_org_and_groups([member.phone], member.groups) | |
| logger.info(f"accessy sync inviting: {accessy_member}") | |
| accessy_session.invite_phone_to_org_and_groups([accessy_member.phone], accessy_member.groups) | |
| for accessy_member in diff.invites: | |
| assert accessy_member.phone is not None | |
| if accessy_member.phone in pending_invites: | |
| logger.info(f"accessy sync skipping, invite already pending: {accessy_member}") | |
| continue | |
| max_retries = 3 | |
| for attempt in range(max_retries): | |
| try: | |
| logger.info(f"accessy sync inviting: {accessy_member} (attempt {attempt + 1}/{max_retries})") | |
| accessy_session.invite_phone_to_org_and_groups([accessy_member.phone], accessy_member.groups) | |
| break | |
| except Exception as e: | |
| if attempt == max_retries - 1: | |
| logger.error(f"Failed to invite {accessy_member} after {max_retries} attempts: {e}") | |
| raise | |
| logger.warning(f"Failed to invite {accessy_member} (attempt {attempt + 1}/{max_retries}): {e}") |
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Refactor
Tests