Conditional HTML parsing for Displaynames and HTML parsing for threadheaders #74254
Conversation
|
@QichenZhu 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] |
|
All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
|
LGTM 👍 Thank you for your hard work! |
QichenZhu
left a comment
There was a problem hiding this comment.
Good job! Two minor comments:
joekaufmanexpensify
left a comment
There was a problem hiding this comment.
Fine from a product perspective
left over from refactor comment is no longer accurate Co-authored-by: Qichen Zhu <zhuqc9@gmail.com>
refactoring based upon correct pattern usage Co-authored-by: Qichen Zhu <zhuqc9@gmail.com>
|
@QichenZhu Thanks for your support going through this process happy you approve of the work. If possible could someone give a review on Upwork? |
Reviewer Checklist
Screenshots/Videos |
Codecov Report❌ Patch coverage is
... and 288 files with indirect coverage changes 🚀 New features to boost your workflow:
|
…g github action processes
|
Prettier failures. |
…ParseHTMLTest.tsx
|
@carlosmiceli I ran the script and pushed the changes, those should be cleared up now. |
|
✋ 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/carlosmiceli in version: 9.2.48-0 🚀
|
|
Could we please update the PR title to make it more suggestive, giving a glimpse of the code changes? |
|
@whiletrace seems like your PR caused this regression, would you be able to fix it quickly today perhaps? And good call by Cristi above to improve the title of this PR as well. cc @brunovjk |
|
@carlosmiceli can fix it today, i can get to it about 15:00 Pacific Standard time today. |
|
@whiletrace We decided to revert, feel free to submit a new PR for the original issue covering for the newfound bug, thank you! |
|
@carlosmiceli okay ill have 👀 on it this afternoon @QichenZhu any feedback for going forward. this didn't pop up in initial testing. Any other related issues this may regess? |
@whiletrace, I think we should fix it here.
Yeah, it's hard to catch edge cases like that unless we were already aware of them.
You can test similar scenarios. I already tested moving an expense to a workspace report and it doesn't have the issue. |
|
@QichenZhu after looking at this I concur: if (isActionOfType(parentReportAction,
CONST.REPORT.ACTIONS.TYPE.UNREPORTED_TRANSACTION)) {
return Parser.htmlToText(getUnreportedTransactionMessage());
}I am not seeing other unescaped HTML that has the potential to create a thread |
|
🚀 Deployed to production by https://github.com/cristipaval in version: 9.2.48-1 🚀
|
|
@cristipaval @QichenZhu @carlosmiceli |














Explanation of Change
Fixed Issues
$ #68721 (comment)
PROPOSAL: #68721 (comment)
Tests
2 click actions button on left of screen marked +
Offline tests
2 click actions button on left of screen marked +
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
Same as tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.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
android.mp4
Android: mWeb Chrome
compressed.mp4
iOS: Native
ioscomressed.mp4
iOS: mWeb Safari
safari-mobileweb.mp4
MacOS: Chrome / Safari
chrome.mp4
MacOS: Desktop
macappcompressed.mp4