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

2315 bug declinebydefaultat field is incorrectly being set #9861

Merged

Conversation

elceebee
Copy link
Contributor

@elceebee elceebee commented Sep 23, 2024

Context

It was noted earlier this month that some application choices have decline_by_default_at dates set. Some, but not all, were set to the actual CycleTimetable.decline_by_default_date.

Now that declining by default is only something that happens at the end of cycle, and is not specific to a single application_form, we no longer need decline_by_default_at or decline_by_default_days on each application_choice

There will be two further PRs:

  1. A migration to remove the data from the 40,100 + application choices.
  2. A PR to deprecate the columns so we don't start accidentally updating them again in the future.

Changes proposed in this pull request

  1. Remove the places in the code where this date was being set:
  • The job from 2023
  • When an offer is made
  • When an offer is reinstated
  1. Remove uses / methods related to these columns throughout.
  2. Fix flaky spec (unrelated, separate commit)

No real UI changes.

Guidance to review

Sadly, this won't be in in time for any of our 'bug parties', so it would be good if people could use the review app to just check everything is working as expected for making offers / reinstating offers.

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
  • Inform data insights team due to database changes
  • Make sure all information from the Trello card is in here
  • Rebased main
  • Cleaned commit history
  • Tested by running locally
  • Add PR link to Trello card

@elceebee elceebee self-assigned this Sep 23, 2024
@elceebee elceebee force-pushed the 2315-bug-declinebydefaultat-field-is-incorrectly-being-set branch from c37d04a to de355b4 Compare September 23, 2024 18:42
Copy link
Contributor

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 Change first name"

@elceebee elceebee force-pushed the 2315-bug-declinebydefaultat-field-is-incorrectly-being-set branch from ed1d084 to e8bc596 Compare September 24, 2024 12:25
@elceebee elceebee force-pushed the 2315-bug-declinebydefaultat-field-is-incorrectly-being-set branch from e8bc596 to 6e23924 Compare September 24, 2024 12:53
@elceebee elceebee force-pushed the 2315-bug-declinebydefaultat-field-is-incorrectly-being-set branch from 6e23924 to 7e54709 Compare September 24, 2024 12:54
@elceebee elceebee force-pushed the 2315-bug-declinebydefaultat-field-is-incorrectly-being-set branch from 7e54709 to f26bf4b Compare September 24, 2024 13:25
@elceebee elceebee added the deploy_v2 Deploy the review app to AKS label Sep 24, 2024
@github-actions github-actions bot temporarily deployed to review_aks-9861 September 24, 2024 13:48 Destroyed
@elceebee
Copy link
Contributor Author

@elceebee elceebee marked this pull request as ready for review September 24, 2024 14:47
@elceebee elceebee requested a review from a team September 24, 2024 14:50
@elceebee elceebee marked this pull request as draft September 24, 2024 14:51
@elceebee elceebee force-pushed the 2315-bug-declinebydefaultat-field-is-incorrectly-being-set branch from f26bf4b to 09df794 Compare September 24, 2024 14:59
@github-actions github-actions bot temporarily deployed to review_aks-9861 September 24, 2024 15:04 Destroyed
@elceebee elceebee force-pushed the 2315-bug-declinebydefaultat-field-is-incorrectly-being-set branch from 09df794 to edcfeba Compare September 24, 2024 15:17
@elceebee elceebee marked this pull request as ready for review September 24, 2024 15:21
@github-actions github-actions bot temporarily deployed to review_aks-9861 September 24, 2024 15:29 Destroyed
Copy link
Collaborator

@tomas-stefano tomas-stefano left a comment

Choose a reason for hiding this comment

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

Take a closer look and everything is good 👍

One small question, is it this PR should remove things like?

  1. ReinstateDeclinedOffer#reset_all_dbd
  2. time_limit_config chase_candidate_before_dbd rule
  3. DBD mention on app/views/support_interface/application_forms/application_choices/reinstate_declined_offer/confirm_reinstate_offer.html.erb

If is not, can you create a small card to handle those any time in the future?

@elceebee
Copy link
Contributor Author

elceebee commented Sep 24, 2024

Great work spotting these other uses, @tomas-stefano. I will add them to this PR. More deleting! Hurrah!

@github-actions github-actions bot temporarily deployed to review_aks-9861 September 24, 2024 16:13 Destroyed
@elceebee elceebee force-pushed the 2315-bug-declinebydefaultat-field-is-incorrectly-being-set branch from f18269e to 9021251 Compare September 24, 2024 16:26
@github-actions github-actions bot temporarily deployed to review_aks-9861 September 24, 2024 16:31 Destroyed
@elceebee elceebee merged commit fd6b2aa into main Sep 25, 2024
60 checks passed
@elceebee elceebee deleted the 2315-bug-declinebydefaultat-field-is-incorrectly-being-set branch September 25, 2024 08:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deploy_v2 Deploy the review app to AKS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants