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

Mg 948 consistent results #70

Merged
merged 5 commits into from
Jun 24, 2024
Merged

Mg 948 consistent results #70

merged 5 commits into from
Jun 24, 2024

Conversation

allthesignals
Copy link
Contributor

@allthesignals allthesignals commented Jun 24, 2024

Ticket

Resolves 948

Changes

Adds new event hook.

TODO:

  • Wait for that hook to complete
  • Only filter for the 90 day range

Context for reviewers

Leverages ninety_days_synced event for more sensible and consistent results. Also filters by "90 days ago".

We must re-run the Webhook step for production! We need to subscribe to this new event or it won't work

Multiple controllers will be easier to test, reason about, and
simultaneously edit as we continue development. This commit splits up
the controllers as such:

* All CBV flow pages still have URLs under `/cbv/`
* The controllers are nested in the `app/controllers/cbv` directory, and
  are within a new `Cbv` Ruby module accordingly. E.g. `/cbv/entry` is
  in `app/controllers/cbv/entries_controller.rb` and the class name is
  `Cbv::EntriesController`.
* Common functionality is extracted into a `Cbv::BaseController` which
  all pages inherit from.

This refactor also changes a couple other things:
1. The invitation URL is now not in the `/cbv/` URL path. It is now
   `/invitations/new?secret=[secret]`. I've added a redirect.
2. Moves I18n keys to relative keys where possible

[Finishes FFS-807](https://jiraent.cms.gov/browse/FFS-807)
@allthesignals allthesignals changed the base branch from main to td/refactor-cbv-flow-into-multiple-controllers-ffs-807 June 24, 2024 19:20
Base automatically changed from td/refactor-cbv-flow-into-multiple-controllers-ffs-807 to main June 24, 2024 19:33
@allthesignals allthesignals marked this pull request as ready for review June 24, 2024 21:40
Copy link
Contributor

@tdooner tdooner 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. I'm going to merge + deploy now so it's ready for bug bash tomorrow.

onSuccess: this.onSignInSuccess.bind(this),
}));
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

ty

@allthesignals allthesignals merged commit 6e7e103 into main Jun 24, 2024
3 checks passed
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