Sync OWASP Board of Directors Members Data #2345
Conversation
Summary by CodeRabbit
WalkthroughAdds a new Django management command to sync OWASP Board of Directors members for a specified year from a remote YAML source, creating/updating EntityMember records linked to a BoardOfDirectors instance, with fuzzy GitHub user matching, transactional processing, progress logging, error handling, and comprehensive tests. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests
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 |
|
Hy @arkid15r Sorry for begin late due to my college exam i am not able to Contribute. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/apps/owasp/management/commands/owasp_sync_board_members.py(1 hunks)backend/tests/apps/owasp/management/commands/owasp_sync_board_members_test.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
backend/apps/owasp/management/commands/owasp_sync_board_members.py (2)
backend/apps/owasp/models/board_of_directors.py (1)
BoardOfDirectors(6-19)backend/apps/owasp/models/entity_member.py (2)
EntityMember(10-112)Role(13-16)
backend/tests/apps/owasp/management/commands/owasp_sync_board_members_test.py (3)
backend/apps/owasp/management/commands/owasp_sync_board_members.py (6)
Command(23-178)handle(37-79)_extract_board_members_for_year(81-95)_create_or_update_member(97-125)_fuzzy_match_github_user(127-178)add_arguments(28-35)backend/apps/owasp/models/board_of_directors.py (1)
BoardOfDirectors(6-19)backend/apps/owasp/models/entity_member.py (2)
EntityMember(10-112)Role(13-16)
🪛 Ruff (0.13.2)
backend/apps/owasp/management/commands/owasp_sync_board_members.py
4-4: typing.Dict is deprecated, use dict instead
(UP035)
4-4: typing.List is deprecated, use list instead
(UP035)
43-43: Probable use of requests call without timeout
(S113)
48-48: Use explicit conversion flag
Replace with conversion flag
(RUF010)
95-95: Unnecessary assignment to members before return statement
Remove unnecessary assignment
(RET504)
122-122: Local variable entity_member is assigned to but never used
Remove assignment to unused variable entity_member
(F841)
124-124: Do not catch blind exception: Exception
(BLE001)
125-125: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
150-150: Magic value used in comparison, consider replacing 100 with a constant variable
(PLR2004)
156-156: Variable SIMILARITY_THRESHOLD in function should be lowercase
(N806)
174-174: Consider moving this statement to an else block
(TRY300)
176-176: Do not catch blind exception: Exception
(BLE001)
backend/tests/apps/owasp/management/commands/owasp_sync_board_members_test.py
467-467: No newline at end of file
Add trailing newline
(W292)
backend/apps/owasp/management/commands/owasp_sync_board_members.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
backend/apps/owasp/management/commands/owasp_sync_board_members.py (5)
4-4: Use modern type hints.Python 3.9+ supports
dict,list, andOptional(orX | None) directly without importing fromtyping. Modernizing these imports improves readability and aligns with current Python best practices.Apply this diff:
-from typing import Any, Dict, List, Optional +from typing import AnyThen update the type hints throughout the file:
Dict[str, Any]→dict[str, Any]List[Dict[str, str]]→list[dict[str, str]]User | Noneis already using modern syntax (no change needed)
46-50: Optional: Simplify exception formatting.The f-string can directly interpolate the exception without explicit
str()conversion.Apply this diff:
except (requests.RequestException, yaml.YAMLError) as e: self.stderr.write( - self.style.ERROR(f"Failed to fetch or parse board history: {str(e)}") + self.style.ERROR(f"Failed to fetch or parse board history: {e}") )
81-95: Optional: Return directly.The intermediate
membersvariable can be eliminated.Apply this diff:
def _extract_board_members_for_year( self, board_data: List[Dict[str, Any]], year: int ) -> List[Dict[str, str]]: """Extract board members for a specific year from board history data.""" - # Find the entry for the specified year year_entry = next( (entry for entry in board_data if entry.get("year") == year), None ) if not year_entry: return [] - members = year_entry.get("members", []) - return members + return year_entry.get("members", [])
132-133: Uselogger.exceptionfor better debugging.When logging errors in an exception handler,
logger.exceptionautomatically includes the stack trace, making it much easier to diagnose issues.Apply this diff:
except Exception as e: - logger.error("Failed to process board member %s: %s", member_name, str(e)) + logger.exception("Failed to process board member %s", member_name)
158-164: Extract magic numbers to module-level constants.The candidate limit (100) and similarity threshold (80) should be defined as module-level constants for better maintainability and to follow naming conventions.
Add these constants near the top of the file (after line 20):
BOARD_HISTORY_URL = "https://raw.githubusercontent.com/OWASP/www-board/master/_data/board-history.yml" FUZZY_MATCH_CANDIDATE_LIMIT = 100 FUZZY_MATCH_SIMILARITY_THRESHOLD = 80Then update the function:
- if candidate_users.count() > 100: + if candidate_users.count() > FUZZY_MATCH_CANDIDATE_LIMIT: candidate_users = candidate_users.filter( models.Q(name__istartswith=first_name + " ") | models.Q(name__icontains=f" {first_name} ") ) - SIMILARITY_THRESHOLD = 80 potential_matches = [] for user in candidate_users: similarity = fuzz.ratio(member_name.lower(), user.name.lower()) - if similarity >= SIMILARITY_THRESHOLD: + if similarity >= FUZZY_MATCH_SIMILARITY_THRESHOLD: potential_matches.append((user, similarity))backend/tests/apps/owasp/management/commands/owasp_sync_board_members_test.py (3)
85-91: Prefix unused loop variable with underscore.The loop variable
iis not used within the loop body.Apply this diff:
mock_entity_members = [] - for i in range(3): + for _ in range(3): mock_entity_member = mock.Mock(spec=EntityMember) mock_entity_member.is_active = False mock_entity_member.member_id = None mock_entity_member.save = mock.Mock() mock_entity_members.append(mock_entity_member)
137-142: Prefix unused unpacked variable.The unpacked
argsvariable is not used.Apply this diff:
for mock_entity_member in mock_entity_members: mock_entity_member.save.assert_called_once() call_args = mock_entity_member.save.call_args - args, kwargs = call_args + _args, kwargs = call_args assert 'update_fields' in kwargs assert 'is_active' in kwargs['update_fields']
457-457: Add trailing newline.Python files should end with a newline character per PEP 8.
Add a blank line at the end of the file after line 457.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/apps/owasp/management/commands/owasp_sync_board_members.py(1 hunks)backend/tests/apps/owasp/management/commands/owasp_sync_board_members_test.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
backend/apps/owasp/management/commands/owasp_sync_board_members.py (3)
backend/apps/github/api/internal/queries/user.py (1)
user(40-53)backend/apps/owasp/models/board_of_directors.py (1)
BoardOfDirectors(6-19)backend/apps/owasp/models/entity_member.py (2)
EntityMember(10-112)Role(13-16)
backend/tests/apps/owasp/management/commands/owasp_sync_board_members_test.py (3)
backend/apps/owasp/management/commands/owasp_sync_board_members.py (6)
Command(23-186)handle(37-79)_extract_board_members_for_year(81-95)_create_or_update_member(97-133)_fuzzy_match_github_user(135-186)add_arguments(28-35)backend/apps/owasp/models/board_of_directors.py (1)
BoardOfDirectors(6-19)backend/apps/owasp/models/entity_member.py (2)
EntityMember(10-112)Role(13-16)
🪛 Ruff (0.13.2)
backend/apps/owasp/management/commands/owasp_sync_board_members.py
4-4: typing.Dict is deprecated, use dict instead
(UP035)
4-4: typing.List is deprecated, use list instead
(UP035)
48-48: Use explicit conversion flag
Replace with conversion flag
(RUF010)
95-95: Unnecessary assignment to members before return statement
Remove unnecessary assignment
(RET504)
132-132: Do not catch blind exception: Exception
(BLE001)
133-133: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
158-158: Magic value used in comparison, consider replacing 100 with a constant variable
(PLR2004)
164-164: Variable SIMILARITY_THRESHOLD in function should be lowercase
(N806)
182-182: Consider moving this statement to an else block
(TRY300)
184-184: Do not catch blind exception: Exception
(BLE001)
backend/tests/apps/owasp/management/commands/owasp_sync_board_members_test.py
86-86: Loop control variable i not used within loop body
Rename unused i to _i
(B007)
140-140: Unpacked variable args is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
457-457: No newline at end of file
Add trailing newline
(W292)
|
2 participants
Resolves #2177
Description
This PR implements the functionality to sync OWASP Board of Directors members for a given election year by parsing the board-history.yml file from the www-board repository
Checklist
make check-testlocally; all checks and tests passed.