-
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
feat: [Auth Violations] Support isDismissed on newDot to hide violations. #54455
base: main
Are you sure you want to change the base?
Conversation
…ons. Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Hmm, not sure why lint warnings increases every time I commit changes. |
report={report} | ||
policy={policy} | ||
/> | ||
), | ||
}; | ||
} | ||
if (TransactionUtils.hasPendingRTERViolation(TransactionUtils.getTransactionViolations(transaction?.transactionID ?? '-1', transactionViolations))) { | ||
if (TransactionUtils.hasPendingRTERViolation(TransactionUtils.getTransactionViolations(transaction?.transactionID))) { |
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.
@rojiphil, could you please check the ESLint failures below? I have already resolved dozens of those but it keeps increasing everytime I push a commit. I want your help especially in resolving the warning just below this comment and when we use ??
while getting a route, I have no idea how to deal with that, please let me know if you are familiar with that. Thanks 🙏🏻
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.
In the case of calling route
, can we not do an early return as shown below? It does not seem correct to call navigate with an invalid report id anyway.
The guidelines for the fix of the ESlint failures is already mentioned here. Please look for similar handling for ESlint failures elsewhere in code as that can help. But let me know if you get stuck otherwise.
if(!reportID) {
return;
}
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.
@rojiphil thanks for the help 🙇🏻.
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
I will fix ESLint issues again today. |
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Finally🥴, all the lint issues are fixed, I'm on my way to record the videos... @rojiphil do you know how to replicate RTER violation? |
@Krishna2323 Is it ready for @rojiphil's review? |
@pecanoro, the code changes are done (new ESLint check issues😬). I was trying to record videos for native platforms, and it was failing on iOS a few days back. I will try again and upload them today. By the way, do you know how to replicate an RTER violation? |
@Krishna2323 This issue is for a different bug but it explains how the RTER violation is triggered: #47511 |
@pecanoro, I think a company card is required to trigger an RTER violation, or is there alternative way? |
@Krishna2323 You can record for the remaining scenarios and during PR review stage we can see how we can ensure validating the fix for RTER violation. Does this help? |
Yeah, we can do that, I will try to find an account that we can use to check if it works for RTER |
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Yeah, sure. I will record the videos today. I'm currently busy with a super high-priority PR and have spent many hours resolving the ESLint issues on this PR, but the warnings increase every time I push a commit🤧. |
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
@rojiphil, you can start reviewing this, recordings have been added. |
@rojiphil Don't forget to get to this when you have some time! |
Explanation of Change
Fixed Issues
$ #53398
PROPOSAL: #53398 (comment)
Tests
Offline tests
QA Steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)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_native.mp4
Android: mWeb Chrome
android_chrome.mp4
iOS: Native
ios_native.mp4
iOS: mWeb Safari
ios_safari.mp4
MacOS: Chrome / Safari
web_chrome.mp4
MacOS: Desktop
desktop_app.mp4