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 2022-12-02] [$250] Link with trailing spaces doesn't got trimmed reported by @kerupuksambel #12511

Closed
kavimuru opened this issue Nov 7, 2022 · 41 comments
Assignees
Labels
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

@kavimuru
Copy link

kavimuru commented Nov 7, 2022

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 staging.new.expensify.com
  2. Send a link on Markdown with trailing spaces for its URL title (for example, test )

Expected Result:

The URL title should be trimmed

Actual Result:

The URL title isn't trimmed

Workaround:

unknown

Platform:

Where is this issue occurring?

  • Web

Version Number: 1.2.24-0
Reproducible in staging?: y
Reproducible in production?: y
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos:

Screenshot_2022-11-06_20-33-29
3

Expensify/Expensify Issue URL:
Issue reported by: @kerupuksambel
Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1667742263937549

View all open jobs on GitHub

@kavimuru kavimuru added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Nov 7, 2022
@melvin-bot
Copy link

melvin-bot bot commented Nov 7, 2022

Triggered auto assignment to @abekkala (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

@0xmiros
Copy link
Contributor

0xmiros commented Nov 7, 2022

Proposal

https://github.com/Expensify/expensify-common/blob/805a4c34debc7e27b14e33847ac4df4d59b6d878/lib/ExpensiMark.js#L68-L92

-                   return `<a href="${g3 && g3.includes('http') ? '' : 'https://'}${g2}" target="_blank" rel="noreferrer noopener">${g1}</a>`;
+                   return `<a href="${g3 && g3.includes('http') ? '' : 'https://'}${g2}" target="_blank" rel="noreferrer noopener">${g1.trim()}</a>`;

@abekkala
Copy link
Contributor

abekkala commented Nov 7, 2022

I was able to reproduce.

@abekkala abekkala added the External Added to denote the issue can be worked on by a contributor label Nov 7, 2022
@melvin-bot
Copy link

melvin-bot bot commented Nov 7, 2022

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

@melvin-bot
Copy link

melvin-bot bot commented Nov 7, 2022

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Nov 7, 2022
@melvin-bot
Copy link

melvin-bot bot commented Nov 7, 2022

Triggered auto assignment to @neil-marcellini (External), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

@melvin-bot melvin-bot bot changed the title Link with trailing spaces doesn't got trimmed reported by @kerupuksambel [$250] Link with trailing spaces doesn't got trimmed reported by @kerupuksambel Nov 7, 2022
@rushatgabhane
Copy link
Member

@neil-marcellini i recommend @0xmiroslav's proposal.

🎀 👀 🎀 C+ reviewed

@neil-marcellini
Copy link
Contributor

@neil-marcellini i recommend @0xmiroslav's proposal.

🎀 👀 🎀 C+ reviewed

I agree, it looks good. We will also need to update the package.json once the expensify-common PR is merged.

"expensify-common": "git+https://github.com/Expensify/expensify-common.git#6010b3c49a16f63ee9cecc0c720f6d10395775ef",

@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Nov 9, 2022
@melvin-bot
Copy link

melvin-bot bot commented Nov 9, 2022

📣 @0xmiroslav You have been assigned to this job by @neil-marcellini!
Please apply to this job in Upwork 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 📖

@abekkala
Copy link
Contributor

@neil-marcellini can I confirm that we're waiting for the proposal in Upwork before I can do the hiring task?

@neil-marcellini
Copy link
Contributor

I think you should be on this SO step?

@abekkala
Copy link
Contributor

Ah got it.
The three have been hired via Upwork

@melvin-bot
Copy link

melvin-bot bot commented Nov 14, 2022

@abekkala, @neil-marcellini, @rushatgabhane, @0xmiroslav Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

@melvin-bot melvin-bot bot added the Overdue label Nov 14, 2022
@Gonals Gonals self-assigned this Nov 14, 2022
@abekkala
Copy link
Contributor

@neil-marcellini do yo know why this hasn't auto-populated a payment date in the GH title?

@melvin-bot melvin-bot bot removed the Overdue label Nov 14, 2022
@neil-marcellini
Copy link
Contributor

@abekkala That webhook only triggers when there's a new release of the App. Actually that reminds me that we still need to update the package.json in app to use the update version of expensify-common as I mentioned here. @0xmiroslav could you please put up a PR for that update?

Sorry about that April. We should only pay after the App PR is merged. Once it is merged the "HOLD for payment" text and the date will show in the title. All you need to do is wait until that date to issue payments.

@melvin-bot melvin-bot bot added the Overdue label Nov 21, 2022
@neil-marcellini
Copy link
Contributor

The last PR we need got merged and will be deployed soon.

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Nov 22, 2022
@melvin-bot
Copy link

melvin-bot bot commented Nov 25, 2022

@abekkala, @Gonals, @neil-marcellini, @rushatgabhane, @0xmiroslav Whoops! This issue is 2 days overdue. Let's get this updated quick!

@neil-marcellini
Copy link
Contributor

neil-marcellini commented Nov 25, 2022

The App PR was deployed to production. I'm not sure why the checklist didn't post automatically so here it is.

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

  • [@neil-marcellini / @rushatgabhane] The PR that introduced the bug has been identified. Link to the PR:
    N/A this was not a regression, it was overlooked during the initial implementation.
  • [@neil-marcellini / @rushatgabhane] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
    N/A same as above.
  • [@neil-marcellini / @rushatgabhane] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion in Slack here.
  • [@abekkala] A regression test has been added or updated so that the same bug will not reach production again. Link to the GH issue for creating the test here:

@abekkala
Copy link
Contributor

@neil-marcellini it looks like the PR was merged but the payment wasn't updated for title, is that correct?

@melvin-bot melvin-bot bot removed the Overdue label Nov 28, 2022
@neil-marcellini
Copy link
Contributor

Yes

@abekkala
Copy link
Contributor

Looks like that occurred 4 days ago on Nov 25 so payment is on hold until Dec 02

@abekkala abekkala changed the title [$250] Link with trailing spaces doesn't got trimmed reported by @kerupuksambel [HOLD for payment 2022-12-02] [$250] Link with trailing spaces doesn't got trimmed reported by @kerupuksambel Nov 29, 2022
@melvin-bot melvin-bot bot added the Overdue label Dec 2, 2022
@abekkala
Copy link
Contributor

abekkala commented Dec 2, 2022

Not overdue. payment is today

@melvin-bot melvin-bot bot removed the Overdue label Dec 2, 2022
@abekkala
Copy link
Contributor

abekkala commented Dec 2, 2022

Payments were sent to @0xmiroslav , @rushatgabhane and @kerupuksambel

We're all set here. Closing

Thank you! 🎉

@abekkala abekkala closed this as completed Dec 2, 2022
@0xmiros
Copy link
Contributor

0xmiros commented Dec 2, 2022

I think timeline bonus can be applied here
assigned on Nov 9 (#12511 (comment))
merged on Nov 14 (Expensify/expensify-common#471 (comment))

@abekkala
Copy link
Contributor

abekkala commented Dec 5, 2022

@0xmiroslav
I had noted that assignment was on Nov 9 and merged on Nov 14.
That is 5 days.

Merged PR within 3 business days receive a 50% bonus

Your merge was not within 3 business days.

@0xmiros
Copy link
Contributor

0xmiros commented Dec 5, 2022

@abekkala there was a weekend in between

@abekkala
Copy link
Contributor

abekkala commented Dec 5, 2022

ah yes yes, sorry!

@abekkala
Copy link
Contributor

abekkala commented Dec 5, 2022

@0xmiroslav @rushatgabhane are to receive a 50% bonus ($125 each)

@abekkala
Copy link
Contributor

abekkala commented Dec 5, 2022

Screen Shot 2022-12-05 at 11 10 04 AM

@abekkala
Copy link
Contributor

abekkala commented Dec 5, 2022

@0xmiroslav you've been paid the bonus! 🎉

@abekkala
Copy link
Contributor

abekkala commented Dec 5, 2022

@rushatgabhane you've been paid the bonus! 🎉

@0xmiros
Copy link
Contributor

0xmiros commented Dec 5, 2022

@0xmiroslav you've been paid the bonus! 🎉

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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
None yet
Development

No branches or pull requests

7 participants