Clarify invoice privileged role messaging#300
Conversation
📝 WalkthroughWalkthroughThis PR adds "Workflows Engineer" to the privileged roles allowed to validate invoices and updates the denial message and unit tests to reference the privileged-roles list. ChangesInvoice Authorization
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/discord_bot/src/five08/discord_bot/cogs/invoices.py (1)
117-123:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUser-facing denial message is now out of sync with
_PRIVILEGED_ROLES.The error message still enumerates only "Steering Committee members or confirmed ERP project members." With Workflows Engineer added to
_PRIVILEGED_ROLES, a Workflows Engineer denied access (e.g., due to role hierarchy mismatch) would see a message that doesn't mention their role, which is confusing. Consider updating the message to reflect the new privileged role set, or generalize the wording.📝 Proposed wording update
if not include_all and not emails: await interaction.followup.send( - "Invoice validation is available to Steering Committee members " - "or confirmed ERP project members.", + "Invoice validation is available to Steering Committee or " + "Workflows Engineer members, or confirmed ERP project members.", ephemeral=True, )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/discord_bot/src/five08/discord_bot/cogs/invoices.py` around lines 117 - 123, The user-facing denial string in the invoices validation block (where include_all and emails are checked before calling interaction.followup.send) is stale relative to the _PRIVILEGED_ROLES set; update the message returned by interaction.followup.send to either enumerate the current privileged roles (including "Workflows Engineer") or use a generalized phrase like "privileged roles (e.g., Steering Committee, confirmed ERP project members, Workflows Engineers)" or "authorized project/committee members" so it aligns with _PRIVILEGED_ROLES and avoids confusion when a Workflows Engineer is denied.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@apps/discord_bot/src/five08/discord_bot/cogs/invoices.py`:
- Around line 117-123: The user-facing denial string in the invoices validation
block (where include_all and emails are checked before calling
interaction.followup.send) is stale relative to the _PRIVILEGED_ROLES set;
update the message returned by interaction.followup.send to either enumerate the
current privileged roles (including "Workflows Engineer") or use a generalized
phrase like "privileged roles (e.g., Steering Committee, confirmed ERP project
members, Workflows Engineers)" or "authorized project/committee members" so it
aligns with _PRIVILEGED_ROLES and avoids confusion when a Workflows Engineer is
denied.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dc116aff-1522-4b94-8b56-d4d9d528a183
📒 Files selected for processing (1)
apps/discord_bot/src/five08/discord_bot/cogs/invoices.py
lairwaves
force-pushed
the
feat/invoices-workflows-engineer-role
branch
from
8ea51e2 to
52a2fee
Compare
There was a problem hiding this comment.
Pull request overview
Expands invoice-validation authorization in the Discord bot so the Workflows Engineer role is treated as privileged for viewing/validating invoices.
Changes:
- Adds
Workflows Engineerto the invoice cog’s_PRIVILEGED_ROLESallowlist. - Updates the “no ERP identity” denial message and adjusts the corresponding unit test assertion.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| apps/discord_bot/src/five08/discord_bot/cogs/invoices.py | Grants privileged invoice access to Workflows Engineer and updates the user-facing denial message. |
| tests/unit/test_invoices_cog.py | Updates unit test expectation to match the revised denial message. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/unit/test_invoices_cog.py (1)
157-157: ⚡ Quick winConsider strengthening the assertion to verify all privileged roles appear.
The current assertion checks that at least one privileged role appears in the denial message. To more precisely verify that the message includes all configured privileged roles, consider:
✨ More precise assertion
- assert any(role in sent for role in _PRIVILEGED_ROLES) + for role in _PRIVILEGED_ROLES: + assert role in sent🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/test_invoices_cog.py` at line 157, The test currently asserts that any privileged role appears in the denial message; update the assertion to require that all privileged roles are present by checking every member of _PRIVILEGED_ROLES against the message (use sent and _PRIVILEGED_ROLES to locate the check), e.g. replace the any(...) check with an all(...) membership assertion or an explicit subset check to ensure every role in _PRIVILEGED_ROLES is contained in sent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/unit/test_invoices_cog.py`:
- Line 157: The test currently asserts that any privileged role appears in the
denial message; update the assertion to require that all privileged roles are
present by checking every member of _PRIVILEGED_ROLES against the message (use
sent and _PRIVILEGED_ROLES to locate the check), e.g. replace the any(...) check
with an all(...) membership assertion or an explicit subset check to ensure
every role in _PRIVILEGED_ROLES is contained in sent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cd23d05a-c4c9-4b16-a1f5-46383a61cfd7
📒 Files selected for processing (2)
apps/discord_bot/src/five08/discord_bot/cogs/invoices.pytests/unit/test_invoices_cog.py
- Build the access-denied message dynamically from _PRIVILEGED_ROLES so role names are always in sync without manual updates - Update the header comment to reference _PRIVILEGED_ROLES instead of hard-coding "Steering Committee" - Import _PRIVILEGED_ROLES in tests and assert against it so future role additions are covered automatically - Add Workflows Engineer fixture and access test
lairwaves
force-pushed
the
feat/invoices-workflows-engineer-role
branch
from
cc34a46 to
866130f
Compare
michaelmwu
changed the title
Description
Keep invoice full access anchored to Steering Committee-level access.
Workflows Engineeralready satisfies that through the shared Discord role hierarchy, so this PR avoids treating it as a separate invoice-specific role requirement.Update the invoice denial copy to list both
Steering CommitteeandWorkflows Engineer, and add invoice-level regression coverage thatWorkflows Engineergets full access without an ERP identity lookup.How Has This Been Tested?
uv run pytest tests/unit/test_invoices_cog.py tests/unit/test_role_decorators.py::TestHierarchicalRoles::test_check_user_roles_with_hierarchy_workflows_engineer_grants_steering_committee_access -qruffpassedruff formatpassedmypypassed once, but rewroteuv.lockdue the globaluvexclude-newer setting; I reverted that lockfile churn and retried the commit withSKIP=mypy