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

refactor(logic): consolidate logic functions into shared #7136

Merged
merged 5 commits into from
Mar 19, 2024

Conversation

justynoh
Copy link
Contributor

Problem

There is currently some tech debt incurred as a result of the angular->react migration that has not yet been cleaned up. In particular, the functions used to evaluate logic have not been refactored from src to shared, because the client-side types for input data differs between angular and react.

As a result, for a period of time, the logic for logic (ha!) had to work over three schemas if they were truly to be shared - server, angular and react. Instead of making this work, we basically duplicated the logic and made the types work for the frontend, while retaining the original backend code in src.

Now that angular is gone, however, it's time to make this code truly shared.

Solution

This PR moves src/shared/utils/logic into shared/utils/logic and adjusts the types to account for existing behavior both on the frontend and backend.

There are no behavioral changes from the current behavior, and we should not expect any cutover period due to desynced fe/be.

Breaking Changes

  • No - this PR is backwards compatible

Tests

All tests are regression tests.

  • Show fields logic
    • Show fields based on "greater than or equal" to a number/decimal field.
    • Show fields based on "less than or equal" to a number/decimal field.
    • Show fields based on "equal" to a decimal field.
    • Show fields based on "either" for a radio field.
    • Show fields based on "either" for a radio field with "Others" as an option.
  • Prevent submit logic: repeat the same tests as above, but preventing submit instead of show fields.

@justynoh
Copy link
Contributor Author

Also, this was a side quest as a result of tackling backend validation for MRF, which requires these logic functions to work well.

return { _id, fieldType: fieldTypeAndInput.fieldType }
} else if (isLogicableField(fieldTypeAndInput)) {
return { _id, ...fieldTypeAndInput }
} else {
Copy link
Contributor

Choose a reason for hiding this comment

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

this looks iffy... but I can't think of a better solution

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the typechecker needs a lot of help in this case. like you said, I don't really have a good way of doing this other than just guaranteeing at runtime that there is no uncaught case (i.e. guaranteed only by the fact that isNotX(args) is defined to be !isX(args)).

I suppose an alternative is just to like. do a single if-else, and then //@ts-ignore the rest but that feels even more hackish. Having a typeguard, even if self-defined and somewhat weird, is better than having no typeguard at all. I think I might just leave a comment explaining why it's a bit complicated in this case, and that we would rather have the types than not even if it results in strange unreachable code paths that aren't obvious to the typechecker.

frontend/src/features/logic/utils/logic-adaptor.ts Outdated Show resolved Hide resolved
frontend/src/features/logic/utils/logic-adaptor.ts Outdated Show resolved Hide resolved
shared/utils/logic.ts Show resolved Hide resolved
shared/utils/logic.ts Outdated Show resolved Hide resolved
shared/utils/logic.ts Show resolved Hide resolved
shared/utils/logic.ts Show resolved Hide resolved
@justynoh justynoh requested a review from KenLSM March 18, 2024 15:26
Copy link
Contributor

@KenLSM KenLSM left a comment

Choose a reason for hiding this comment

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

LGTM! Yay for cleanups

@justynoh justynoh merged commit 520c5bf into develop Mar 19, 2024
24 checks passed
@justynoh justynoh deleted the refactor/move-logic-to-shared branch March 19, 2024 03:44
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.

None yet

2 participants