-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Standardize on using created instead of timestamp #12540
Conversation
@luacmartins Is it good to test locally? considering its tagged on |
@mananjadhav this will fail right now since the date format returned by the API is invalid for the changes in this PR. We'll ping you once it's ready for you to test! |
@roryabraham I'm going ooo until Nov 22, could you help push this through the finish line please? |
I just reviewed the code. Going through the code to see if we've missed any references and will start with the testing then. |
} catch (IllegalAccessException e) { | ||
e.printStackTrace(); | ||
} catch (InvocationTargetException e) { | ||
} catch (Exception e) { |
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.
Changes and comment updates seem good to me. Testing well also 👍
@Julesssss @roryabraham @luacmartins I was testing this and I've got one blocker. Unfortunately I couldn't get a physical Android device where I could received the Push Notifications for now. The chat timestamps etc, work fine but the PN avatar image I am unable to test. Reviewer Checklist
ScreenshotsWebMobile Web - ChromeMobile Web - SafariDesktopiOSAndroid |
No worries, I'm certain I tested that, but I'm going to test once more quickly to confirm. |
Thanks @Julesssss I've then completed the reviewer checklist. |
Damn, actually I'm going to run out of time for this today.
@luacmartins did you need a release build in order to test this? I'm unable to receive notifications, and we can't use prod builds until the Web-E PR is deployed, right? |
Web-E was deployed to prod last night so I can now test this with a release build. We're still blocked on merging this though:
This test fails btw. I'm not seeing the profile pic on either Android device -- though part of me thinks that this has been broken for a while. @roryabraham @luacmartins please let me know if there was a better way to run the Android notification test. |
I confirmed that this is currently not working on staging, so I don't think that this is something that was broken by this PR. We should make sure we create a follow-up issue to fix that. |
The PR reviewer was filled out here, but the action to verify that has recently changed (along with the PR reviewer checklist itself, if I'm not mistaken). I'm not going to block on this though since the checklist was filled out and this PR was tested and approved by a C+ and internal engineer. |
@roryabraham looks like this was merged without the checklist test passing. Please add a note explaining why this was done and remove the |
Not an emergency, see last comment |
This is Failing on main
|
Fix here #12884 |
🚀 Deployed to staging by @roryabraham in version: 1.2.30-0 🚀
|
🚀 Deployed to staging by @roryabraham in version: 1.2.30-0 🚀
|
🚀 Deployed to production by @luacmartins in version: 1.2.30-0 🚀
|
@luacmartins @roryabraham This is ready for C+ review payout. Can someone please help here? The linked issue is not an App issue so can't comment there. |
Handled here @mananjadhav #13234 |
Details
Refactors App to use
created
instead oftimestamp
cc @roryabraham
Fixed Issues
$ https://github.com/Expensify/Expensify/issues/240878
Tests
Android-specific tests
QA Steps
Same as test steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionQA steps
sectiontoggleReport
and notonIconClick
)src/languages/*
filesWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
this
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)PR Reviewer Checklist
The reviewer will copy/paste it into a new comment and complete it after the author checklist is completed
### Fixed Issues
section aboveTests
sectionQA steps
sectiontoggleReport
and notonIconClick
).src/languages/*
filesWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
have been tested & I retested again)/** comment above it */
this
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Screenshots
Web
Mobile Web - Chrome
Mobile Web - Safari
Desktop
iOS
Android