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

[HOLD for payment 2023-10-05] [$500] Dev: Web - Trailing Newlines are not trimmed on the My Note page. #27596

Closed
1 of 6 tasks
kbecciv opened this issue Sep 16, 2023 · 28 comments
Closed
1 of 6 tasks
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Daily KSv2 External Added to denote the issue can be worked on by a contributor

Comments

@kbecciv
Copy link

kbecciv commented Sep 16, 2023

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Action Performed:

  1. Open the App and Create a workspace. If not created
  2. Go to #admins
  3. Click on header > Private notes > My note
  4. Press on Notes
  5. Add 3-4 newlines before the actual message; Press Save

Expected Result:

Trailing Newlines should be trimmed on the My Note page.

Actual Result:

Trailing Newlines are not trimmed on the My Note page, and it shows the empty lines on it.

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

  • Android / native
  • Android / Chrome
  • iOS / native
  • iOS / Safari
  • MacOS / Chrome / Safari
  • MacOS / Desktop

Version Number: Dev 1.3.70.5
Reproducible in staging?: n
Reproducible in production?: n
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation

empty-newlines.mov

Expensify/Expensify Issue URL:
Issue reported by: @jayeshmangwani
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1694787278356809

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01a1c9388800197ffc
  • Upwork Job ID: 1703123699888500736
  • Last Price Increase: 2023-09-16
  • Automatic offers:
    • jayeshmangwani | Contributor | 26743629
    • jayeshmangwani | Reporter | 26743630
@kbecciv kbecciv added the External Added to denote the issue can be worked on by a contributor label Sep 16, 2023
@ahmedGaber93
Copy link
Contributor

Proposal

Please re-state the problem that we are trying to solve in this issue.

Dev - Trailing Newlines are not trimmed on the My Note page.

What is the root cause of that problem?

we trim the privateNote only on amount in useState defaul value

const [privateNote, setPrivateNote] = useState(parser.htmlToMarkdown(lodashGet(report, ['privateNotes', route.params.accountID, 'note'], '')).trim());

and not trim it when save

const savePrivateNote = () => {
const editedNote = parser.replace(privateNote);
Report.updatePrivateNotes(report.reportID, route.params.accountID, editedNote);
Keyboard.dismiss();
// Take user back to the PrivateNotesView page
Navigation.goBack();
};

What changes do you think we should make in order to solve the problem?

we need to trim the privateNote before saving

const editedNote = parser.replace(privateNote.trim()); 

const savePrivateNote = () => {
const editedNote = parser.replace(privateNote);
Report.updatePrivateNotes(report.reportID, route.params.accountID, editedNote);
Keyboard.dismiss();
// Take user back to the PrivateNotesView page
Navigation.goBack();
};

What alternative solutions did you explore? (Optional)

N/A

@melvin-bot melvin-bot bot changed the title Dev: Web - Trailing Newlines are not trimmed on the My Note page. [$500] Dev: Web - Trailing Newlines are not trimmed on the My Note page. Sep 16, 2023
@melvin-bot
Copy link

melvin-bot bot commented Sep 16, 2023

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

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

melvin-bot bot commented Sep 16, 2023

Triggered auto assignment to @garrettmknight (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

@melvin-bot melvin-bot bot added the Daily KSv2 label Sep 16, 2023
@melvin-bot
Copy link

melvin-bot bot commented Sep 16, 2023

Triggered auto assignment to Contributor-plus team member for initial proposal review - @burczu (External)

@kbecciv
Copy link
Author

kbecciv commented Sep 16, 2023

Proposal by: @jayeshmangwani
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1694787278356809

Proposal

Please re-state the problem that we are trying to solve in this issue.

Trailing Newlines are not trimmed on the My Note page.

What is the root cause of that problem?

We are not trimming Private Note Value on Private Notes page, before passing the privateNote value to the API

What changes do you think we should make in order to solve the problem?

We should trim the privateNote Value here before passing to the API

const savePrivateNote = () => {
const editedNote = parser.replace(privateNote);

    const editedNote = parser.replace(privateNote.trim());

What alternative solutions did you explore? (Optional)

none

@jayeshmangwani
Copy link
Contributor

Proposal

Please re-state the problem that we are trying to solve in this issue.

Trailing Newlines are not trimmed on the My Note page.

What is the root cause of that problem?

We are not trimming Private Note Value on Private Notes page, before passing the privateNote value to the API

What changes do you think we should make in order to solve the problem?

We should trim the privateNote Value here before passing to the API

const savePrivateNote = () => {
const editedNote = parser.replace(privateNote);

const editedNote = parser.replace(privateNote.trim());

Note This was my proposal that I had added on Slack, so I am commenting on GH, too.

What alternative solutions did you explore? (Optional)

none

@burczu
Copy link
Contributor

burczu commented Sep 18, 2023

I've just reviewed all the proposals - they are all the same (and correct), but in fact the one from @jayeshmangwani was the first (he posted in the slack thread first, then @kbecciv posted it for him here, and then he posted it here himself), so we should proceed with it.

🎀 👀 🎀 C+ reviewed

@melvin-bot
Copy link

melvin-bot bot commented Sep 18, 2023

Triggered auto assignment to @MariaHCD, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Sep 19, 2023
@melvin-bot
Copy link

melvin-bot bot commented Sep 19, 2023

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

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

@melvin-bot
Copy link

melvin-bot bot commented Sep 19, 2023

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

Offer link
Upwork job

@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Sep 19, 2023
@jayeshmangwani
Copy link
Contributor

PR is ready for review
cc: @burczu

@melvin-bot
Copy link

melvin-bot bot commented Sep 21, 2023

📣 @iamjey6! 📣
Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork.
Please follow these steps:

  1. Make sure you've read and understood the contributing guidelines.
  2. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
  3. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
  4. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.
    Screen Shot 2022-11-16 at 4 42 54 PM
    Format:
Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

@jayeshmangwani
Copy link
Contributor

@burczu friendly bump for review

@melvin-bot
Copy link

melvin-bot bot commented Sep 25, 2023

Based on my calculations, the pull request did not get merged within 3 working days of assignment. Please, check out my computations here:

  • when @jayeshmangwani got assigned: 2023-09-19 01:14:44 Z
  • when the PR got merged: 2023-09-25 01:38:50 UTC
  • days elapsed: 4

On to the next one 🚀

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Sep 28, 2023
@melvin-bot
Copy link

melvin-bot bot commented Sep 28, 2023

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.3.74-3 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2023-10-05. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

  • External issue reporter
  • Contributor that fixed the issue
  • Contributor+ that helped on the issue and/or PR

For reference, here are some details about the assignees on this issue:

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

  • Merged PR within 3 business days of assignment - 50% bonus
  • Merged PR more than 9 business days after assignment - 50% penalty

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Oct 5, 2023
@jayeshmangwani
Copy link
Contributor

@garrettmknight today payment will released so I want to ask if the urgency bonus be considered ?, though PR merged on 4th day but PR was raised after few hours I've assigned but C+ reviewed on 4th day, TIA

Assigned 6:44 AM
Screenshot 2023-10-05 at 11 48 25 PM

PR raised 11:32 AM , Same day
Screenshot 2023-10-05 at 11 48 39 PM

@melvin-bot melvin-bot bot added the Overdue label Oct 9, 2023
@melvin-bot
Copy link

melvin-bot bot commented Oct 9, 2023

@garrettmknight, @burczu, @MariaHCD, @jayeshmangwani Whoops! This issue is 2 days overdue. Let's get this updated quick!

@garrettmknight
Copy link
Contributor

@jayeshmangwani the urgency bonus is based on the PR getting merged so this one didn't make it.

Summary of payments:

Upwork job: https://www.upwork.com/jobs/~01a1c9388800197ffc

@kbecciv
Copy link
Author

kbecciv commented Oct 14, 2023

Issue is reproducible on build 1.3.84.0

Recording.4987.mp4

@kbecciv kbecciv reopened this Oct 14, 2023
@melvin-bot melvin-bot bot added the Overdue label Oct 14, 2023
@jayeshmangwani
Copy link
Contributor

@kbecciv Video you have attached is an expected behavior; we do not trim new lines between the notes. We trim if the note message has new lines at the start or end of the message

@melvin-bot melvin-bot bot removed the Overdue label Oct 14, 2023
@namhihi237
Copy link
Contributor

namhihi237 commented Oct 14, 2023

@jayeshmangwani This attached is different a bit with this issue when we click edit again it is still not trimmed, I think it's another issue and caused by another PR

@namhihi237
Copy link
Contributor

Can see details here

Screen.Recording.2023-10-14.at.21.21.07.mov

@melvin-bot
Copy link

melvin-bot bot commented Oct 14, 2023

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

@melvin-bot melvin-bot bot added the Overdue label Oct 16, 2023
@melvin-bot
Copy link

melvin-bot bot commented Oct 18, 2023

@garrettmknight, @burczu, @MariaHCD, @jayeshmangwani Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

@jayeshmangwani
Copy link
Contributor

@namhihi237 is right here; this is another issue caused by another PR. It used to work during existng issue's PR merge, but resurfaced after this PR. It looks like this is a regression from this issue.

cc: @burczu @MariaHCD

@MariaHCD
Copy link
Contributor

Okay, I don't have the most context on if that other issue caused this issue so I've asked here: #28599 (comment)

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Oct 23, 2023
@MariaHCD
Copy link
Contributor

Looks like it is indeed a regression from the other issue: #28599 (comment)

I think we can close this one out.

@melvin-bot melvin-bot bot removed the Overdue label Oct 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Daily KSv2 External Added to denote the issue can be worked on by a contributor
Projects
None yet
Development

No branches or pull requests

7 participants