forgejo_admin/basketball-api
1
0
Fork 1
Code Issues 12 Pull requests 3 Projects Releases Packages Wiki Activity Actions

fix: set requires_approval=false on new GroupMe groups #195

Merged
forgejo_admin merged 2 commits from 159-fix-reconciliation-requires-approval into main 2026-03-27 21:25:49 +00:00
Conversation 1 Commits 2 Files changed 2 +169 -3
forgejo_admin commented 2026-03-27 21:18:55 +00:00
Owner
Copy link

Summary

  • Parents clicking GroupMe share links were stuck waiting for admin approval because the GroupMe API defaults requires_approval=true
  • The reconciliation script now calls update_group(requires_approval=False) after creation and passes the required nickname kwarg to add_member

Changes

  • scripts/create_groupme_groups.py: call client.update_group(group_id, requires_approval=False) after each create_group(); add nickname=name to every add_member() call; extract group_id to local variable
  • tests/test_reconciliation.py: 3 new tests verifying update_group is called with requires_approval=False, add_member includes nickname, and existing groups are skipped (idempotent)

Test Plan

  • Tests pass locally (pytest tests/ -- 596/596 pass)
  • pytest tests/test_reconciliation.py -k reconciliation -- 3/3 pass
  • ruff format + ruff check -- clean
  • Manual verification: run script against staging GroupMe groups

Review Checklist

  • Passed automated review-fix loop
  • No secrets committed
  • No unnecessary file changes
  • Commit messages are descriptive

Related Notes

  • Closes #159
  • project-groupme-westside -- the project this work belongs to
  • Parent issue: #156 (GroupMe data model + outbox)
## Summary - Parents clicking GroupMe share links were stuck waiting for admin approval because the GroupMe API defaults `requires_approval=true` - The reconciliation script now calls `update_group(requires_approval=False)` after creation and passes the required `nickname` kwarg to `add_member` ## Changes - `scripts/create_groupme_groups.py`: call `client.update_group(group_id, requires_approval=False)` after each `create_group()`; add `nickname=name` to every `add_member()` call; extract `group_id` to local variable - `tests/test_reconciliation.py`: 3 new tests verifying `update_group` is called with `requires_approval=False`, `add_member` includes `nickname`, and existing groups are skipped (idempotent) ## Test Plan - [x] Tests pass locally (`pytest tests/` -- 596/596 pass) - [x] `pytest tests/test_reconciliation.py -k reconciliation` -- 3/3 pass - [x] `ruff format` + `ruff check` -- clean - [ ] Manual verification: run script against staging GroupMe groups ## Review Checklist - [x] Passed automated review-fix loop - [x] No secrets committed - [x] No unnecessary file changes - [x] Commit messages are descriptive ## Related Notes - Closes #159 - `project-groupme-westside` -- the project this work belongs to - Parent issue: #156 (GroupMe data model + outbox)
fix: set requires_approval=false on new GroupMe groups + add nickname to add_member
Some checks failed
ci/woodpecker/pr/woodpecker Pipeline failed
Details
4f2fa1f5e9 
Parents clicking share links were stuck waiting for admin approval because
the GroupMe API defaults requires_approval=true. Now the reconciliation
script calls update_group(requires_approval=False) immediately after
creation, and passes the nickname kwarg to add_member as required by the SDK.

Closes #159

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
forgejo_admin commented 2026-03-27 21:19:36 +00:00
Author
Owner
Copy link

QA Review

Script changes (scripts/create_groupme_groups.py)

update_group call -- Correctly calls client.update_group(group_id, requires_approval=False) after creation. The SDK's update_group accepts **kwargs and POSTs to /groups/{id}/update, so requires_approval is forwarded correctly. Wrapped in try/except so a single failure doesn't abort the batch. Good.

add_member nickname fix -- The SDK signature requires nickname as the second parameter. The old code omitted it entirely, which would have raised TypeError at runtime. Now passes the person's name ("Lucas"/"Marcus"). Correct.

group_id local variable -- Clean extraction avoids repeated str() calls. No behavioral change.

Test changes (tests/test_reconciliation.py)

  • 3 tests covering all acceptance criteria from #159
  • Mocking strategy is correct: patches groupme_sdk.GroupMeClient at module level since the script imports it locally inside main()
  • Fixtures use the existing db/tenant conftest fixtures properly

Nit (non-blocking)

  • _run_script() helper function (lines 57-75) is defined but never called -- all three tests inline the same setup. Could be removed or the tests could be refactored to use it. Not a blocker.

Verdict

APPROVE -- All acceptance criteria met. No secrets, no unrelated changes, tests pass (596/596).

## QA Review ### Script changes (`scripts/create_groupme_groups.py`) **update_group call** -- Correctly calls `client.update_group(group_id, requires_approval=False)` after creation. The SDK's `update_group` accepts `**kwargs` and POSTs to `/groups/{id}/update`, so `requires_approval` is forwarded correctly. Wrapped in try/except so a single failure doesn't abort the batch. Good. **add_member nickname fix** -- The SDK signature requires `nickname` as the second parameter. The old code omitted it entirely, which would have raised `TypeError` at runtime. Now passes the person's name ("Lucas"/"Marcus"). Correct. **group_id local variable** -- Clean extraction avoids repeated `str()` calls. No behavioral change. ### Test changes (`tests/test_reconciliation.py`) - 3 tests covering all acceptance criteria from #159 - Mocking strategy is correct: patches `groupme_sdk.GroupMeClient` at module level since the script imports it locally inside `main()` - Fixtures use the existing `db`/`tenant` conftest fixtures properly ### Nit (non-blocking) - `_run_script()` helper function (lines 57-75) is defined but never called -- all three tests inline the same setup. Could be removed or the tests could be refactored to use it. Not a blocker. ### Verdict **APPROVE** -- All acceptance criteria met. No secrets, no unrelated changes, tests pass (596/596).
chore: remove unused _run_script helper from reconciliation tests
All checks were successful
ci/woodpecker/pr/woodpecker Pipeline was successful
Details
344afc52de 
QA review nit -- the helper was defined but never called.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
forgejo_admin deleted branch 159-fix-reconciliation-requires-approval 2026-03-27 21:25:49 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
domain:backend

   
domain:devops

   
domain:frontend

   
status:approved

QA passed, awaiting merge approval

  
status:in-progress

Dev agent is actively working

  
status:needs-fix

QA found issues, back to dev

  
status:qa

PR submitted, awaiting QA review

  
type:bug

Bug fix

  
type:devops

Infrastructure/CI/config work

  
type:feature

New functionality

No labels
domain:backend
domain:devops
domain:frontend
status:approved
status:in-progress
status:needs-fix
status:qa
type:bug
type:devops
type:feature
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
forgejo_admin/basketball-api!195
No description provided.