FFS-2340: Encapsulate Events Handling in a New Model #491

tdooner · 2025-03-06T03:30:07Z

Ticket

Changes

FFS-2340: Create WebhookEvents to track sync success/failure

We currently use Pinwheel-specific columns on PayrollAccount to track
the status of the sync: income/paystubs/employment/identity
synced_at/errored_at.

To accommodate other payroll aggregators, and better record sync
statuses, let's create a WebhookEvents model to record which webhooks
we've received for a given PayrollAccount. Success or failure, then, is
not determined by the columns, but by the presence of the necessary
WebhookEvents for that payroll aggregator.

Context for reviewers

See above.

Acceptance testing

No acceptance testing needed
- This change will not affect the user experience (bugfix, dependency updates, etc.)
Acceptance testing prior to merge
- This change can be verified visually via screenshots attached below or by sending a link to a local development environment to the acceptance tester
- Acceptance testing should be done by design for visual changes, product for behavior/logic changes, or both for changes that impact both.
Acceptance testing after merge
- This change is hard to test locally, so we'll test it in the demo environment (deployed automatically after merge.)
- Make sure to notify the team once this PR is merged so we don't inadvertently deploy the unaccepted change to production. (e.g. :alert: Deploy block! @ffs-eng I just merged PR [#123] and will be doing acceptance testing in demo - please don't deploy until I'm finished!)

tdooner · 2025-03-06T03:32:20Z

app/app/models/payroll_account.rb

@@ -11,9 +11,12 @@ def self.sti_class_for(type_name)

  belongs_to :cbv_flow

-  after_update_commit {
-    I18n.with_locale(cbv_flow.cbv_flow_invitation.language) do
-      broadcast_replace target: self, partial: "cbv/synchronizations/indicators", locals: { pinwheel_account: self }


I moved this from the model -> webhooks controller, since it made a bit more sense to me to put it there.

We currently use Pinwheel-specific columns on `PayrollAccount` to track the status of the sync: income/paystubs/employment/identity synced_at/errored_at. To accommodate other payroll aggregators, and better record sync statuses, let's create a WebhookEvents model to record which webhooks we've received for a given PayrollAccount. Success or failure, then, is not determined by the columns, but by the presence of the necessary WebhookEvents for that payroll aggregator.

tdooner · 2025-03-06T04:33:08Z

app/app/controllers/webhooks/pinwheel/events_controller.rb

-      Rails.logger.info "Unable to find CbvFlow for end_user_id: #{params["payload"]["end_user_id"]}"
-      return render json: { status: "ok" }
+    @payroll_account = @cbv_flow.payroll_accounts.find_or_create_by(type: :pinwheel, pinwheel_account_id: params["payload"]["account_id"]) do |new_payroll_account|
+      new_payroll_account.supported_jobs = get_supported_jobs(params["payload"]["platform_id"])


This fixes a rare bug when the webhooks are received out-of-order, previously it failed because the account hadn't been created yet. Now, whichever webhook is received first will create the PayrollAccount.

I wonder if this is the bug that I encountered when pairing with @daphnegold. The synchronizations page would immediately fail for some reason unknown to me. Let's see if this fixes that issue.

Was just pairing with her - hers was a setup issue (we have a limit of 10 pinwheel webhook subscriptions, and periodically need to garbage collect old engineers out of there). So her sync was failing because pinwheel never registered the webhooks.

GeorgeCodes19

Good works. Seems like a lot. I really dig the updates to the test suite.

GeorgeCodes19 · 2025-03-07T23:36:24Z

app/app/models/payroll_account.rb

-      broadcast_replace target: self, partial: "cbv/synchronizations/indicators", locals: { pinwheel_account: self }
+  private
+
+  def webhook_event(event_name, event_outcome = nil)


I was a bit confused on the purpose of this method at first, but after reviewing some other logic in the code I can see that the purpose of this method is to check for the presence of a WebhookEvent matching the name and status/outcome. Maybe we can consider adding a verb to this method? i.e. find_webhook_by_status. It's a big verbose, but it makes following the flow a bit easier.

Yeah that's a good idea. I was waffling on it - brevity vs being explicit. I almost always lean towards "being explicit" and I think you're right that I should have here as well. I'll go with find_webhook_event.

GeorgeCodes19 · 2025-03-07T23:39:07Z

app/app/models/payroll_account/pinwheel.rb

+    (supported_jobs.exclude?("paystubs") || webhook_event(JOBS_TO_WEBHOOK_EVENTS["paystubs"]).present?) &&
+      (supported_jobs.exclude?("employment") || webhook_event(JOBS_TO_WEBHOOK_EVENTS["employment"]).present?) &&
+      (supported_jobs.exclude?("income") || webhook_event(JOBS_TO_WEBHOOK_EVENTS["income"]).present?) &&
+      (supported_jobs.exclude?("identity") || webhook_event(JOBS_TO_WEBHOOK_EVENTS["identity"]).present?)
  end

  def job_succeeded?(job)


Nice, this looks a lot better than the previous iteration.

GeorgeCodes19 · 2025-03-07T23:43:57Z

app/spec/controllers/cbv/payment_details_controller_spec.rb

-    let(:income_errored_at) { nil }
-    let(:paystubs_errored_at) { nil }
-    let(:employment_errored_at) { nil }
+    let(:errored_jobs) { [] }


Nice- I like this pattern more than explicitly digging up different *_errored_at vars

GeorgeCodes19 · 2025-03-07T23:45:37Z

app/spec/factories/payroll_account.rb

+    transient do
+      # Use this to create a test case where a job has failed to sync for a
+      # PayrollAccount.
+      with_errored_jobs { %w[] }


For some reason this is my favorite part about the PR. I appreciate a nice test suite.

GeorgeCodes19 · 2025-03-07T23:47:20Z

app/spec/models/payroll_account/pinwheel_spec.rb

+    end
+
+    context "when job is unsupported" do
+      let(:supported_jobs) { super() - [ "income" ] }


What's going on here with super()?

Good question. It refers to the supported_jobs in the enclosing context/describe. Here's an example:

describe "how super() works" do let(:arguments) { [1, 2] } it { expect(arguments).to eq([1, 2]) } # passes context "when there are three arguments" do let(:arguments) { super() + [3] } it { expect(arguments).to eq([1, 2, 3]) } # passes end end

I try not to over-use it, because lord knows Rspec tests are already hard enough to understand where the variables are coming from even without this. But I did like how readable it is to do super() - [ "income" ] to denote the fact that the "income" job is not supported.

tdooner commented Mar 6, 2025

View reviewed changes

tdooner force-pushed the td/ffs-2340-encapsulate-events-handling-in-new branch from 566fbe6 to 2b7ba7b Compare March 6, 2025 03:54

tdooner marked this pull request as ready for review March 6, 2025 04:30

tdooner commented Mar 6, 2025

View reviewed changes

tdooner requested a review from GeorgeCodes19 March 6, 2025 19:28

GeorgeCodes19 approved these changes Mar 7, 2025

View reviewed changes

Rename webhook_event -> find_webhook_event

5dd7244

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FFS-2340: Encapsulate Events Handling in a New Model #491

FFS-2340: Encapsulate Events Handling in a New Model #491

tdooner commented Mar 6, 2025

tdooner Mar 6, 2025

tdooner Mar 6, 2025 •

edited

Loading

GeorgeCodes19 Mar 7, 2025

tdooner Mar 8, 2025

GeorgeCodes19 left a comment

GeorgeCodes19 Mar 7, 2025

tdooner Mar 8, 2025

GeorgeCodes19 Mar 7, 2025

GeorgeCodes19 Mar 7, 2025

GeorgeCodes19 Mar 7, 2025

GeorgeCodes19 Mar 7, 2025

tdooner Mar 8, 2025 •

edited

Loading

tdooner Mar 8, 2025

FFS-2340: Encapsulate Events Handling in a New Model #491

Are you sure you want to change the base?

FFS-2340: Encapsulate Events Handling in a New Model #491

Conversation

tdooner commented Mar 6, 2025

Ticket

Changes

Context for reviewers

Acceptance testing

Choose a reason for hiding this comment

tdooner Mar 6, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GeorgeCodes19 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tdooner Mar 8, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tdooner Mar 6, 2025 •

edited

Loading

tdooner Mar 8, 2025 •

edited

Loading