LG-11441 - Fix Flakey In-person Enrollment test

[JackRyan1989]

🎫 Ticket

🛠 Summary of changes

The issue appears not to be with the spec, but rather with how we were calculating the difference between the start date and the current date. Now using the Time object to perform simple subtraction between the start date and current date.

📜 Testing Plan

Make sure that /spec/models/in_person_enrollment_spec.rb doesn't fail intermittently.

[aduth]

If the issue is with the underlying logic, can we update or add spec coverage to reliably check against regressions?

[JackRyan1989]

@aduth I think the tests I added are sufficient, let me know your thoughts.

[mitchellhenke]

is there a config this is based on that could be used? Ditto for the other places where the tests use 9.5, 10, etc.

[JackRyan1989]

I could set the values in let blocks like above.

[JackRyan1989]

@mitchellhenke see b0fcb1847acc2ffb8b212e7cc55b54e339b71de5

[aduth]

@JackRyan1989 I think @mitchellhenke's point is that the timespans depend on the in_person_enrollment_validity_in_days configuration, so we should control that either by basing the timespans on the configuration, or stubbing the configuration to a fixed value for the duration of the test.

The risk is that if we change the default configuration in the future or if a developer has their own overrides for the configuration value, the tests may suddenly start breaking.

[JackRyan1989]

Ah whoops, I got confused. I thought you were specifically talking about the calculation for the enrollment_established_at value, and that can be any date and is not necessarily dependent on the configuration.

I've updated the expectations to match the previous pattern, e.g. expect(enrollment.days_to_due_date).to eq(validity_in_days - days_ago_established_at)

So for example:

expect(enrollment.days_to_due_date).to eq((validity_in_days - nine).to_i)
  expect(enrollment.due_date).to(
            eq((validity_in_days - nine).days.from_now),
            )

I think I understand now, but please let me know if I'm still missing something.

[mitchellhenke]

Sorry, yeah, I didn't explain it well. The goal would be to avoid a change in validity_in_days changing and breaking the tests. fa67fbb was I think the preferred approach.

My suggestion is something along these lines:

    it 'due_date returns the enrollment expiration date based on when it was established' do
      freeze_time do
        enrollment = create(
          :in_person_enrollment,
          enrollment_established_at: 3.days.ago,
        )
        expect(enrollment.due_date).to eq((validity_in_days - 3).days.from_now)
      end
    end

    it 'days_to_due_date returns the number of days left until the due date' do
      freeze_time do
        enrollment = create(
          :in_person_enrollment,
          enrollment_established_at: 3.days.ago,
        )
        expect(enrollment.days_to_due_date).to eq(validity_in_days - 3)
      end
    end

    context 'check edges to confirm date calculation is correct' do
      it 'returns the correct due date and days to due date with 1 day left' do
        freeze_time do
          enrollment = create(
            :in_person_enrollment,
            enrollment_established_at: (validity_in_days - 1).days.ago,
          )
          expect(enrollment.days_to_due_date).to eq(1)
          expect(enrollment.due_date).to eq(Time.zone.now + 1.day)
        end
      end

      it 'returns the correct due date and days to due date with 0.5 days left' do
        freeze_time do
          enrollment = create(
            :in_person_enrollment,
            enrollment_established_at: (validity_in_days - 0.5).days.ago,
          )
          expect(enrollment.days_to_due_date).to eq(0)
          expect(enrollment.due_date).to eq(Time.zone.now + 0.5.days)
        end
      end

      it 'returns the correct due date and days to due date with 0 days left' do
        freeze_time do
          enrollment = create(
            :in_person_enrollment,
            enrollment_established_at: validity_in_days.days.ago,
          )
          expect(enrollment.days_to_due_date).to eq(0)
          expect(enrollment.due_date).to eq(Time.zone.now)
        end
      end
    end

[JackRyan1989]

Thanks for breaking this down @mitchellhenke and @aduth. Appropriate changes should be up

[aduth]

Clarifying question, but otherwise LGTM 👍

[aduth]

Just want to double-check that rounding down to zero is the expected result here? I see some related subtraction math on this number that might worry me if we tell the user that they "have -1 days remaining" or some such.

identity-idp/app/presenters/idv/in_person/ready_to_verify_presenter.rb

Lines 17 to 20 in 6a4d5db

	# Reminder is exclusive of the day the email is sent (1 less than days_to_due_date)
	def days_remaining
	enrollment.days_to_due_date - 1
	end

[JackRyan1989]

@aduth great callout. I think we're ok. I'm currently digging through the ready_to_verify_presenter_spec: I just added two tests locally for 1 day remaining and 0 days remaining. 1 day remaining works, 0 days remaining fails because we get a -1, as expected by your observation.

The other test already in that file passes, so I think the behavior is expected. We do have a in_person_deadline_passed email that goes out, and I'm just digging around now for the logic that controls when that is sent so I can confirm that on 0 days remaining that email is sent and the in_person_ready_to_verify_reminder email is not. I'll confirm in another message once I'm done digging.

[JackRyan1989]

In the email_reminder_job the latest we send the reminder is 1 day before expiration. In that case we'd say '0 days left' or something similar, and we would not send a negative value.

[aduth]

Ok 👍 Thanks for checking