Add Scene.ApplyModifier #764
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the contact modification system by extracting the single-modifier application logic from BulkModify into a new Scene.ApplyModifier method. The changes enable applying individual modifiers to contacts while maintaining the existing bulk modification functionality.
- Renamed
ApplyModifierstoBulkModifyfor clearer naming - Extracted single modifier application logic into
Scene.ApplyModifiermethod - Updated all callers to use the new
BulkModifyfunction name
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| core/runner/scene.go | Adds new ApplyModifier method to Scene struct for applying single modifiers |
| core/runner/modifiers.go | Refactors bulk modification to use Scene.ApplyModifier and renames function |
| web/contact/modify.go | Updates function call from ApplyModifiers to BulkModify |
| web/contact/create.go | Updates function call from ApplyModifiers to BulkModify |
| core/runner/handlers/base_test.go | Updates test to use new BulkModify function name |
| core/imports/contacts.go | Updates function call from ApplyModifiers to BulkModify |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| events := make([]flows.Event, 0) | ||
| modifiers.Apply(eng, env, oa.SessionAssets(), s.Contact, mod, func(e flows.Event) { events = append(events, e) }) | ||
|
|
||
| // TODO limit user crediting to only the first event? We might have contact_groups_changed events here from changing contact fields etc. |
There was a problem hiding this comment.
This TODO comment appears to be copied from the original code but doesn't provide sufficient context about what 'user crediting' means or when this limitation should be implemented. Consider expanding this comment to explain the business logic behind user crediting and the specific scenarios where multiple events might cause issues.
| // TODO limit user crediting to only the first event? We might have contact_groups_changed events here from changing contact fields etc. | |
| // TODO: Currently, we credit the user (identified by userID) for all events generated by applying this modifier. | |
| // However, in some cases, a single modifier application can generate multiple events, such as contact_groups_changed events | |
| // that result from changes to contact fields. The business logic may require that user crediting (e.g., for audit trails or | |
| // attribution) should only apply to the primary event (typically the first event), to avoid attributing secondary or | |
| // system-generated events to the user. Review the requirements for user crediting and, if necessary, update this logic | |
| // to ensure that only the intended event(s) are attributed to the user, and that secondary events (like contact_groups_changed) | |
| // are not incorrectly credited. Clarify with product/business stakeholders if unsure. |
Copilot uses AI. Check for mistakes.
39cc8f2 to
1df9058
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #764 +/- ##
==========================================
- Coverage 51.04% 51.03% -0.01%
==========================================
Files 250 250
Lines 15110 15112 +2
==========================================
Hits 7713 7713
- Misses 6589 6591 +2
Partials 808 808 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
No description provided.