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-06-01] [$2000] Request a call is redirecting to the link - reported by @adeel0202 #7909

Closed
mvtglobally opened this issue Feb 25, 2022 · 70 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor

Comments

@mvtglobally
Copy link

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 Concierge chat
  2. click on Request a call link in the message

Expected Result:

Request a call should redirect to a screen to request a call

Actual Result:

Request a call screen is redirecting to the link

Workaround:

unknown

Platform:

Where is this issue occurring?

  • Web
  • iOS
  • Android
  • Desktop App
  • Mobile Web

Version Number: 1.1.40-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: Any additional supporting documentation

22-02-09-21-45-54.mp4

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

View all open jobs on GitHub

@mvtglobally mvtglobally added AutoAssignerTriage Auto assign issues for triage to an available triage team member Daily KSv2 labels Feb 25, 2022
@MelvinBot
Copy link

Triggered auto assignment to @conorpendergrast (AutoAssignerTriage), see https://stackoverflow.com/c/expensify/questions/4749 for more details.

@MelvinBot MelvinBot added Overdue and removed AutoAssignerTriage Auto assign issues for triage to an available triage team member labels Feb 25, 2022
@MelvinBot
Copy link

@conorpendergrast Whoops! This issue is 2 days overdue. Let's get this updated quick!

@conorpendergrast
Copy link
Contributor

Reproduced; the expected effect is the same as clicking the phone icon ("Start a call") in the top-right

2022-03-01_15-55-40

@MelvinBot MelvinBot removed the Overdue label Mar 1, 2022
@conorpendergrast conorpendergrast removed their assignment Mar 1, 2022
@MelvinBot
Copy link

Triggered auto assignment to @alex-mechler (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

@alex-mechler
Copy link
Contributor

Ah interesting, nice catch. I think this can be handled externally.

@alex-mechler alex-mechler removed their assignment Mar 1, 2022
@alex-mechler alex-mechler added the External Added to denote the issue can be worked on by a contributor label Mar 1, 2022
@MelvinBot
Copy link

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

@mallenexpensify
Copy link
Contributor

@MelvinBot
Copy link

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

@MelvinBot MelvinBot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Mar 1, 2022
@MelvinBot
Copy link

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

@iwiznia
Copy link
Contributor

iwiznia commented Mar 3, 2022

I doubt it is this, but it's possible this was broken by #7662

@rushatgabhane
Copy link
Member

rushatgabhane commented Mar 3, 2022

@iwiznia hmm looks like we merged 7662 three days ago. And this issue was created days one week ago. So that shouldn't be possible

@iwiznia
Copy link
Contributor

iwiznia commented Mar 3, 2022

Yeah sorry, it is not that issue.
The problem is that it is a regular link in the text of message. The app is opening it as is, but we need to open it differently calling openOldDotLink so that it ensures the link opens logged in in oldDot.

@iwiznia
Copy link
Contributor

iwiznia commented May 18, 2022

Done! Thanks a lot!

@rushatgabhane
Copy link
Member

rushatgabhane commented May 18, 2022

can you please add details on the regression to the RCA sheet? Then, follow the steps in the C+ doc

@mallenexpensify all done, thanks for raising this!


To be honest I don't think we gonna have some case we are going to use expensify.com on chat out of attachments.

@mateusbra idk maybe we'll have expense reports embedded in chat. But the proposal seems fine for now.
I've added a few comments in the linked PR #8980

@mallenexpensify
Copy link
Contributor

@rushatgabhane , @rushatgabhane or @iwiznia , is the 'clock ticking' on the 7 days without a regression to issue payment on this?
Also, @rushatgabhane is your compensation half because of the regression?

@rushatgabhane
Copy link
Member

@mallenexpensify we aren't there yet.
Here's the PR incase you wanna track #8980

And yes, comp. is 1k for me

@mallenexpensify
Copy link
Contributor

Thanks @rushatgabhane this is labeled weekly so I should get pinged and will then check the PR you linked for updates

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels May 25, 2022
@melvin-bot melvin-bot bot changed the title [$2000] Request a call is redirecting to the link - reported by @adeel0202 [HOLD for payment 2022-06-01] [$2000] Request a call is redirecting to the link - reported by @adeel0202 May 25, 2022
@melvin-bot
Copy link

melvin-bot bot commented May 25, 2022

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.1.66-1 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 2022-06-01. 🎊

@mallenexpensify
Copy link
Contributor

Can someone help me understand this a lil better?
May 9th - PR deployed to Staging - link
May 11th - @mateusbra posts sorry for regression.
May 13th - PR deployed to production - link

The first step in the regression process is

  1. Make sure that the regression happens in production. If it does not happen in production, then the RCA process does not need to be followed.

Is this technically a regression if it was never deployed to production? And/or.. is there something I'm missing?
cc @rushatgabhane

@thienlnam
Copy link
Contributor

Yes, the regression was found on staging and I released a CP PR that reverted it and therefore it never got to production

@mallenexpensify
Copy link
Contributor

Interesting, for our Regression Cause Analysis (RCA) process (internal/C+) step 1 is

  1. Make sure that the regression happens in production. If it does not happen in production, then the RCA process does not need to be followed.

Since this didn't hit production, according to the process/doc, @rushatgabhane shouldn't have his pay reduced. If we disagree with this setup we can discuss updating the process

@rushatgabhane
Copy link
Member

rushatgabhane commented May 31, 2022

@thienlnam please correct me if I'm wrong, the offending PR was in production for 3 days.

Here’s what makes me believe that - the revert PR and the offending PR were deployed to production in different versions.

  • Offending PR deployed to production in v1.1.57-17 (gh)
  • Revert PR deployed to production in v1.1.60-3 (gh)

More details in this short thread - https://expensify.slack.com/archives/C01GTK53T8Q/p1652890950040879?thread_ts=1652889554.746239&cid=C01GTK53T8Q

@mallenexpensify the regression was in production for 3 days (again, if I'm not wrong)

@mallenexpensify
Copy link
Contributor

@rushatgabhane, for "Offending PR deployed to production in v1.1.57-17" I'm seeing May 13 as the date and @mateusbra commented on May 11th "sorry for regression", so I'm still confused.
image

@rushatgabhane
Copy link
Member

rushatgabhane commented May 31, 2022

commented on May 11th "sorry for regression", so I'm still confused.

@mallenexpensify ahhh I hope the timeline will add some clarity -

  • 9 May, offending PR is live on staging (gh)
  • 10 May, regression detected by Applause in staging only (gh)
  • 11 May, "sorry for regression" comment
  • 12 May, revert PR merged into main (development)
  • 13 May, offending PR deployed to production
  • 13 May, revert PR deployed to staging
  • 16 May, revert PR deployed to production

Are you confused by the fact that we knew there was a regression on 10 May, but still deployed it to production on 13 May?

  • Short answer is process failure. The revert PR wasn't CPed for stability reasons because of the upcoming earnings call (gh)

Please lemme know if this helped or just added to the confusion 😅

@thienlnam
Copy link
Contributor

Oh yes that's correct - I didn't look at the dates and forgot about the hold we had on CPs

@mallenexpensify
Copy link
Contributor

So... we had knowledge of the regression and an opportunity to NOT deploy it to production but decided against it? (cuz of the earnings call). If so, I don't think you should be penalized since it was Expensify's decision to deploy the PR to production.

@thienlnam , what do you think?

@thienlnam
Copy link
Contributor

So... we had knowledge of the regression and an opportunity to NOT deploy it to production but decided against it?

This was my bad - I should have added the deploy blocker label to the issue. I created a PR the same day and assumed it would just be merged / CPed in with the rest of the staging changes but it didn't happen immediately which allowed it to get deployed to production.

If we're only docking regressions that make it to production then we caught this one and it should have been deployed before production and it was my bad for not adding the deploy blocker label on the issue and the C+ should not be penalized.

@melvin-bot melvin-bot bot added Daily KSv2 Overdue and removed Weekly KSv2 labels Jun 1, 2022
@melvin-bot
Copy link

melvin-bot bot commented Jun 6, 2022

@iwiznia, @mallenexpensify, @rushatgabhane, @mateusbra Huh... This is 4 days overdue. Who can take care of this?

@melvin-bot
Copy link

melvin-bot bot commented Jun 6, 2022

@iwiznia, @mallenexpensify, @rushatgabhane, @mateusbra Eep! 4 days overdue now. Issues have feelings too...

@iwiznia
Copy link
Contributor

iwiznia commented Jun 6, 2022

So is there anything left to be done here?

@melvin-bot melvin-bot bot removed the Overdue label Jun 6, 2022
@mallenexpensify
Copy link
Contributor

I think we can close.
@rushatgabhane shouldn't have his compensation cut in half for this one.

@mateusbra
Copy link
Contributor

I think only the payment is pendent here

@mallenexpensify
Copy link
Contributor

Thanks @mateusbra I got so caught up in the potential regression I wasn't tracking payment :|
Based on the production deploy link and @thienlnam 's comment, I issued payment. $2000 to @mateusbra for the fix, $2000 to @rushatgabhane for C+ and $250 to @adeel0202 for reporting. Thanks all!!

#8980 (comment)
#8980 (comment)

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 Engineering External Added to denote the issue can be worked on by a contributor
Projects
None yet
Development

No branches or pull requests

10 participants