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

Test: task service tests; minimal test infrastructure for other services #2197

Merged
merged 15 commits into from
Feb 7, 2024

Conversation

jfmcquade
Copy link
Collaborator

PR Checklist

  • PR title descriptive (can be used in release notes)

Description

Follow-up to #2176

  • Adds unit tests for the task service
  • Includes some minor refactors of task logic informed by writing the tests
  • Removes task_group_set_highlighted action
    • This was unused and relies on "skipped_field" column in tasks, which was never implemented
    • The logic remains, commented out, for skipping ahead and forward within a group of tasks by explicitly setting the highlighted task
  • Adds test scaffolding and mocks for other services on which the task service depends:
    • campaign
    • template field
    • template translate
    • app config
    • app data
  • Includes Fix/ng tests #2080

Git Issues

Closes #

Screenshots/Videos

Screenshot 2024-02-07 at 14 26 05

)
).toBe(true);
});
it("completing the highlighted task causes the next highest priority task to be highlighted upon re-evaluation", async () => {
Copy link
Member

Choose a reason for hiding this comment

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

nit(non-blocking) - for future dev, would it make sense to automatically re-evaluate after setting completed?

expect(previousHighlightedTaskGroup).toBe(MOCK_DATA.data_list[taskGroupsListName].rows[0].id);
expect(newHighlightedTaskGroup).toBe(MOCK_DATA.data_list[taskGroupsListName].rows[2].id);
});
it("when all tasks are completed, the highlighted task group is set to ''", async () => {
Copy link
Member

Choose a reason for hiding this comment

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

nit(non-blocking) - again more on the dev side. Given that previous and next highlighted task groups can be undefined (e.g. when running method without any params), does assign '' provide any additional info? e.g. to we either check for ==='' or just use truthy/falsy checks? If the latter than can probably set as undefined and keep just the two potential states (truthy string or undefined)

Copy link
Member

@chrismclarke chrismclarke left a comment

Choose a reason for hiding this comment

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

Thanks for adding all the tests and tidying up the code. I've run the tests locally and all looking good to me.

I've added a couple minor non-blocking comments (likely to feed into v2 proposals/plans), but very happy to merge here for now - will definitely make it a lot easier to continue to develop the feature in the future.

Also thanks for migrating the draft pr notes to a separate issue, makes a lot more sense having as an issue.

@chrismclarke chrismclarke merged commit 07e9102 into master Feb 7, 2024
8 checks passed
@chrismclarke chrismclarke deleted the test/task-service branch February 7, 2024 17:34
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