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

#2033 - Validating FT and PT offering minimum length #2041

Merged
merged 6 commits into from
Jun 27, 2023

Conversation

dheepak-aot
Copy link
Collaborator

@dheepak-aot dheepak-aot commented Jun 22, 2023

Update the minimum offering length validation as per offering intensity

  • Updated @PeriodMinLength decorator to accept either a function or a value for minDaysAllowed.
    When the same decorator is used for offering validation, the function studyPeriodMinLength is assigned to minAllowedDays which calculates minAllowedDays based on offering intensity.
  • Updated @HasValidOfferingPeriodForFundedDays decorator to have an additional argument to get the minimum allowed days for offering and not use a direct value from constant. The third argument is a function which provides the minimum allowed days based on the offering intensity.
  • Updated educationprogramoffering form to have client side hidden variables to get minimum and maximum days for study period length and display the banner messaged using the variables.

Full Time

image

Part Time

image

Bulk offering upload testing

  • Tested for offering period less the minimum days for Full Time

image

  • Tested for offering period less the minimum days for Part Time

image

  • Tested with valid minimum days for Full Time

image

@dheepak-aot dheepak-aot self-assigned this Jun 22, 2023
@dheepak-aot dheepak-aot added the SIMS-Api SIMS-Api label Jun 22, 2023
@dheepak-aot dheepak-aot marked this pull request as ready for review June 26, 2023 23:53
@dheepak-aot dheepak-aot added the Form.io Form IO definitions changed. label Jun 26, 2023
@@ -23,20 +20,32 @@ class HasValidOfferingPeriodForFundedDaysConstraint
implements ValidatorConstraintInterface
{
validate(studyBreaks: StudyBreak[], args: ValidationArguments): boolean {
const offeringMinDaysAllowedValue = this.getOfferingMinAllowedDays(args);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice implementation to pass the args from the form with respect to the offering intensity, but can this be shared done as a constant without passing them from the form?

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.

Nice work @dheepak-aot, just a doubt I have to better understand the implementation.

@ann-aot
Copy link
Contributor

ann-aot commented Jun 27, 2023

image
I am sorry, I didn't understand the last screenshot. was the intention to show the approved status?

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. Added some minor comments

private getMinAllowedDays(args: ValidationArguments): number {
const [, minDaysAllowed] = args.constraints;
const minDaysAllowedValue =
minDaysAllowed instanceof Function
Copy link
Collaborator

Choose a reason for hiding this comment

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

Cool!

Copy link
Collaborator

@andrepestana-aot andrepestana-aot left a comment

Choose a reason for hiding this comment

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

There are just minor things comment by Ann to be changed but the looks good to me. Well done.

@dheepak-aot
Copy link
Collaborator Author

image
I am sorry, I didn't understand the last screenshot. was the intention to show the approved status?

Yes @ann-aot the intention was to show one happy path of bulk offering upload testing.

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.

Great work, only minor comments 👍

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.

Looks good to me Dheepak 👍. Thank You for providing a walkthrough of your PR.

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, thanks for the explanation @dheepak-aot

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 👍 Thanks for doing the changes @dheepak-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.94% ( 2125 / 11843 )
Methods: 8.33% ( 126 / 1512 )
Lines: 20.74% ( 1861 / 8975 )
Branches: 10.18% ( 138 / 1356 )

@github-actions
Copy link

E2E Queue Consumers Coverage Report

Totals Coverage
Statements: 69.4% ( 390 / 562 )
Methods: 59.15% ( 42 / 71 )
Lines: 71.52% ( 344 / 481 )
Branches: 40% ( 4 / 10 )

@github-actions
Copy link

E2E Workflow Workers Coverage Report

Totals Coverage
Statements: 49.81% ( 267 / 536 )
Methods: 41.56% ( 32 / 77 )
Lines: 55.33% ( 218 / 394 )
Branches: 26.15% ( 17 / 65 )

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 even the minor changes, look good 👍

@github-actions
Copy link

E2E SIMS API Coverage Report

Totals Coverage
Statements: 49.56% ( 3497 / 7056 )
Methods: 44.43% ( 407 / 916 )
Lines: 54.84% ( 2885 / 5261 )
Branches: 23.32% ( 205 / 879 )

@dheepak-aot dheepak-aot merged commit f7e819d into main Jun 27, 2023
@dheepak-aot dheepak-aot temporarily deployed to DEV June 27, 2023 23:21 — with GitHub Actions Inactive
@dheepak-aot dheepak-aot temporarily deployed to DEV June 27, 2023 23:25 — with GitHub Actions Inactive
@dheepak-aot dheepak-aot temporarily deployed to DEV June 27, 2023 23:25 — with GitHub Actions Inactive
@dheepak-aot dheepak-aot temporarily deployed to DEV June 27, 2023 23:25 — with GitHub Actions Inactive
@dheepak-aot dheepak-aot temporarily deployed to DEV June 27, 2023 23:25 — with GitHub Actions Inactive
@dheepak-aot dheepak-aot temporarily deployed to DEV June 27, 2023 23:27 — with GitHub Actions Inactive
@dheepak-aot dheepak-aot temporarily deployed to DEV June 27, 2023 23:27 — with GitHub Actions Inactive
@dheepak-aot dheepak-aot deleted the enhancement/#2033-minimum-study-period-length branch June 30, 2023 13:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Form.io Form IO definitions changed. SIMS-Api SIMS-Api
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants