Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(plan-charges-cascading): Support charge filters cascading #2761

Merged
merged 7 commits into from
Nov 13, 2024

Conversation

lovrocolic
Copy link
Collaborator

@lovrocolic lovrocolic commented Oct 31, 2024

Context

Currently when plan is updated, these changes are not cascaded to all the children plans.

Description

This PR supports cascading of charge filters. There are several rules:

  • If main attribute that supports cascading is not true -> cascading is disabled
  • Charge filter properties are cascaded only if the same properties are not already overridden in the child filter
  • We need to ensure that all filters that are specific to the child are not changed in any way (extra filters that don't exist on parent)
  • When performing cascading, we are basically at the same time updating parent filters and children filters. However, we need to ensure that logic which handles cascading on children filter uses parent filter attributes before update for comparison. That's why we pass parent filter attributes before any update to the children logic.

Copy link
Collaborator

@vincent-pochet vincent-pochet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two comments, looks good otherwise 🚀

app/services/plans/update_service.rb Outdated Show resolved Hide resolved
@annvelents annvelents merged commit 5d506a3 into main Nov 13, 2024
6 checks passed
@annvelents annvelents deleted the feat-filters-cascade-1 branch November 13, 2024 10:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants