Skip to content

[fix]: Apply rules using trigger section/row period#185

Open
deeonwuli wants to merge 1 commit intodevelopmentfrom
fix/apply-rules-using-trigger-section-period
Open

[fix]: Apply rules using trigger section/row period#185
deeonwuli wants to merge 1 commit intodevelopmentfrom
fix/apply-rules-using-trigger-section-period

Conversation

@deeonwuli
Copy link
Copy Markdown
Contributor

📌 References

📝 Implementation

PR #177 made rule condition lookups always use the form's base period. That broke rules whose trigger DE lives in a section with a period offset: in PAHO country TUB sections 6.2 / 6.4 (startOffset/endOffset: -1) the trigger's value sits at the cohort year, not the form year, so the lookup found nothing and the dependent rows in 6.3 / 6.5 stayed disabled.

getApplicablePeriod now resolves the period from the row period when the trigger's section has data there (so multi-period sections evaluate per-row), otherwise the form's base period.

🎨 Screenshots

Screen.Recording.2026-04-30.at.14.20.18.mov

🔥 Notes to the tester

#869cmxcc8

@bundlemon
Copy link
Copy Markdown

bundlemon Bot commented Apr 30, 2026

BundleMon

No change in files bundle size

Groups updated (1)
Status Path Size Limits
Build Folder
./**/*
1.52MB (+98B +0.01%) +20%

Final result: ✅

View report in BundleMon website ➡️


Current branch size history | Target branch size history

Copy link
Copy Markdown
Contributor

@MatiasArriola MatiasArriola left a comment

Choose a reason for hiding this comment

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

Good refactor for getValueAndVerifyCondition to receive an options object instead a lot of positional parameters. Also, getApplicablePeriod is a nice and well-focused abstraction. The intent aligns well with the PR description.

Only some comments that might be worth reviewing:

  • getValueAndVerifyCondtion: We are using period instead of applicablePeriod in the evaluateTotalRule call. Is that correct? Maybe this is just a smell, but wanted to raise the question. I think we should confirm and test around this, and if no change is needed, add comment on why period is used.
  • FormRuleOptions type includes dataElementTotalRule: Maybe<DataElementTotalRule | SectionTotalRule>; but `SectionTotalRule seems to be not needed

Thanks @deeonwuli!

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.

2 participants