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] Profile - Address section blinks when add new address #46348

Closed
1 of 6 tasks
lanitochka17 opened this issue Jul 27, 2024 · 23 comments
Closed
1 of 6 tasks

[$250] Profile - Address section blinks when add new address #46348

lanitochka17 opened this issue Jul 27, 2024 · 23 comments
Assignees
Labels
AutoAssignerNewDotQuality Used to assign quality issues to engineers Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Internal Requires API changes or must be handled by Expensify staff Weekly KSv2

Comments

@lanitochka17
Copy link

lanitochka17 commented Jul 27, 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: 9.0.13-1
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught during regression testing, add the test name, ID and link from TestRail: N/A
Email or phone of affected tester (no customers): [email protected]
Issue reported by: Applause - Internal Team

Action Performed:

  1. Go to Profile> Address> Complete the necessary details> Save
  2. Take a note at Address section behavior on Profile page

Expected Result:

Address section Should not blink when add new address

Actual Result:

Address section blinks when add new address

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

Add any screenshot/video evidence

Bug6554026_1722021381303.Recording__3612.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01065cf64bab14773d
  • Upwork Job ID: 1818065643720366498
  • Last Price Increase: 2024-07-29
Issue OwnerCurrent Issue Owner: @hoangzinh
@lanitochka17 lanitochka17 added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Jul 27, 2024
Copy link

melvin-bot bot commented Jul 27, 2024

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

@lanitochka17
Copy link
Author

We think that this bug might be related to #vip-vsp

@lanitochka17
Copy link
Author

@jliexpensify FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

@melvin-bot melvin-bot bot added the Overdue label Jul 29, 2024
@perunt
Copy link
Contributor

perunt commented Jul 29, 2024

Hi, I am Taras from Margelo, an expert agency and I can work on this issue
I see that blinking as well, on mobile and web

@melvin-bot melvin-bot bot removed the Overdue label Jul 29, 2024
@jliexpensify jliexpensify added External Added to denote the issue can be worked on by a contributor Overdue labels Jul 29, 2024
Copy link

melvin-bot bot commented Jul 29, 2024

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

@melvin-bot melvin-bot bot changed the title Profile - Address section blinks when add new address [$250] Profile - Address section blinks when add new address Jul 29, 2024
@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Jul 29, 2024
Copy link

melvin-bot bot commented Jul 29, 2024

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

@melvin-bot melvin-bot bot removed the Overdue label Jul 29, 2024
@jliexpensify jliexpensify removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Jul 29, 2024
@jliexpensify
Copy link
Contributor

Sorry I missed this! Was assigned over my weekend.

All yours @perunt!

@Amoralchik
Copy link
Contributor

Amoralchik commented Jul 30, 2024

I believe this is a server-side error. After we add or update the address, we immediately write the data to Onyx. However, the server response does not include the address field, as initially we perform a SET operation that does not include the address field.

    "onyxData": [
        {
            "key": "private_personalDetails",
            "onyxMethod": "set",
            "value": {
                "addressCity": "Independence",
                "addressCountry": "US",
                "addressState": "KS",
                "addressStreet": "Quail Run",
                "addressZip": "67301",
                "addresses": [
                    {
                        "city": "Lakeside",
                        "country": "US",
                        "current": false,
                        "state": "CA",
                        "street": "Los Santos",
                        "type": "home",
                        "zip": "92040"
                    },
                ],
                "avatarThumbnail": "",
                "dob": "",
                "legalFirstName": "",
                "legalLastName": ""
            }
        },
        {
            "key": "private_personalDetails",
            "onyxMethod": "merge",
            "value": {
                "address": {
                    "city": "Queens",
                    "country": "US",
                    "current": true,
                    "state": "NY",
                    "street": "Queensboro Plaza Station",
                    "type": "home",
                    "zip": "11101"
                }
            }
        }

As a workaround, we can add a check to prevent changes to the address when this field is undefined. Unfortunately, with this approach, the user will never know that the address was removed when changing other parameters in private details (unless they refresh the profile page).
I also noticed, that when you change other private details, are remove address, it also happens on every platform

We are currently storing private personal data without the currentUserAccountID. Consequently, when login into another account, can potentially retrieve details from the previous user.

@perunt
Copy link
Contributor

perunt commented Jul 30, 2024

I raised this question in an open-source channel this morning with all the findings, but I haven't received an answer yet.

However, the server response does not include the address field

It includes an address field, but it's not a relevant address. I receive string instead of object. Also, that string is predefined. It was the first test address I entered for this account, and it is newer changing
Screenshot 2024-07-30 at 13 32 04
I'm not sure if it makes sense to keep the address field if we have addresses.
As a workaround, I can use addresses. But it would be nice to sync it with the backend
Are you part of the backed team?

@Amoralchik
Copy link
Contributor

@perunt
Are you part of the backed team? - No
As a workaround, I can use addresses. - as I mentioned before, if the user changes private details, the address is removed (probably need to retest on a newer version if you say is a string now), and if you use the last element in address array, he never know about it
I raised this question in an open-source channel this morning with all the findings, but I haven't received an answer yet. - sorry didn't notice it, I hope the answers will be of assistance.

@hoangzinh
Copy link
Contributor

I think it's a BE issue. @jliexpensify can you add an internal label so someone from the internal team can pick this issue and check the response body of API UpdateHomeAddress? Thank you.

@jliexpensify jliexpensify added Internal Requires API changes or must be handled by Expensify staff and removed External Added to denote the issue can be worked on by a contributor labels Aug 1, 2024
Copy link

melvin-bot bot commented Aug 6, 2024

@hoangzinh, @jliexpensify, @perunt Huh... This is 4 days overdue. Who can take care of this?

@melvin-bot melvin-bot bot added the Overdue label Aug 6, 2024
@jliexpensify
Copy link
Contributor

Not overdue

Copy link

melvin-bot bot commented Aug 8, 2024

@hoangzinh, @jliexpensify, @perunt Still overdue 6 days?! Let's take care of this!

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

melvin-bot bot commented Aug 8, 2024

Triggered auto assignment to @marcaaron (AutoAssignerNewDotQuality)

@melvin-bot melvin-bot bot added Weekly KSv2 and removed Overdue labels Aug 8, 2024
@mountiny
Copy link
Contributor

mountiny commented Aug 8, 2024

Based on @perunt this requires backend fix and I think this blinking can be considered as NewDotQuality bug, although I am not sure which category exactly would this fit in, probably LOW performance.

However, looking at the video of the bug, I think its super minor and it we might be good to close. I will defer to @marcaaron though

@marcaaron
Copy link
Contributor

marcaaron commented Aug 9, 2024

I'm not sure I fully understand the comments that have been left here yet so far, but address should not be a string.

I would suspect the changes in this PR might have something to do with it cc @arosiclair

@marcaaron
Copy link
Contributor

There are not any explicit places where we use an Onyx set on the private_personalDetails so, unfortunately this is going to be hard to debug without being able to get a stack trace. I can try to reproduce it, but also not sure about:

I think its super minor and it we might be good to close

The backend sending data in an unexpected format does not sound minor. But might be if we don't actually use the "address" field for anything. I can't really remember why we have both. But know it's come up before cc @Beamanator.

@marcaaron
Copy link
Contributor

Actually I can't repro this so I think it's been solved. Let's close.

@marcaaron
Copy link
Contributor

The response body of that API request looks normal.

@arosiclair
Copy link
Contributor

Yeah this probably got fixed by #45751. We deprecated address in favor of addresses since that's what we store in the DB.

It looks like we do need to cleanup a couple address updates here and here though. They shouldn't break anything but I'll post a PR to clean them up.

@Beamanator
Copy link
Contributor

Ooh I'm glad this is working and that we're going to be storing onyx data as we have in the DB. I remember some annoying discussions about this when implementing private personal details in App, glad that's all getting updated :D

@mountiny
Copy link
Contributor

Ah nice, glad this is resolved! 🎉

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 Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Internal Requires API changes or must be handled by Expensify staff Weekly KSv2
Projects
Development

No branches or pull requests

9 participants