Conversation
|
New Issues (3)Checkmarx found the following issues in this Pull Request
Fixed Issues (4)Great job! The following issues were fixed in this Pull Request
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7159 +/- ##
=======================================
Coverage 56.88% 56.88%
=======================================
Files 2028 2028
Lines 88830 88830
Branches 7918 7918
=======================================
+ Hits 50532 50533 +1
+ Misses 36468 36467 -1
Partials 1830 1830 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
eliykat
left a comment
eliykat
left a comment
There was a problem hiding this comment.
Thanks for coming back to this!
| ## Limitations | ||
|
|
||
| 1. We don't have a way to keep this whole process idempotent, so if there is an exception at any point that is not being handled, the state will stay where the process failed. | ||
|
|
||
|
|
||
|
|
There was a problem hiding this comment.
Section appears to be unfinished? Also, this is a good callout, but I think it's more about it not being atomic than idempotent. A failure does not roll back any changes already made.
There was a problem hiding this comment.
Good catch. Also, this is completed. This is the only issue I'm aware of. I'm happy to add more if you know of anything else. I used a bullet list so we can easily add new ones.
There was a problem hiding this comment.
I think it should only be a list if there is more than 1 item. For now it should just be a single paragraph.
…teEvents/README.md Co-authored-by: Thomas Rittson <31796059+eliykat@users.noreply.github.com>
…teEvents/README.md Co-authored-by: Thomas Rittson <31796059+eliykat@users.noreply.github.com>
…teEvents/README.md Co-authored-by: Thomas Rittson <31796059+eliykat@users.noreply.github.com>
…teEvents/README.md Co-authored-by: Thomas Rittson <31796059+eliykat@users.noreply.github.com>
|
| When an organization policy is created or updated, the save workflow runs a series of ordered steps. A policy handler can hook into any step by implementing the corresponding Policy Update Event interface. | ||
|
|
||
| Note: If you don’t want to hook into these events, you don’t need to create a handler, and your policy will simply upsert to the database with log events. | ||
| Note: If you do not need to hook into any step, you do not need to create a policy handler. The policy will simply upsert to the database with an audit log event. |
There was a problem hiding this comment.
Non-blocking: I misspoke before. Contractions (e.g. "don't") are OK, abbreviations (e.g. "org" instead of "organization") should be minimized.
|
|
||
| Typical uses: creating collections, sending notifications that depend on the new policy state. | ||
|
|
||
| Note: This is more useful for enabling a policy than for disabling a policy, since when the policy is disabled, there is no easy way to find the users the policy should be enforced on. |
There was a problem hiding this comment.
OK good point, I see now. Maybe something like this?
Note: This is more useful for enabling a policy than for disabling a policy, since when the policy is disabled, it does not have any effect.
| ## Limitations | ||
|
|
||
| 1. We don't have a way to keep this whole process idempotent, so if there is an exception at any point that is not being handled, the state will stay where the process failed. | ||
|
|
||
|
|
||
|
|
There was a problem hiding this comment.
I think it should only be a list if there is more than 1 item. For now it should just be a single paragraph.
|
|
||
| ### `IOnPolicyPreUpdateEvent` | ||
|
|
||
| Executes side effects **before** the policy is upserted to the database. |
There was a problem hiding this comment.
It's the expected behaviour for normal imperative code flow, but here we've built a little system to handle this process, so it's not necessarily obvious how we've designed the error handling. This is alerting the dev to the fact that we will not handle their errors for them, so if they want the update to continue they would have to do that themselves. Or alternatively, it gives them a way of aborting the operation.
|
My comments above are mostly replying to threads inline, you may have to scroll up for proper context. |
eliykat
added
ai-review-vnext
2 participants
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-29129
📔 Objective
📸 Screenshots
No code changes, just a markdown file.