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.

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

Modify mention user and mention report extras field names in ExpensiMark htmlToText and htmlToMarkdown rules #708

Merged
merged 5 commits into from
May 28, 2024

Conversation

tsa321
Copy link
Contributor

@tsa321 tsa321 commented May 28, 2024

This PR changes field name of extras in ExpensiMark to fix type check fails.
Modify mention user and mention report extras field names in ExpensiMark htmlToText and htmlToMarkdown rules

Fixed Issues

$ Expensify/App#40928

Tests

  1. What unit/integration tests cover your change? What autoQA tests cover your change?
    typecheck on expensify/App should run successfully

  2. What tests did you perform that validates your changed worked?
    I have tested it on my other PR, thread report from report action that contains mention room and mention user should display report name properly in header report and in LHN.

QA

  1. What does QA need to do to validate your changes?
    N/A

  2. What areas to they need to test for regressions?
    N/A

@tsa321 tsa321 requested a review from a team as a code owner May 28, 2024 02:37
Copy link

github-actions bot commented May 28, 2024

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

@melvin-bot melvin-bot bot requested review from thienlnam and removed request for a team May 28, 2024 02:37
@tsa321
Copy link
Contributor Author

tsa321 commented May 28, 2024

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

Signed-off-by: Tsaqif <[email protected]>
Copy link
Contributor

@mollfpr mollfpr left a comment

Choose a reason for hiding this comment

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

It seems we have a test failure.

Screenshot 2024-05-28 at 17 32 06

I'll be expecting the changes to minimal as it could so we don't break anything here.

return `@${extras.accountIdToName[g1]}`;
return `@${accountToNameMap[g1]}`;
}
return Str.removeSMSDomain(g2)
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the purpose of these changes?

Copy link
Contributor

Choose a reason for hiding this comment

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

+1

Copy link
Contributor Author

@tsa321 tsa321 May 28, 2024

Choose a reason for hiding this comment

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

The failling tests are because I didn't changes the fields in the test files yet, I am sorry.

For accountToNameMap, I am following the pattern in the file for reportToNameMap:

return reportToNameMap[g1];

accountToNameMap is just a variable that store extras.accountIdToName. It is the same thing:

const accountToNameMap = extras.accountIdToName;

Should I revert it?

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 we can revert it.

@tsa321
Copy link
Contributor Author

tsa321 commented May 28, 2024

@mollfpr The failling tests are because I didn't changes the fields in the test files yet, I am sorry. Should be fine now...

@tsa321 tsa321 requested a review from mollfpr May 28, 2024 13:53
@tsa321
Copy link
Contributor Author

tsa321 commented May 28, 2024

@mollfpr I have reverted the changes.

Copy link
Contributor

@mollfpr mollfpr left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@luacmartins luacmartins merged commit ea61a12 into Expensify:main May 28, 2024
6 checks passed
Copy link

🚀Published to npm in v2.0.3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants