Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix: mentions are not shown in invite user or remove user report #831

Merged
merged 5 commits into from
Feb 10, 2025
Merged

fix: mentions are not shown in invite user or remove user report #831

merged 5 commits into from
Feb 10, 2025

Conversation

daledah
Copy link
Contributor

@daledah daledah commented Jan 24, 2025 •
edited
Loading

This PR fixes the issue that the mention report and mention user sometimes has no quotation marks

Update htmlToTextRules userMention and reportMention regex to work for both <selfClosedTag /> and non self closing tags <openTag></closeTag> to be like htmlToMarkdownRules

Example:

image

Fixed Issues

$ Expensify/App#54630

Tests

  1. What unit/integration tests cover your change? What autoQA tests cover your change?

New test case added

// mention-user
Test string: "<mention-user accountID=1234></mention-user>"
Expected result: "@[email protected]"

Test string: "<mention-user accountID=1234 />"
Expected result: "@[email protected]"

// mention-report
Test string: "<mention-report reportID=1234/>"
Expected result: "#room-name"
  1. What tests did you perform that validates your changed worked?
  • Create a workspace
  • Mention an user that is not invited to the workspace
  • Click "Invite" on the whisper
  • Long press the "Invited ..." message and reply in thread
  • Verify that mention is shown in the thread header

QA

  1. What does QA need to do to validate your changes?
  2. What areas to they need to test for regressions?
    Same as tests

@github-actions GitHub Actions
Copy link

github-actions bot commented Jan 24, 2025 •
edited
Loading

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

@ahmedGaber93 ahmedGaber93 mentioned this pull request Jan 27, 2025
Open
8 tasks
@daledah daledah marked this pull request as ready for review January 31, 2025 03:40
@daledah daledah requested a review from a team as a code owner January 31, 2025 03:40
@daledah
Copy link
Contributor Author

daledah commented Jan 31, 2025

I have read the CLA Document and I hereby sign the CLA

@melvin-bot melvin-bot bot requested a review from neil-marcellini January 31, 2025 03:40
CLABotify added a commit to Expensify/CLA that referenced this pull request Jan 31, 2025
@CLABotify
@daledah has signed the CLA in Expensify/expensify-common#831
eb44508
@melvin-bot melvin-bot bot removed the request for review from a team January 31, 2025 03:40
@neil-marcellini neil-marcellini requested review from nkuoch and removed request for neil-marcellini January 31, 2025 17:45
@neil-marcellini
Copy link
Contributor

Re-assigning to @nkuoch because the proposal is pretty involved and she is the engineer managing this.

@ahmedGaber93
Copy link
Contributor

This PR fixes the issue that the mention report and mention user sometimes has no quotation marks

@daledah please also add this change to PR details

update htmlToTextRules userMention and reportMention regex to work for both <selfClosedTag /> and non self closing tags <openTag></closeTag> to be like htmlToMarkdownRules

ahmedGaber93
ahmedGaber93 reviewed Feb 2, 2025
View reviewed changes
__tests__/ExpensiMark-HTMLToText-test.js
@@ -193,6 +199,9 @@ test('Mention report html to text', () => {

testString = '<mention-report reportID="1234" />';
expect(parser.htmlToText(testString, extras)).toBe('#room-name');

testString = '<mention-report reportID=1234/>';
expect(parser.htmlToText(testString, extras)).toBe('#room-name');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add more test cases here

<mention-report reportID=1234></mention-report>
<mention-report reportID="1234"></mention-report>

@ahmedGaber93 ahmedGaber93 mentioned this pull request Feb 2, 2025
Open
50 tasks
@daledah
Copy link
Contributor Author

daledah commented Feb 6, 2025

@ahmedGaber93 i updated

ahmedGaber93
ahmedGaber93 reviewed Feb 6, 2025
View reviewed changes
__tests__/ExpensiMark-HTMLToText-test.js Outdated
'1234': '[email protected]',
1234: '[email protected]',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain why this change? I think it is not affected

ahmedGaber93
ahmedGaber93 reviewed Feb 6, 2025
View reviewed changes
__tests__/ExpensiMark-HTMLToText-test.js Outdated
Comment on lines 200 to 216
const testString2 = '<video data-expensify-source="https://www.expensify.com/chat-attachments/123/test.mp4" data-expensify-width=720 data-expensify-height=1640 data-expensify-duration=20>test.mp4</video>';
const testString2 =
'<video data-expensify-source="https://www.expensify.com/chat-attachments/123/test.mp4" data-expensify-width=720 data-expensify-height=1640 data-expensify-duration=20>test.mp4</video>';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this change is not related with this PR

@daledah
Copy link
Contributor Author

daledah commented Feb 7, 2025

@ahmedGaber93 I updated. My linter was acting on its own.

ahmedGaber93
ahmedGaber93 approved these changes Feb 7, 2025
View reviewed changes
Copy link
Contributor

@ahmedGaber93 ahmedGaber93 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@ahmedGaber93
Copy link
Contributor

ahmedGaber93 commented Feb 7, 2025 •
edited
Loading

@nkuoch I just review the changes but not tested the package with E/App (we will do it in Expensify/App#55699 when bump expensify-common), I think the test cases is enough here. Please let me know if you want me test it in E/App. Thanks!

@nkuoch nkuoch merged commit 9d4c74d into Expensify:main Feb 10, 2025
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nkuoch nkuoch nkuoch approved these changes

@ahmedGaber93 ahmedGaber93 ahmedGaber93 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@daledah @neil-marcellini @ahmedGaber93 @nkuoch