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

convert html to flat text for LHN uses #440

Merged
merged 7 commits into from
Apr 27, 2022

Conversation

mollfpr
Copy link
Contributor

@mollfpr mollfpr commented Apr 19, 2022

@parasharrajat @Luke9389 will you please review this?

[Explanation of the change or anything fishy that is going on]
I'm adding a new function to replace the HTML report message that use for showing text on LHN.

Fixed Issues

$ Expensify/App#8134

Tests

  1. What unit/integration tests cover your change? What autoQA tests cover your change?
    __tests__/ExpensiMark-FlatText-test.js
  2. What tests did you perform that validates your changed worked?
> Confusing stuff
Tell me about it

Type above text and should be converted to text Confusing stuff Tell me about it

QA

  1. What does QA need to do to validate your changes?
    Verify that __tests__/ExpensiMark-FlatText-test.js is pass.
  2. What areas to they need to test for regressions?
    The function created is used on showing the converted html last message on LHN.

@mollfpr mollfpr requested a review from a team as a code owner April 19, 2022 06:05
@github-actions
Copy link

github-actions bot commented Apr 19, 2022

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

@melvin-bot melvin-bot bot requested review from Luke9389 and removed request for a team April 19, 2022 06:05
@mollfpr
Copy link
Contributor Author

mollfpr commented Apr 19, 2022

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

@mollfpr
Copy link
Contributor Author

mollfpr commented Apr 19, 2022

recheck

lib/ExpensiMark.js Outdated Show resolved Hide resolved
lib/ExpensiMark.js Outdated Show resolved Hide resolved
lib/ExpensiMark.js Outdated Show resolved Hide resolved
mollfpr and others added 3 commits April 20, 2022 00:50
Co-authored-by: Rajat Parashar <parasharrajat@users.noreply.github.com>
Co-authored-by: Rajat Parashar <parasharrajat@users.noreply.github.com>
Co-authored-by: Rajat Parashar <parasharrajat@users.noreply.github.com>
const html = '1<br />2<br />3';
const text = '1 2 3';

expect(parser.htmlToFlatText(html)).toBe(text);
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be updated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh yeah forgot about that, I'm on it.

lib/ExpensiMark.js Outdated Show resolved Hide resolved
lib/ExpensiMark.js Outdated Show resolved Hide resolved
mollfpr and others added 2 commits April 20, 2022 03:36
Co-authored-by: Rajat Parashar <parasharrajat@users.noreply.github.com>
@mollfpr
Copy link
Contributor Author

mollfpr commented Apr 21, 2022

Any things I should update here?

@parasharrajat
Copy link
Member

cc: @Luke9389

{
name: 'blockquote',
regex: /<\/blockquote>/gm,
replacement: ' ',
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm surprised the tests with blockquotes work, considering that the

const html = '<blockquote>Confusing stuff</blockquote>Tell me about it';
const text = 'Confusing stuff Tell me about it';

Why isn't there a space before the word "Confusing" here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, it seems we're only capturing the closing tag. I must be wrong about that though because the tests are passing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why isn't there a space before the word "Confusing" here?

Also, it seems we're only capturing the closing tag. I must be wrong about that though because the tests are passing.

Here we loop the 3 rules

  1. replacing breakline with space /((<br[^>]*>)+)/gi
  2. replacing blockquote with space /<\/blockquote>/gm. This the solution for the issue
  3. strip all the tags left behind with an empty string /(<([^>]+)>)/gi
        this.htmlToTextRules.forEach((rule) => {
            replacedText = replacedText.replace(rule.regex, rule.replacement);
        });

@parasharrajat @Luke9389

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahh, ok, I see. I didn't realize we apply all rules in a row.

thanks for the explanation!

@s77rt
Copy link
Contributor

s77rt commented May 25, 2023

This PR caused a bug Expensify/App#17658. The html to text function should have also unescaped html entities e.g. &lt; to <.

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.

4 participants