-
Notifications
You must be signed in to change notification settings - Fork 3k
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
integrate payment card section with API #43473
integrate payment card section with API #43473
Conversation
920840f
to
1601259
Compare
@rushatgabhane Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
* @returns - The next billing date in 'yyyy-MM-dd' format. | ||
*/ | ||
function getNextBillingDate(initialDate: string): string { | ||
const start = new Date(initialDate); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add some validations for dates here? My experience with date-fns
is that if the date is invalid, it'll throw an error.
cc - @amyevans
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes to validation as a best practice if it's not too challenging, although the data from the BE should be pretty uniform so it'd have to be something very weird happening.
Also I mentioned this in another PR and @MrMuzyk said he might create a util, but due to timezone differences I think at least for now we should pass T00:00:00
so that the date is properly calculated (e.g. for me if my start date is 6/1/24 the next bill date should be 7/1/24, but it's currently showing 6/30/24)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated with validation and proper billing day
* @param initialDate - The initial billing date in 'yyyy-MM-dd' format. | ||
* @returns - The next billing date in 'yyyy-MM-dd' format. | ||
*/ | ||
function getNextBillingDate(initialDate: string): string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what do you think of this to get rid of while
loop.
function getNextBillingDate(initialDate: string): string {
const start = new Date(initialDate);
const today = new Date();
// Calculate the number of months difference and add one if start date is before today
const monthsDiff = differenceInMonths(today, start);
const nextBillingDate = addMonths(start, monthsDiff + 1);
return format(nextBillingDate, CONST.DATE.MONTH_DAY_YEAR_FORMAT);
}
cc - @amyevans
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah if we can eliminate a loop that would be great!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
@@ -0,0 +1,21 @@ | |||
import {addMonths, format, isBefore} from 'date-fns'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@amyevans Does it makes sense to add tests for this utils?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I think that'd be good!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tests added
I see some flicker from default text to next billing date. I am not sure if it's because of the default card useEffect. flicker-card-number.mov |
@mananjadhav Yes, flickering happening because of test data |
@mananjadhav @amyevans PR is ready for rereview |
I’ll be able to review this in an hour. |
start = new Date(); | ||
} | ||
|
||
const current = new Date(start); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Problem fixed
I also found one more edge case and fixed it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I raised one more comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't reviewed the function logic closely but just glancing at the expected next billing dates in the test, I wanted to clarify expected behavior... the next billing date should always be the first of the next month (internal ref)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have the access to the link, but does that mean we don't even need the initialDate
. We should always:
startOfMonth(addMonths(today, 1))
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes we can simplify to that, apologies for not catching it sooner!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No worries. Better we caught this now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pasyukevich Can you take care of this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
I did the review again and except for the getNextBillingDate revised method, rest looks good to me. |
let nextBillingDate = addDays(addMonths(start, monthsDiff), 1); | ||
|
||
if (nextBillingDate.toUTCString() < today.toUTCString()) { | ||
nextBillingDate = addMonths(nextBillingDate, 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this fix to always ensure that the nextBillingDate is in the future? In this case should this be
nextBillingDate = addMonths(nextBillingDate, 1); | |
nextBillingDate = addMonths(today, 1); |
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, but this update is not doing same as now
In this case, we will have today's as a billing date
PR updated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, @mananjadhav can you do a final review/complete the checklist?
describe('getNextBillingDate', () => { | ||
beforeAll(() => { | ||
jest.useFakeTimers(); | ||
jest.setSystemTime(new Date(2024, 6, 5)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NAB, maybe we can add a comment that month is zero-indexed? It tripped me up for a sec 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
CC: @blimpich
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is the comment? I think what Amy meant was something like this
jest.setSystemTime(new Date(2024, 6, 5)); | |
// Month is zero indexed, so this is July 5th 2024 | |
jest.setSystemTime(new Date(2024, 6, 5)); |
Requesting final review from @blimpich as well since I'm headed out on extended leave after today |
For the next two weeks, I will be OOO @JKobrynski will take to work on this issue |
Code changes are fine now. Working on the checklist. |
Reviewer Checklist
Screenshots/Videos |
@blimpich All yours. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks good to me, but I'm not able to successfully follow the QA steps. I get a bunch of errors when I try to follow the setup part of the QA to modify src/pages/settings/Subscription/CardSection/CardSection.tsx
. And when I don't follow that it doesn't work.
Can we modify the QA steps to show exactly what diff should be applied to CardSection.tsx
?
describe('getNextBillingDate', () => { | ||
beforeAll(() => { | ||
jest.useFakeTimers(); | ||
jest.setSystemTime(new Date(2024, 6, 5)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is the comment? I think what Amy meant was something like this
jest.setSystemTime(new Date(2024, 6, 5)); | |
// Month is zero indexed, so this is July 5th 2024 | |
jest.setSystemTime(new Date(2024, 6, 5)); |
@blimpich forgot to push 🤦 now it should be fine |
I'm still not able to QA this, shouldn't a card being showing up in the drop down? Screen.Recording.2024-06-20.at.11.55.52.AM.mov
Still curious about this. How exactly is CardSection.tsx suppose to be updated in order to QA this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Finally got it to QA successfully, sorry it took me so long. I think I had a typo the first time I tried to QA and it really threw me off. Looks good, lets get the conflicts fixed and merge this 👍
@blimpich conflicts resolved! |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by https://github.com/blimpich in version: 9.0.1-0 🚀
|
🚀 Deployed to production by https://github.com/yuwenmemon in version: 9.0.1-19 🚀
|
Details
Prepare account for testing:
Sign up for an account on staging.expensify.com, go to Settings > Workspaces > New Workspace. Create a Collect workspace.
To access this newly created component, paste the following link into the browser
https://dev.new.expensify.com:8082/settings/subscription
or add this effect to InitialSettingsPage.tsx
Add card data to CardSection.tsx :
Fixed Issues
$ #38618 #38617
PROPOSAL:
Tests
Offline tests
QA Steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop