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

[Payment due][$250] Centralize ExpensiMark usage with a dedicated Parser module #44451

Closed
blazejkustra opened this issue Jun 26, 2024 · 15 comments
Assignees
Labels
AutoAssignerNewDotQuality Used to assign quality issues to engineers Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor

Comments

@blazejkustra
Copy link
Contributor

blazejkustra commented Jun 26, 2024

Proposal:

  • Create a Parser lib, where ExpensiMark is going to be imported and initialized only once
  • Forbid importing ExpensiMark from expensify-common globally (can be easily done with ESLint)
  • (optional) Pass Onyx context with a new ExpensiMark method instead of wrapping parseHtmlToText, parseHtmlToMarkdown like in src/libs/OnyxAwareParser.ts.

This way, there will be only one instance of ExpensiMark, and src/libs/Parser will be used throughout the codebase. This will potentially prevent regressions like this going forward.

Problems

  1. ExpensiMark methods (parseHtmlToText, parseHtmlToMarkdown) should always be used with Onyx context. Otherwise server is spammed with a lot of errors like this.
  2. We introduced OnyxAwareParser a while ago, but we can't expect everybody to know it's mandatory to use it. Instead contributors often import ExpensiMark, initialize it and use it's methods, which causes problems (like here).
  3. ExpensiMark is initialized 14 times in E/App.
Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01ddda297d9f5526e7
  • Upwork Job ID: 1813359382413182920
  • Last Price Increase: 2024-07-16
  • Automatic offers:
    • hoangzinh | Reviewer | 103148367
Issue OwnerCurrent Issue Owner: @
Issue OwnerCurrent Issue Owner: @kadiealexander
@blazejkustra blazejkustra added Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 labels Jun 26, 2024
Copy link

melvin-bot bot commented Jun 26, 2024

Triggered auto assignment to @kadiealexander (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

@mountiny mountiny added the AutoAssignerNewDotQuality Used to assign quality issues to engineers label Jun 26, 2024
Copy link

melvin-bot bot commented Jun 26, 2024

Current assignee @neil-marcellini is eligible for the AutoAssignerNewDotQuality assigner, not assigning anyone new.

@ShridharGoel
Copy link
Contributor

Create a Parser lib, where ExpensiMark is going to be imported and initialized only once

Can we not use the existing OnyxAwareParser.ts with the needed modifications ?

@blazejkustra
Copy link
Contributor Author

@ShridharGoel I think I'll reuse OnyxAwareParser.ts, but change the name to Parser to make it less verbose.

@blazejkustra
Copy link
Contributor Author

I'll come back to this issue early next week as I'm OOO 27-30.06.

@neil-marcellini
Copy link
Contributor

Not overdue, waiting for Blazej to come back.

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Jun 28, 2024
@blazejkustra
Copy link
Contributor Author

I'm on it, will update tomorrow :)

@melvin-bot melvin-bot bot removed the Overdue label Jul 1, 2024
@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Jul 2, 2024
@blazejkustra
Copy link
Contributor Author

Update: PR is ready for C+ review!

@hoangzinh
Copy link
Contributor

@kadiealexander @neil-marcellini can you help to assign me and apply payment status here? I helped review this PR #44732. Thank you.

@kadiealexander kadiealexander added the External Added to denote the issue can be worked on by a contributor label Jul 16, 2024
@melvin-bot melvin-bot bot changed the title Centralize ExpensiMark usage with a dedicated Parser module [$250] Centralize ExpensiMark usage with a dedicated Parser module Jul 16, 2024
Copy link

melvin-bot bot commented Jul 16, 2024

Job added to Upwork: https://www.upwork.com/jobs/~01ddda297d9f5526e7

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Jul 16, 2024
Copy link

melvin-bot bot commented Jul 16, 2024

Current assignee @hoangzinh is eligible for the External assigner, not assigning anyone new.

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Jul 16, 2024
@kadiealexander kadiealexander added Weekly KSv2 and removed Daily KSv2 Help Wanted Apply this label when an issue is open to proposals by contributors labels Jul 16, 2024
Copy link

melvin-bot bot commented Jul 16, 2024

📣 @hoangzinh 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job

@hoangzinh
Copy link
Contributor

@kadiealexander I think we can apply "Awaiting Payment" label for this issue, the PR has been deployed to Production #44732 (comment)

@neil-marcellini neil-marcellini added Awaiting Payment Auto-added when associated PR is deployed to production and removed Reviewing Has a PR in review labels Jul 27, 2024
@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Jul 27, 2024
@kadiealexander
Copy link
Contributor

kadiealexander commented Jul 29, 2024

Payouts due:

Upwork job is here.

@kadiealexander kadiealexander changed the title [$250] Centralize ExpensiMark usage with a dedicated Parser module [Payment due][$250] Centralize ExpensiMark usage with a dedicated Parser module Jul 29, 2024
@kadiealexander kadiealexander added Daily KSv2 and removed Weekly KSv2 labels Jul 29, 2024
@hoangzinh
Copy link
Contributor

@kadiealexander This issue is just a refactoring issue, therefore I think we don't have a BZ checklist.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AutoAssignerNewDotQuality Used to assign quality issues to engineers Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor
Projects
Development

No branches or pull requests

6 participants