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(#8633) accept array on schedule.start_from #8639

Merged
merged 7 commits into from
Oct 25, 2023

Conversation

1yuv
Copy link
Member

@1yuv 1yuv commented Oct 13, 2023

Description

Allow start_from on schedule to be configured using arrays as well for fixing #8633

Code review checklist

  • Readable: Concise, well named, follows the style guide, documented if necessary.
  • Documented: Configuration and user documentation on cht-docs
  • Tested: Unit and/or e2e where appropriate
  • Internationalised: All user facing text
  • Backwards compatible: Works with existing data and configuration or includes a migration. Any breaking changes documented in the release notes.

Compose URLs

If Build CI hasn't passed, these may 404:

  • CHT_CORE_COMPOSE_URL
  • COUCH_SINGLE_COMPOSE_URL
  • COUCH_CLUSTER_COMPOSE_URL

License

The software is provided under AGPL-3.0. Contributions to this project are accepted under the same license.

Copy link
Member

@dianabarsan dianabarsan left a comment

Choose a reason for hiding this comment

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

Great! Can you also please add an e2e test that has a schedule with multiple fields?

shared-libs/transitions/src/lib/schedules.js Show resolved Hide resolved
Copy link
Contributor

@garethbowen garethbowen left a comment

Choose a reason for hiding this comment

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

Awesome!

I think there are other ways to do it but this is a nice simple solution that solves the problem today. If you can add the e2e test I think we can call this done.

Copy link
Contributor

@garethbowen garethbowen left a comment

Choose a reason for hiding this comment

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

A couple of nitpicks inline but I thin we're basically there. Thanks!

shared-libs/transitions/src/lib/schedules.js Outdated Show resolved Hide resolved
shared-libs/transitions/test/integration/schedules.js Outdated Show resolved Hide resolved
Copy link
Member

@dianabarsan dianabarsan left a comment

Choose a reason for hiding this comment

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

Great work, loving the e2e test, thanks a lot for adding it. I just have one small request inline.

chai.expect(updWithClinic.scheduled_tasks[2]).to.deep.nested.include(expectedMessage4('scheduled'));
});
});
});
Copy link
Member

Choose a reason for hiding this comment

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

I think we'll need to assert on the due date as well, to make sure that the correct date is picked, not just that the message gets created.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've verified that by calculating difference between duedate and start_date and ensuring it matches our offset here

Copy link
Member

Choose a reason for hiding this comment

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

Yea, I mean assert on every scheduled task, maybe included the expectedMessage function?

@1yuv 1yuv requested a review from dianabarsan October 18, 2023 20:56
Copy link
Member

@dianabarsan dianabarsan left a comment

Choose a reason for hiding this comment

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

Awesome! Thank you so much for adding an e2e test, I know schedules were not covered at all so this is great addition to see.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking about edge cases or what could go wrong. What happens if we define start_from as an array of more than two elements? What happens if every field in the start_from array is unavailable?
Does this make sense?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hi @lorerod , sure we can. Some of these were already in integraiton tests. I've added more tests and also added comment and made variable names more explanatory.

Copy link
Contributor

@lorerod lorerod left a comment

Choose a reason for hiding this comment

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

Thank you, @1yuv, for this change and for adding an integration test to cover the change. This PR looks excellent!
I left a comment if it makes sense to implement an edge case. But it is definitely not a blocker.
I couldn't find any issue on cht-docs to update the schedule documentation and include this new feature. I can create one and work with you on the change.
Let me know what you think.

@1yuv
Copy link
Member Author

1yuv commented Oct 25, 2023

Hi @lorerod , medic/cht-docs#1207 is the releated issue for documentation update and has been approved on medic/cht-docs#1208. Also, this change required cht-conf to be updated. Related issue for cht-conf was created here and fixed by this PR.

@garethbowen garethbowen merged commit 77b8ada into medic:master Oct 25, 2023
17 checks passed
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.

4 participants