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: [DHIS2-12544] Add verbose logging to rules engine #3480

Merged
merged 11 commits into from
Dec 18, 2023

Conversation

eirikhaugstulen
Copy link
Contributor

@eirikhaugstulen eirikhaugstulen commented Dec 6, 2023

Tech-summary:

  • Added an option to enable verbose=true in the url
  • Pass a onVerboseLog callback to the rules engine
  • Add optional flags to the RulesEngine.js-class

@eirikhaugstulen eirikhaugstulen marked this pull request as ready for review December 6, 2023 13:11
@eirikhaugstulen eirikhaugstulen requested a review from a team as a code owner December 6, 2023 13:11
Copy link
Contributor

@simonadomnisoru simonadomnisoru left a comment

Choose a reason for hiding this comment

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

Hey @eirikhaugstulen,

Can you add some unit tests for the new flag logic you added in RulesEngine.js and runExpression.js? Ideally, we should aim to keep the same coverage percentage and test all new features.

Thank you!

@eirikhaugstulen
Copy link
Contributor Author

Sure thing, @simonadomnisoru! Thanks for pointing that out 👍

Copy link

github-actions bot commented Dec 9, 2023

</React.Fragment>
);
export const App = ({ store }: Props) => {
useRuleEngineFlags();
Copy link
Member

Choose a reason for hiding this comment

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

As far as I can tell, this will cause the App component to re-render every time the url changes. Consequently, all children will render unless we have a PureComponent in the chain. I think it would be beneficial to use a PureComponent as the child where we use this hook (For a function component: Wrap it in a Memo).

Comment on lines 16 to 22
useEffect(() => {
if (verbose === 'true') {
updateFlags({ verbose: true });
} else {
updateFlags({ verbose: false });
}
}, [verbose]);
Copy link
Member

Choose a reason for hiding this comment

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

With this approach we can in theory run the rules engine before the verbose flag has been set, because an useEffect in an inner component will run before this one (in practice it will probably work because we use async functions). Did you put this in an useEffect because side-effects should not be part of the rendering, or because some part of react-router-dom is not fully initialised in the render? The preferred solution here might be to use useLayoutEffect (as that one is fired before useLayout)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No special thoughts behind it, just choosing useEffect over useLayoutEffect by default. Switched now 👍

Copy link

@geethaalwan geethaalwan left a comment

Choose a reason for hiding this comment

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

Tested successfully on 2.41,2.40.3,2.39.5,2.38.6 versions

Copy link
Member

@JoakimSM JoakimSM left a comment

Choose a reason for hiding this comment

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

🎉 Nice!

@eirikhaugstulen eirikhaugstulen merged commit 2a6d4a8 into master Dec 18, 2023
35 of 37 checks passed
@eirikhaugstulen eirikhaugstulen deleted the eh/feat/DHIS2-12544_VerboseRulesEngine branch December 18, 2023 13:32
dhis2-bot added a commit that referenced this pull request Dec 18, 2023
# [100.49.0](v100.48.0...v100.49.0) (2023-12-18)

### Features

* [DHIS2-12544] Add verbose logging to rules engine ([#3480](#3480)) ([2a6d4a8](2a6d4a8))
@dhis2-bot
Copy link
Contributor

🎉 This PR is included in version 100.49.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants