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

[$250] Allow support agents to unsubscribe on a user’s behalf via NewDot Supportal #50842

Open
1 of 6 tasks
m-natarajan opened this issue Oct 15, 2024 · 33 comments
Open
1 of 6 tasks
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors Needs Reproduction Reproducible steps needed Reviewing Has a PR in review Weekly KSv2

Comments

@m-natarajan
Copy link

m-natarajan commented Oct 15, 2024

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!


Version Number:
Reproducible in staging?: needs reproduction
Reproducible in production?: needs reproduction
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
Expensify/Expensify Issue URL:
Issue reported by: @maddylewis
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1729018431544889

Action Performed:

  1. Support log into NewDot customer account
  2. Head to Account Settings > Preference > App preferences
  3. Attempt to disable the "receive relevant feature updates and expensify news" setting

Expected Result:
Actual Result: i cannot d

Expected Result:

Should be able to disable "receive relevant feature updates and expensify news" while support logged into a NewDot account

Actual Result:

Unable to disable "receive relevant feature updates and expensify news"

Workaround:

unknown

Platforms:

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

  • Android: Native
  • Android: mWeb Chrome
  • iOS: Native
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

2024-10-14_09-25-42.mp4
Add any screenshot/video evidence

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021847125281120053453
  • Upwork Job ID: 1847125281120053453
  • Last Price Increase: 2024-10-18
Issue OwnerCurrent Issue Owner: @allgandalf
@m-natarajan m-natarajan added Daily KSv2 Needs Reproduction Reproducible steps needed Bug Something is broken. Auto assigns a BugZero manager. labels Oct 15, 2024
Copy link

melvin-bot bot commented Oct 15, 2024

Triggered auto assignment to @alexpensify (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.

@MelvinBot
Copy link

This has been labelled "Needs Reproduction". Follow the steps here: https://stackoverflowteams.com/c/expensify/questions/16989

@maddylewis
Copy link
Contributor

@alexpensify - this is super easy to repro! just follow the steps outlined above to confirm you don't have the option to disable that setting while support logged in 👍

@alexpensify
Copy link
Contributor

Yep, I can replicate this experience.

@alexpensify alexpensify added the External Added to denote the issue can be worked on by a contributor label Oct 18, 2024
@melvin-bot melvin-bot bot changed the title Allow support agents to unsubscribe on a user’s behalf via NewDot Supportal [$250] Allow support agents to unsubscribe on a user’s behalf via NewDot Supportal Oct 18, 2024
Copy link

melvin-bot bot commented Oct 18, 2024

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

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

melvin-bot bot commented Oct 18, 2024

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

@alexpensify
Copy link
Contributor

@allgandalf—We need proposals here, but I've marked this as external to start the process for this issue.

Heads up, I will be offline until Tuesday, October 22, 2024, and will not actively watch over this GitHub during that period.

If this GitHub requires an urgent update, please ask for help in the #expensify-open-source Slack Room. If the inquiry can wait, I'll review it when I return online.

@shahinyan11
Copy link
Contributor

shahinyan11 commented Oct 18, 2024

@m-natarajan Hi there. Could you please explain what means Support log. Please provide more details so that it is understandable to external contributors. Thanks

@allgandalf
Copy link
Contributor

@alexpensify @maddylewis , Can you make a video of this bug with networks tab open? If you see a call to UpdateNewsletterSubscription then please open networks tab and go to payload and response to get better understanding:

You can follow steps in the video below:

Screen.Recording.2024-10-18.at.1.05.06.PM.mov

@narefyev91
Copy link
Contributor

Hi, I'm Nicolay from Callstack - expert contributor group - and I would like to work on this issue.

@melvin-bot melvin-bot bot added the Overdue label Oct 21, 2024
@allgandalf
Copy link
Contributor

cool! @narefyev91 can you start working, someone can assign you this soon :))

@melvin-bot melvin-bot bot removed the Overdue label Oct 21, 2024
@allgandalf
Copy link
Contributor

@narefyev91 did you find out the RCA here ?

@narefyev91
Copy link
Contributor

@narefyev91 did you find out the RCA here ?

on it - but closely to add my research

@narefyev91
Copy link
Contributor

narefyev91 commented Oct 22, 2024

My findings so far:
First of all i was not able to reproduce the issue on latest main:

Screen.Recording.2024-10-22.at.13.55.50.mov

But seems like i have some ideas why issue maybe happened in some rare case.
First of all - i see some differences in "Screenshots/Videos" section and what i can see on dev:
DEV:
Screenshot 2024-10-22 at 14 00 46
Videon in issue:
Screenshot 2024-10-22 at 14 00 56

In description in the issue it says that user should login as "Copilot" - but if that happened - user will see dropdown near the name (see images above).
I tried different ways (using a new Copilot account and existing - but without any lack to detect the bug:
A new account:

8mb.video-hF1-Da4kNFwJ.mp4

And existing one:

Screen.Recording.2024-10-22.at.14.05.35.mov

Also i tried with not verified account:

Screen.Recording.2024-10-22.at.13.58.24.mov

Which mean that now in current time FE and API is working fine.
But one more thing - i realised during investigating code:
Screenshot 2024-10-22 at 14 15 03
This switch is used to enable/disable "Receive relevant feature updates and Expensify news". We can see that we have a fallback "isOn={user?.isSubscribedToNewsletter ?? true}" .
isSubscribedToNewsletter is coming from API. If the issue happened before - i can say 99% that API returns isSubscribedToNewsletter = null for some reason.
Which means that user try to toggle off -> API request sent -> in response get isSubscribedToNewsletter = null -> default value true is set to Toggle - example:

Screen.Recording.2024-10-22.at.14.19.38.mov

I see that fallback value was added just because isOn could not be undefined (that was done during re-writing Switch component to TS).
Anywhere - it's not correct - we should only on Onyx/API data and not set true where it should not be.

My suggestion here - to remove that fallback and add optional isOn to TS type for Switch
/** Whether the switch is toggled to the on position */ isOn?: boolean;

let me know your thoughts @allgandalf

@allgandalf
Copy link
Contributor

@narefyev91 i guess you mistook supportal for co-pilot 😅 , the issue is for supportal account logged in into customer account and not co-pilot, do let me know if you are still confused with this

@narefyev91
Copy link
Contributor

@narefyev91 i guess you mistook supportal for co-pilot 😅 , the issue is for supportal account logged in into customer account and not co-pilot, do let me know if you are still confused with this

Well and how does support account is set to login to customer account?

@allgandalf
Copy link
Contributor

allgandalf commented Oct 22, 2024

Well and how does support account is set to login to customer account?

@maddylewis can you please help in here, I think it's from a different platform, hence i asked for network tag video in this comment, i will bump her on slack and C.c you @n

@narefyev91
Copy link
Contributor

In any case we have 2 possible issues here:

  1. API return isSubscribedToNewsletter = null
  2. not correct fallback true is set

@allgandalf
Copy link
Contributor

I feel 2. is the issue here, if the API return null then we would have seen it on the UI, I guess we have failureData there as well.

Lets explore option 2 for now

@narefyev91
Copy link
Contributor

I feel 2. is the issue here, if the API return null then we would have seen it on the UI, I guess we have failureData there as well.

Lets explore option 2 for now

Yeah agree - fallback should be removed 100%

@allgandalf
Copy link
Contributor

allgandalf commented Oct 22, 2024

I agree with your analysis and it makes sense to me ! I will also get an internal engineer involved to proceed with this bug.

🎀👀🎀

Copy link

melvin-bot bot commented Oct 22, 2024

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

@allgandalf
Copy link
Contributor

hey @chiragsalian , The bug is when someone Supportals into customer account, We are still waiting for the network tag API call from @maddylewis (IF you have access to supportal even you can help here) but overall, the RCA by @narefyev91 here makes sense to me as to why that remains on. How should we proceed here :)

@allgandalf
Copy link
Contributor

Okay cool, @narefyev91 @chiragsalian , when we supportal into an account and try to toggle the button, the API never gets called:

Here are recordings with network tab on by @maddylewis :

2024-10-22_09-34-48.mp4
2024-10-22_09-17-09.mp4

@narefyev91
Copy link
Contributor

Okay cool, @narefyev91 @chiragsalian , when we supportal into an account and try to toggle the button, the API never gets called:

Here are recordings with network tab on by @maddylewis :

2024-10-22_09-34-48.mp4
2024-10-22_09-17-09.mp4

API may not be called - if anything blocked API.write to be executed - like some permissions or something else

@narefyev91
Copy link
Contributor

Will dive deeper tomorrow - have some ideas to check. Will keep posted!

@chiragsalian
Copy link
Contributor

Haven't investigated, but this part,

when we supportal into an account and try to toggle the button, the API never gets called

Sounds like a front end issue no 😛
Was i tagged because its difficult to reproduce supportalling into an account? Just trying to understand how to help out.

In description in the issue it says that user should login as "Copilot" - but if that happened - user will see dropdown near the name

I don't see copilot mentioned in the issue. Also to be clear, copilot and supportal and very different flows.
For copilot primary account needs to give you permission and once you have it it's like logging into the account and you can do all the same actions as the primary.
For supportal no permission is provided, it's a feature only allowed by internal employees to see what the customer sees to help diagnose issues. Some actions are permitted but a lot are usually blocked. I wonder if we've placed some code in newDot like - if user has supportal token then don't allow any write action.

@allgandalf
Copy link
Contributor

Was i tagged because its difficult to reproduce supportalling into an account? Just trying to understand how to help out.

BINGO!!!!, I wanted to see whether the API call is made or not, cause @maddylewis was OoO and I for obvious reasons cannot reproduce this.

if we've placed some code in newDot like - if user has supportal token then don't allow any write action.

is it? Do you have any example code where the action is blocked on ND for supportal? I can reference that to find the RCA

@chiragsalian
Copy link
Contributor

BINGO!!!!, I wanted to see whether the API call is made or not, cause @maddylewis was OoO and I for obvious reasons cannot reproduce this.

Tested on staging. The API call is not made for me when i use supportal. I tested toggling "Receive relevant feature updates and Expensify news".

is it? Do you have any example code where the action is blocked on ND for supportal? I can reference that to find the RCA

It was just my first guess. I looked around a little bit and found it here. So if we're using a supportal authtoken and its not a supportal request then no API call is made. The list of supported supportal actions are listed here ✌️

@allgandalf
Copy link
Contributor

It was just my first guess. I looked around a little bit and found it here. So if we're using a supportal authtoken and its not a supportal request then no API call is made. The list of supported supportal actions are listed here ✌️

Damn!!!, okay so if we simply add the WRITE command for the toggle in isSupportRequest then we will make an actual API call and not return a promise that will resolve itself. if that is it them i guess @narefyev91 can have a PR ready for us tomorrow !, but we need your help testing once it is on staging as even the QA won't be able to help here :))

@narefyev91
Copy link
Contributor

Yeah thanks @chiragsalian ! Ended up yesterday on exactly the same file as you mentioned - did not even know that we have separate logic to prevent API calls to be executed for customer support. Will create a fix PR soon

@allgandalf
Copy link
Contributor

did not even know that we have separate logic to prevent API calls to be executed for customer support.

yeah, even I didn't know that. just curious: @chiragsalian is this validation FE only or do we have these checks on the BE as well?

@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Oct 23, 2024
@chiragsalian
Copy link
Contributor

@chiragsalian is this validation FE only or do we have these checks on the BE as well?

We have these checks on the BE as well. Its very easy to add BE support but yeah we need to test and confirm if any changes need to be back to the backend. When I'm testing the FE PR ill check and make changes if needed.

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. External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors Needs Reproduction Reproducible steps needed Reviewing Has a PR in review Weekly KSv2
Projects
Status: Support (HIGH)
Development

No branches or pull requests

8 participants