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.

Sign up for GitHub

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

Jump to bottom

#2181 - Queued Assessments - Assessment Workflow Enqueuer Scheduler - Cancel Assessment #2375

Merged
merged 17 commits into from
Oct 11, 2023
Merged

#2181 - Queued Assessments - Assessment Workflow Enqueuer Scheduler - Cancel Assessment #2375

merged 17 commits into from
Oct 11, 2023

Conversation

andrepestana-aot
Copy link
Collaborator

@andrepestana-aot andrepestana-aot commented Oct 4, 2023 •
edited
Loading

  • Created new service call in the assessment workflow enqueuer to add queried items to cancel-application-assessment queue.
  • Created query to select applications that has assessments with status CancellationRequested but no other assessments with status CancellationQueued.
  • Changed the points that were enqueueing the assessments right away to be canceled to change the status to CancellationRequested.
  • Added an update to the assessment status to Cancelled in the cancel-application-assessment.processor.ts.
  • Changed the workflow-wrap-up to only change the status to Complete if the assessment is not already cancelled or it is not in the process of being cancelled.
  • Added e2e tests.

@andrepestana-aot andrepestana-aot self-assigned this Oct 5, 2023
@andrepestana-aot andrepestana-aot changed the title initial commit #2181 - Queued Assessments - Assessment Workflow Enqueuer Scheduler - Cancel Assessment Oct 5, 2023
@andrepestana-aot andrepestana-aot marked this pull request as ready for review October 5, 2023 21:08
andrewsignori-aot
andrewsignori-aot reviewed Oct 10, 2023
View reviewed changes
Copy link
Collaborator

@andrewsignori-aot andrewsignori-aot 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 making the changes, only minor comments are missing.

dheepak-aot
dheepak-aot approved these changes Oct 10, 2023
View reviewed changes
Copy link
Collaborator

@dheepak-aot dheepak-aot 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 making the changes 👍

ann-aot
ann-aot reviewed Oct 10, 2023
View reviewed changes
sources/packages/backend/apps/queue-consumers/src/services/application/application.service.ts
status: StudentAssessmentStatus.CancellationRequested,
})
// This condition may not be directly related to the current application logic but has been added to ensure that
// at any given time now or future the queue does not pick an application to cancel when there is already an
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

ann-aot
ann-aot approved these changes Oct 10, 2023
View reviewed changes
Copy link
Contributor

@ann-aot ann-aot left a comment

Choose a reason for hiding this comment

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

LGTM 👍 Great work @andrepestana-aot . Pls take a look at the minor comments added by the devs

sh16011993
sh16011993 reviewed Oct 11, 2023
View reviewed changes
...ces/packages/backend/apps/queue-consumers/src/services/workflow/workflow-enqueuer.service.ts
if (!applications.length) {
return;
}
const children = await processInParallel(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we also consider specifying the 3rd parameter: maxParallelRequests here?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hope that it was clarified but when not provided it will be set to the amount of CPUs.

sh16011993
sh16011993 approved these changes Oct 11, 2023
View reviewed changes
Copy link
Collaborator

@sh16011993 sh16011993 left a comment

Choose a reason for hiding this comment

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

Nice work @andrepestana-aot 👍 Just added one info comment.

guru-aot
guru-aot approved these changes Oct 11, 2023
View reviewed changes
Copy link
Collaborator

@guru-aot guru-aot left a comment

Choose a reason for hiding this comment

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

LGTM, nice work @andrepestana-aot

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@github-actions
Copy link

Backend Unit Tests Coverage Report

Totals Coverage
Statements: 17.35% ( 2203 / 12701 )
Methods: 8.06% ( 129 / 1601 )
Lines: 20.06% ( 1930 / 9622 )
Branches: 9.74% ( 144 / 1478 )

@github-actions
Copy link

E2E Workflow Workers Coverage Report

Totals Coverage
Statements: 46.3% ( 300 / 648 )
Methods: 40% ( 32 / 80 )
Lines: 50.6% ( 251 / 496 )
Branches: 23.61% ( 17 / 72 )

@github-actions
Copy link

E2E Queue Consumers Coverage Report

Totals Coverage
Statements: 75.72% ( 555 / 733 )
Methods: 69.32% ( 61 / 88 )
Lines: 77.64% ( 486 / 626 )
Branches: 42.11% ( 8 / 19 )

@github-actions
Copy link

E2E SIMS API Coverage Report

Totals Coverage
Statements: 54.98% ( 4026 / 7322 )
Methods: 52.05% ( 496 / 953 )
Lines: 59.76% ( 3267 / 5467 )
Branches: 29.16% ( 263 / 902 )

andrewsignori-aot
andrewsignori-aot approved these changes Oct 11, 2023
View reviewed changes
Copy link
Collaborator

@andrewsignori-aot andrewsignori-aot 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 doing the changes, looks good 👍

@andrepestana-aot andrepestana-aot merged commit ed6cdcf into main Oct 11, 2023
@andrepestana-aot andrepestana-aot deleted the 2181_queued_assessment_cancellation branch October 11, 2023 19:48
@andrepestana-aot andrepestana-aot temporarily deployed to DEV October 11, 2023 20:02 — with GitHub Actions Inactive
@andrepestana-aot andrepestana-aot temporarily deployed to DEV October 11, 2023 20:03 — with GitHub Actions Inactive
@andrepestana-aot andrepestana-aot temporarily deployed to DEV October 11, 2023 20:03 — with GitHub Actions Inactive
@andrepestana-aot andrepestana-aot temporarily deployed to DEV October 11, 2023 20:03 — with GitHub Actions Inactive
@andrepestana-aot andrepestana-aot temporarily deployed to DEV October 11, 2023 20:03 — with GitHub Actions Inactive
@andrepestana-aot andrepestana-aot temporarily deployed to DEV October 11, 2023 20:05 — with GitHub Actions Inactive
@andrepestana-aot andrepestana-aot temporarily deployed to DEV October 11, 2023 20:05 — with GitHub Actions Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ann-aot ann-aot ann-aot approved these changes

@guru-aot guru-aot guru-aot approved these changes

@andrewsignori-aot andrewsignori-aot andrewsignori-aot approved these changes

@dheepak-aot dheepak-aot dheepak-aot approved these changes

@sh16011993 sh16011993 sh16011993 approved these changes

@JasonCTang JasonCTang Awaiting requested review from JasonCTang

Assignees

@andrepestana-aot andrepestana-aot

Labels
Camunda Workers Queue Consumers
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@andrepestana-aot @dheepak-aot @sh16011993 @andrewsignori-aot @guru-aot @ann-aot