Page MenuHomePhabricator

[Cashtab] Set rewards eligibility to true on countdown expiration
ClosedPublic

Authored by bytesofman on May 4 2024, 21:00.

Details

Summary

Current behavior: button keeps counting down (negative countdown).

After this: user can click button to claim rewards exactly when countdown runs out.

Test Plan

npm test

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

advance by more time in the test

bytesofman published this revision for review.May 4 2024, 21:46
bytesofman added inline comments.
cashtab/src/components/Rewards/__tests__/index.test.js
172 ↗(On Diff #47620)

mock API so that user is eligible in a few seconds

cashtab/src/components/Rewards/index.js
119 ↗(On Diff #47620)

these gates before changing state are to handle handleCountdownExpiration being called multiple times

This can happen as react state setting is async, and, until countdownInterval is cleared, we will still be calling getParsedTimeRemaining from the interval.

We don't want to unnecessarily change the state....but the real reason this was added is bc the tests throw a "too many rerenders" error if it isn't here. This is (probably) caused by jest fake timers and some unrelated issue, but it is also good to not change state unnecessarily.

Fabien requested changes to this revision.May 6 2024, 08:40
Fabien added a subscriber: Fabien.
Fabien added inline comments.
cashtab/src/components/Rewards/index.js
91 ↗(On Diff #47621)

I don't understand how this works. This returns 3 numbers so you are skipping the padding from below, shouldn't it be return { hours: '00', minutes: '00', seconds: '00' }; instead ?

98 ↗(On Diff #47621)
This revision now requires changes to proceed.May 6 2024, 08:40
bytesofman marked 2 inline comments as done.

use const for repeated elapsed timers, correct double 0 string issue

cashtab/src/components/Rewards/index.js
91 ↗(On Diff #47621)

indeed.

98 ↗(On Diff #47621)

Yes. We would still need to apply it conditionally, though, as we only need padStart for the < 10 case.

In this case, I think 0${hours} is more readable...but yeah, subjective. TIL, will keep padStart in mind for future problems.

cashtab/src/components/Rewards/index.js
98 ↗(On Diff #47621)

The whole point of the padStart function is that you don't need the if at all.
hours.padStart(2, '0) will return '01' when hours is '1' or '12' if hours is '12' as expected.

Fabien requested changes to this revision.May 7 2024, 07:18
Fabien added inline comments.
cashtab/src/components/Rewards/index.js
95 ↗(On Diff #47647)

So here it's a number, right ? So if hours is >= 10, it will remain a number and not be converted to string ? You only solved half of the issue I mentioned previously.

Also the tests are obviously not catching it which is another issue.

This revision now requires changes to proceed.May 7 2024, 07:18
bytesofman marked 2 inline comments as done.

improve logic by using padstart

cashtab/src/components/Rewards/index.js
95 ↗(On Diff #47647)

the tests are designed around the expected behavior of the app. Yes, the implementation here was lazy. And yes, the tests do not approach the granularity of "confirm all numbers are converted to string at the same time before being rendered."

I don't think it's a major systemic issue here though. In this case, confirming the user sees what is expected is the important thing. The actual math and logic for eligibility is handled by the server, where tests are more appropriate to this requirement.

98 ↗(On Diff #47621)

yup I misread the padStart docs. updated + implemented.

cashtab/src/components/Rewards/index.js
118 ↗(On Diff #47662)

Do you really need the if (!isEligible) ? Looks like doing setIsEligible(true) unconditionally has the same effect.

Fabien requested changes to this revision.May 8 2024, 07:58
This revision now requires changes to proceed.May 8 2024, 07:58
bytesofman marked an inline comment as done.
bytesofman added inline comments.
cashtab/src/components/Rewards/index.js
118 ↗(On Diff #47662)

Yes, it does seem to be overkill. I added it because the integration test failed without it ("too many re-renders")

The "too many rerenders" is likely due to using fake timers in the test. But there is still the issue of state setting being async -- so we could reasonably expect handleCountdownExpiration to be called more than once in prod before the interval is really cleared.

It's not a big deal if we setIsEligible(true) a few times (would expect once or twice tops in prod). But since this was needed to clear the test, and does avoid an unnecessary state change, went ahead and put it in.

This revision is now accepted and ready to land.May 8 2024, 11:58