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

M2-7011: feat conditional logic extend item type #883

Open
wants to merge 14 commits into
base: dev
Choose a base branch
from

Conversation

felipeMetaLab
Copy link
Contributor

@felipeMetaLab felipeMetaLab commented Oct 29, 2024

📝 Description

🔗 Jira Ticket M2-7011
This PR is focused to extend and create item types and conditional logic.
here is described the rules it must to follow:
https://mindlogger.atlassian.net/wiki/spaces/MINDLOGGER1/pages/596410369/Item+Flow+-+Extend+the+list+of+supported+Item+Types

📸 Screenshots

Screenshot 2024-10-27 at 9 28 09 PM

Screen.Recording.2024-10-28.at.2.02.27.PM.mov
Screen.Recording.2024-10-27.at.9.29.36.PM.mov

🪤 Peer Testing

1 - Launch admin app, and create a new applet or open an existed one.
2 - go to the items and create the items you would like to test, example, Date, Message, Text ...
3 - click on "item flow" tab and create your conditional logic, example:
Screenshot 2024-10-27 at 9 28 09 PM
4 - launch the app and choose the applet you created.
5 - check if the logic is working, I left a video sample to demonstrate one of the logics.

✏️ Notes

For it works on development it depends of the backend, the backend branch I am testing is: feature/M2-7011_extend_the_list_of_supported_item_types.

Somehow when you create a condition on admin side, the date looks little different, so its super important makes a validation based on what was inserted on API
Screenshot 2024-11-11 at 12 09 17 AM

@felipeMetaLab felipeMetaLab changed the title feature / conditional logic extend item teype M2-7011: feat conditional logic extend item type Oct 30, 2024
Copy link
Member

@andrevitalb andrevitalb left a comment

Choose a reason for hiding this comment

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

Some minor nit changes, but good overall 🤓. Will do a local test run of all logical conditions. Ok to pre-approve

@@ -15,10 +15,130 @@ type Condition =
| EqualCondition
| NotEqualCondition
| BetweenCondition
| OutsideOfCondition;
| OutsideOfCondition
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Could we keep these and the individual types in both this file & ActivityItemDto.ts in the same order? In order to improve readability

Comment on lines 71 to 72
// @ts-expect-error
delete updatedCondition.itemName;
Copy link
Member

Choose a reason for hiding this comment

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

With the de-structuring done above, these two lines can be deleted.

// @ts-expect-error
delete updatedCondition.itemName;
switch (condition.type) {
Copy link
Member

Choose a reason for hiding this comment

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

Some cases are missing from this one, that aren't straightforward

payload: {
  value: number;
};

General conditions
case 'BETWEEN':
case 'OUTSIDE_OF':

Row option conditions
case 'EQUAL_TO_ROW_OPTION':
case 'NOT_EQUAL_TO_ROW_OPTION':

Slider row conditions
case 'GREATER_THAN_SLIDER_ROWS':
case 'LESS_THAN_SLIDER_ROWS':
case 'EQUAL_TO_SLIDER_ROWS':
case 'NOT_EQUAL_TO_SLIDER_ROWS':
case 'BETWEEN_SLIDER_ROWS':
case 'OUTSIDE_OF_SLIDER_ROWS':

Should those be added?

break;

case 'BETWEEN_DATES':
case 'OUTSIDE_OF_DATES':
Copy link
Member

Choose a reason for hiding this comment

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

The cases 'BETWEEN_TIMES', 'OUTSIDE_OF_TIMES', 'BETWEEN_TIME_RANGES' and 'OUTSIDE_OF_TIME_RANGES' can be grouped with these two, all are passing both minDate and maxDate.

This could be left as is to preserve ordering. Order for this switch can be replicated from the actual Condition type


export const isEqualToDate = (input: Maybe<string>, date: string) => {
if (!input) return false;
return input ? isEqual(parseISO(input), parseISO(date)) : false;
Copy link
Member

Choose a reason for hiding this comment

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

Since we're already validating if input exists, this can be updated to just:

Suggested change
return input ? isEqual(parseISO(input), parseISO(date)) : false;
return isEqual(parseISO(input), parseISO(date));

Comment on lines +66 to +70
function isValidTimeFormat(time: any): boolean {
return (
time && typeof time.hours === 'number' && typeof time.minutes === 'number'
);
}
Copy link
Member

Choose a reason for hiding this comment

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

Are we able to narrow the types time can have? For example, if it's either the expected  { hours: number, minutes: number } or undefined? Maybe replace undefined with null, or even include both, if that's the case. Just to stay on the type-safe side of things

Suggested change
function isValidTimeFormat(time: any): boolean {
return (
time && typeof time.hours === 'number' && typeof time.minutes === 'number'
);
}
function isValidTimeFormat(
time:
| {
hours: number;
minutes: number;
}
| undefined,
): boolean {
return (
!!time &&
typeof time.hours === 'number' &&
typeof time.minutes === 'number'
);
}

return null;
};

const timeToMinutes = (time: { hours: number; minutes: number }): number => {
Copy link
Member

Choose a reason for hiding this comment

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

Ditto as for getTimeBasedOnFieldName and convertToMinutes

return isEqualToValue(selectedOption, optionValue);
},

isGreaterThen(value: number) {
isGreaterThan(value: number) {
Copy link
Member

Choose a reason for hiding this comment

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

Nice catch! Makes me wonder, how were these working properly 🤔

Comment on lines +14 to +18
function isValidTimeFormat(time: any): boolean {
return (
time && typeof time.hours === 'number' && typeof time.minutes === 'number'
);
}
Copy link
Member

Choose a reason for hiding this comment

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

Ditto as for AnswerValidator.ts

@@ -45,7 +45,127 @@ type ConditionDto =
| EqualConditionDto
| NotEqualConditionDto
| BetweenConditionDto
| OutsideOfConditionDto;
| OutsideOfConditionDto
Copy link
Member

Choose a reason for hiding this comment

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

Ditto on the ordering

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