Add Workflows Engineer to invoice privileged roles#300
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 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)
✏️ 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.
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
Description
Add
Workflows Engineerto_PRIVILEGED_ROLESin the invoice validation cog so WorkflowsEngineers get full invoice access without being restricted to their own project roster.
Follow-up from michaelmwu's suggestion on #290 .
How Has This Been Tested?
Existing unit tests in
tests/unit/test_invoices_cog.pycover the privileged role accesspath — all 14 tests pass.
Summary by CodeRabbit