-
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
Format ISO string correctly when missing reportAction to find mostRecentReportActionLastModified
#40734
Conversation
@yuwenmemon 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] |
@marcaaron looks like you have some lint stuff to fix |
Oh woops sorry this was not ready for review. Meant to create a draft 🥲 |
Ok, this is ready now. Full disclosure I only tested this on web due to the relative simplicity of the change. We could have a C+ test it, but I am 99% sure it's fine. |
Thank you for fixing this so quick! |
Reviewer Checklist
Screenshots/VideosAndroid: NativeAndroid: mWeb ChromeiOS: NativeiOS: mWeb SafariMacOS: Chrome / SafariMacOS: Desktop |
🚀 Deployed to staging by https://github.com/yuwenmemon in version: 1.4.65-0 🚀
|
🚀 Deployed to production by https://github.com/mountiny in version: 1.4.65-5 🚀
|
cc @pecanoro as this came up in #39455, but not sure if causing the problem
Details
waf is tossing values when they are not formatted correctly. we initialize the
mostRecentReportActionLastModified
withnew Date(0).toISOString()
, but need to get rid of theT
andZ
for it to pass the regex validation we use.Fixed Issues
Not really any specific issue.
Tests
Take this test with a grain of salt as I am not entirely sure how someone could end up in a situation where they do not have any
reportActions
. But to confirm the changes are valid I:mostRecentReportActionLastModified
Go offline
Come back online which should trigger a call to
ReconnectApp
We should see that the "before" value is a correct ISO formatted string and the "after" has a value (related to some report action). And verify that there is no
T
indicating the "time separator" orZ
(indicating utc) at the end of the timestampVerify we do not see any logs like (which might be hard since you probably have some valid report actions):
Offline tests
❌
QA Steps
No specific QA for this. It's not an easily reproducible situation AFAICT.
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
Skipped because the change is very small and no UI.
Android: mWeb Chrome
Skipped because the change is very small and no UI.
iOS: Native
Skipped because the change is very small and no UI.
iOS: mWeb Safari
Skipped because the change is very small and no UI.
MacOS: Chrome / Safari
MacOS: Desktop
Skipped because the change is very small and no UI.