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

Add course choice validation on sibling status #8605

Merged
merged 10 commits into from
Oct 3, 2023
Merged

Add course choice validation on sibling status #8605

merged 10 commits into from
Oct 3, 2023

Conversation

inulty-dfe
Copy link
Collaborator

@inulty-dfe inulty-dfe commented Sep 28, 2023 •
edited
Loading

Context

This PR must be merged after #8596

Currently, a database unique index on ApplicationChoice table prevents the creation of a record with the same application_form_id and course_option_id.

Duplicate application is one which is made on the same application form to the same course.

This constraint is preventing a candidate from reapplying to a course.

With Continuous Applications, we want to allow Candidates to reapply to the same course on the same application form if any previous applications on the application form to the same CourseOption have a "reapplyable" status. The reapplyable statuses are

  • rejected
  • cancelled
  • withdrawn
  • declined
  • offer_withdrawn

Changes proposed in this pull request

  • Add application level validations on the ApplicationChoice model which prevent a duplicate application WRT reapply status on siblings.

Guidance to review

  • Consider the differences beteween ReapplyValidator and the CourseSelectionValidator. Offer suggestions on how they might share behaviour.

Link to Trello card

Trello Ticket

Things to check

  • If the code removes any existing feature flags, a data migration has also been added to delete the entry from the database
  • This code does not rely on migrations in the same Pull Request
  • If this code includes a migration adding or changing columns, it also backfills existing records for consistency
  • If this code adds a column to the DB, decide whether it needs to be in analytics yml file or analytics blocklist
  • API release notes have been updated if necessary
  • If it adds a significant user-facing change, is it documented in the CHANGELOG?
  • Required environment variables have been updated added to the Azure KeyVault

@inulty-dfe inulty-dfe mentioned this pull request Sep 28, 2023
Merged
7 tasks
@inulty-dfe inulty-dfe added the deploy_v2 Deploy the review app to AKS label Sep 29, 2023
@github-actions github-actions bot temporarily deployed to review_aks-8605 September 29, 2023 08:24 Destroyed
@inulty-dfe inulty-dfe temporarily deployed to review_aks September 29, 2023 08:24 — with GitHub Actions Inactive
@inulty-dfe inulty-dfe marked this pull request as ready for review September 29, 2023 08:28
@inulty-dfe inulty-dfe force-pushed the add-course-choice-validation-on-sibling-status branch from a5c503a to 0f4a430 Compare September 29, 2023 08:41
@inulty-dfe inulty-dfe requested a review from a team September 29, 2023 08:43
@github-actions github-actions bot temporarily deployed to review_aks-8605 September 29, 2023 08:45 Destroyed
@inulty-dfe inulty-dfe temporarily deployed to review_aks September 29, 2023 08:46 — with GitHub Actions Inactive
@inulty-dfe inulty-dfe force-pushed the add-course-choice-validation-on-sibling-status branch from 0f4a430 to 68660c9 Compare September 29, 2023 08:46
@github-actions github-actions bot temporarily deployed to review_aks-8605 September 29, 2023 08:53 Destroyed
@inulty-dfe inulty-dfe temporarily deployed to review_aks September 29, 2023 08:53 — with GitHub Actions Inactive
@inulty-dfe inulty-dfe requested a deployment to review_aks September 29, 2023 09:17 — with GitHub Actions Abandoned
@github-actions
Copy link
Contributor

Code Coverage

Coverage typeFromTo
⬇️Lines covered95.62%94.9%

@github-actions github-actions bot temporarily deployed to review_aks-8605 September 29, 2023 10:02 Destroyed
@inulty-dfe inulty-dfe temporarily deployed to review_aks September 29, 2023 10:02 — with GitHub Actions Inactive
@inulty-dfe inulty-dfe force-pushed the add-course-choice-validation-on-sibling-status branch from 6414c27 to 0e4692c Compare September 29, 2023 10:16
@github-actions github-actions bot temporarily deployed to review_aks-8605 September 29, 2023 10:21 Destroyed
@inulty-dfe inulty-dfe temporarily deployed to review_aks September 29, 2023 10:21 — with GitHub Actions Inactive
tomas-stefano
tomas-stefano reviewed Sep 29, 2023
View reviewed changes
app/validators/candidate_interface/continuous_applications/course_selection_validator.rb Outdated
@@ -4,24 +4,40 @@ module ContinuousApplications
# Conditional exists to remove the course that is being edited from the checks for duplication
class CourseSelectionValidator < ActiveModel::Validator
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we could rename this to DuplicateCourseValidator

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure, lets do that in another PR

@inulty-dfe
Copy link
Collaborator Author

CA: Allow candidates to reapply for previously unsuccessful choices

@inulty-dfe inulty-dfe force-pushed the add-course-choice-validation-on-sibling-status branch 2 times, most recently from 258a754 to a8b9aec Compare September 29, 2023 16:02
@github-actions github-actions bot temporarily deployed to review_aks-8605 September 29, 2023 16:07 Destroyed
@inulty-dfe inulty-dfe temporarily deployed to review_aks September 29, 2023 16:07 — with GitHub Actions Inactive
@inulty-dfe inulty-dfe force-pushed the add-course-choice-validation-on-sibling-status branch from a8b9aec to fd0f99a Compare September 29, 2023 16:22
@github-actions github-actions bot temporarily deployed to review_aks-8605 September 29, 2023 16:26 Destroyed
@inulty-dfe inulty-dfe temporarily deployed to review_aks September 29, 2023 16:26 — with GitHub Actions Inactive
@github-actions
Copy link
Contributor

Code Coverage

Coverage typeFromTo
⬇️Lines covered95.63%95.32%

@inulty-dfe inulty-dfe requested review from tomas-stefano and a team October 2, 2023 09:53
@inulty-dfe
Validate application choice on form and course when non-reapply status
679f087 
  An application form may only have one "active" (reapplyable)
  application choice at a time. Validate that the application form and
  course choice is unique on an application form when the application
  choice is in an non-reapply status
@inulty-dfe
provider_id not needed to determine course duplication
6fa84c6
@inulty-dfe
Add spec for updating duplicate course to non-reappliable
4ceba5a 
  When two appliations exist for different course choices on the same
  course and one is reappliable, and the other is not reapplible.

  It is invalid to update the status of the reappliable to a
  non-reapplible status.

  Cannot have two  choices for the same course where both are
  non-reappliable.
@inulty-dfe
Update CourseValidation to use ApplicationChoice validator
7899032
@inulty-dfe
Guaranteed status can be converted to symbol
975af5e
@inulty-dfe
Only remove the record being updated from consideration of duplicate
4d254b1 
  When a candidate is changing the course on theire application, they
  are editing the course choice.

  A course choice must not duplicate a course choice on an application
  form unless the existing choices with the same course are in a
  non-reappliable status.

  When editing a course choice, the current application may be in a
  non-reappliable state. When we check for other records in a
  non-reappliable state it's possible to return the record we are
  editing.

  The previous change removed all applications that made an application
  to the course. We only want to remove the application being edited.

  This scenario is not possible to reproduce in the UI
@inulty-dfe
Pass record as argument to methods in ReapplyValidator
96784f8
@inulty-dfe
Pass record as argument in CourseSelectionValidator
77765a5
@inulty-dfe
Refactor ReapplyValidator
2d7f358
@inulty-dfe
Remove unused methods from spec
42ada3b
@inulty-dfe inulty-dfe force-pushed the add-course-choice-validation-on-sibling-status branch from fd0f99a to 42ada3b Compare October 2, 2023 09:55
@github-actions github-actions bot temporarily deployed to review_aks-8605 October 2, 2023 09:59 Destroyed
@inulty-dfe inulty-dfe temporarily deployed to review_aks October 2, 2023 09:59 — with GitHub Actions Inactive
@github-actions
Copy link
Contributor

github-actions bot commented Oct 2, 2023

You have one or more flakey tests on this branch! ❄️ ❄️ ❄️

Failed 1 out of 3 times at ./spec/system/support_interface/delete_application_spec.rb:7: ⚠️ expected not to find text "Chang" in "First name\nChange first name"

@github-actions
Copy link
Contributor

github-actions bot commented Oct 2, 2023

Code Coverage

Coverage typeFromTo
⬆️Lines covered95.32%95.63%

@github-actions github-actions bot temporarily deployed to review_aks-8605 October 2, 2023 14:49 Destroyed
@inulty-dfe inulty-dfe temporarily deployed to review_aks October 2, 2023 14:49 — with GitHub Actions Inactive
@inulty-dfe inulty-dfe merged commit 13eb2f3 into main Oct 3, 2023
61 checks passed
@inulty-dfe inulty-dfe deleted the add-course-choice-validation-on-sibling-status branch October 3, 2023 11:24
@inulty-dfe inulty-dfe temporarily deployed to review_aks October 3, 2023 11:24 — with GitHub Actions Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JR-G JR-G JR-G approved these changes

@tomas-stefano tomas-stefano Awaiting requested review from tomas-stefano

Assignees
No one assigned
Labels
deploy_v2 Deploy the review app to AKS
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@inulty-dfe @tomas-stefano @JR-G