Update UI to show fraud states#29680
Merged
Merged
Conversation
pasyukevich
force-pushed
the
feature/show-fraud-states
branch
3 times, most recently
from
2c69338 to
d13e96c
Compare
pasyukevich
force-pushed
the
feature/show-fraud-states
branch
from
8d6022c to
5fec082
Compare
melvin-bot
Bot
requested review from
eVoloshchak
and removed request for
a team
|
@eVoloshchak 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] |
grgia
reviewed
Oct 19, 2023
|
|
||
| // Additional styles to apply to the text | ||
| // eslint-disable-next-line react/forbid-prop-types | ||
| textStyle: PropTypes.arrayOf(PropTypes.object), |
Contributor
There was a problem hiding this comment.
could we rename this textStyles as it's an array?
| setShouldShowCardDetails(true); | ||
| }; | ||
|
|
||
| const isDetectedDomainFraud = _.some(domainCards, (card) => card.fraud === CONST.EXPENSIFY_CARD.FRAUD_TYPES.DOMAIN); |
Contributor
There was a problem hiding this comment.
I think these read better:
hasDetectedDomainFraud
hasDetectedIndividualFraud
| titleStyle={styles.newKansasLarge} | ||
| /> | ||
| {!_.isEmpty(virtualCard) && ( | ||
| {isDetectedDomainFraud && ( |
Contributor
There was a problem hiding this comment.
Let's use the Boolean ? () : null pattern instead of && for these.
| titleStyle={styles.walletCardMenuItem} | ||
| /> | ||
|
|
||
| {!isDetectedDomainFraud && ( |
Contributor
There was a problem hiding this comment.
Is there a way we could default to this without depending on the fraud variables? Since this the core of this page
Contributor
Author
There was a problem hiding this comment.
Due to the requirements, we need to hide this block in case of detected domain fraud
eVoloshchak
suggested changes
Oct 19, 2023
|
|
||
| // Additional styles to apply to the text | ||
| // eslint-disable-next-line react/forbid-prop-types | ||
| textStyle: PropTypes.arrayOf(PropTypes.object), |
Contributor
There was a problem hiding this comment.
We can use stylePropTypes for this
| textStyle: [], | ||
| }; | ||
|
|
||
| function DotIndicatorMessage(props) { |
Contributor
There was a problem hiding this comment.
Not directly related to this PR, but since we're adding changes to this component, let's use prop destructuring, it's documented as a best practice now
// Bad
function UserInfo(props) {
return (
<View>
<Text>Name: {props.name}</Text>
<Text>Email: {props.email}</Text>
</View>
}
UserInfo.defaultProps = {
name: 'anonymous';
}
// Good
function UserInfo({ name = 'anonymous', email }) {
return (
<View>
<Text>Name: {name}</Text>
<Text>Email: {email}</Text>
</View>
);
}
Contributor
Author
There was a problem hiding this comment.
I'm not sure that we need to update this in the scope of the current task
I believe that the best way is to apply it with typescript migration
| shouldShowRightIcon | ||
| onPress={() => Navigation.navigate(ROUTES.SETTINGS_REPORT_FRAUD.getRoute(domain))} | ||
| brickRoadIndicator="error" | ||
| onPress={() => Link.openOldDotLink('inbox')} |
Contributor
There was a problem hiding this comment.
We should leave a note that this won't work in DEV and staging too, only in production (I wonder if buildOldDotURL is supposed to add environment to URL, since there is no dev/staging version of oldDot)
Screen.Recording.2023-10-19.at.17.06.52.mov
Contributor
There was a problem hiding this comment.
We should leave a note that this won't work in DEV and staging too, only in production (I wonder if buildOldDotURL is supposed to add environment to URL, since there is no dev/staging version of oldDot)
@0xmiroslav Internal employees do have a DEV/staging Old Dot
Contributor
There was a problem hiding this comment.
I guess I'll have to change the profile pic, this isn't the first time this happens 😅
eVoloshchak
suggested changes
Oct 20, 2023
| style: PropTypes.arrayOf(PropTypes.object), | ||
|
|
||
| // Additional styles to apply to the text | ||
| // eslint-disable-next-line react/forbid-prop-types |
Contributor
There was a problem hiding this comment.
Suggested change
| // eslint-disable-next-line react/forbid-prop-types |
Contributor
Reviewer Checklist
Screenshots/VideosWebScreen.Recording.2023-10-20.at.18.26.32.movMobile Web - Chromescreen-20231020-182140.mp4Mobile Web - SafariScreen.Recording.2023-10-20.at.18.18.15.movDesktopScreen.Recording.2023-10-20.at.18.25.10.moviOSScreen.Recording.2023-10-20.at.18.17.14.movAndroidscreen-20231020-182033.mp4 |
eVoloshchak
suggested changes
Oct 20, 2023
eVoloshchak
left a comment
eVoloshchak
left a comment
Contributor
There was a problem hiding this comment.
Looks and tests good, but there are a couple of propType warnings
- Invalid prop
cardList.234523452345.fraudof valueindividualsupplied toExpensifyCardPage, expected one of ["domain",null,"none"].
This line is at fault it seems - Update test data with fraud: "domain" for one of the cards, press on the card, observe the following propType warning:
Invalid prop `messages` of type `array` supplied to `DotIndicatorMessage`, expected an object
We should replacemessages={[translate('cardPage.cardLocked')]}withmessages={{0: translate('cardPage.cardLocked')}} - Not sure if this one is related to the PR, but when you enter wallet page on native platforms (iOS/Android), the ‘VirtualizedLists should never be nested inside plain ScrollViews’ warning pops up
Screenshot
Contributor
Author
|
@eVoloshchak The VirtualizedLists is not directly related to this PR since there are no structure changes on the wallet page, only the adding of new prop |
grgia
approved these changes
Oct 23, 2023
Contributor
|
@eVoloshchak before I merge, can you confirm that you tested that the latest branch has no errors on all platforms |
Contributor
|
@grgia, merged the latest main and re-tested, no errors on all platforms |
Contributor
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
43 tasks
Contributor
|
🚀 Deployed to staging by https://github.com/grgia in version: 1.3.90-0 🚀
|
99 tasks
Contributor
|
🚀 Deployed to production by https://github.com/marcaaron in version: 1.3.90-2 🚀
|
Contributor
|
🚀 Deployed to staging by https://github.com/grgia in version: 1.3.91-0 🚀
|
Contributor
|
🚀 Deployed to production by https://github.com/marcaaron in version: 1.3.91-8 🚀
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Details
Fixed Issues
$ #22881
PROPOSAL:
Tests
1.Update the getComponent for SCREENS.SETTINGS.WALLET, in SettingsModalStackNavigator to import the component from ../../../pages/settings/Wallet/WalletPage/WalletPage
2. Initialise cardList with mock data:
review the transactionbutton (by pressing it user’s old dot inbox should be opened)fraud: "domain"for one of the cards"Your card is temporarily locked while our team reviews your company's account."the message showed with a red dot and all other elements are hiddenOffline tests
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
native-androidp1.webm
native-androidp2.webm
Android: mWeb Chrome
web-android.mp4
iOS: Native
native-ios.mp4
iOS: mWeb Safari
web-ios.mp4
MacOS: Chrome / Safari
web-desktop.mp4
MacOS: Desktop
desktop.mp4