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

[$1000] [HOLD for payment 2023-04-07] Show all Expensify logins in "Contact methods" page, add "Contact details" page, allow "Resend validation" logic #15203

Closed
Beamanator opened this issue Feb 16, 2023 · 42 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Design Engineering Internal Requires API changes or must be handled by Expensify staff

Comments

@Beamanator
Copy link
Contributor

Beamanator commented Feb 16, 2023

Part of the bigger "Account Settings" project - https://github.com/Expensify/Expensify/issues/182122

This is part 2 of our plan, where we're breaking the process into 4 steps (see tracking issue ^)

Goals for this issue:

  • Show entire list of user's contact methods (secondary logins) for Expensify in the "Contact methods" page.
  • When user clicks on any contact method, open a new page for contact method details
    • For now, show "Send validation" button on that page when contact method isn't validated
    • Show "Remove" button as well, if the contact method isn't the user's "Default" / "Primary" contact method
    • Hook up API commands for both of the above
  • If user doesn't have a phone number login, render LoginField only for phone number type (and vice versa) so user has the ability to add at least 1 login of both types.
Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~012fce8a7330e675e7
  • Upwork Job ID: 1626161188767211520
  • Last Price Increase: 2023-04-10
@Beamanator Beamanator added Engineering Weekly KSv2 Internal Requires API changes or must be handled by Expensify staff labels Feb 16, 2023
@Beamanator Beamanator self-assigned this Feb 16, 2023
@melvin-bot melvin-bot bot locked and limited conversation to collaborators Feb 16, 2023
@MelvinBot
Copy link

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

@MelvinBot
Copy link

Triggered auto assignment to Contributor Plus for review of internal employee PR - @rushatgabhane (Internal)

@MelvinBot
Copy link

Triggered auto assignment to @shawnborton (Design), see these Stack Overflow questions for more details.

@Beamanator
Copy link
Contributor Author

@shawnborton tagging you because I'd love if you could check this design once it's closer to being merged - also are you able to provide a SVG asset of the mail-icon icon from https://www.expensify.com/styleguide/components.php#icons?

I need this icon for the 'Resend Verification" button, and I don't believe we have it in App yet - and in Olddot we use some kind of glyph thing for that icon

@shawnborton
Copy link
Contributor

Just wanted to check - is it the icon here from your Slack post that you need?

image

@Beamanator
Copy link
Contributor Author

Beamanator commented Feb 16, 2023

@shawnborton so actually that icon does exist in App and I just used it as a placeholder, sorry for the confusion - the icon we decided to go with in the Design doc looks like this (the one to the left of "Resend Verification"):

Screenshot 2023-02-16 at 4 52 33 PM

@shawnborton
Copy link
Contributor

Ah got it - I think we can just reuse the existing mail icon for now to keep things simple. Thoughts?

@Beamanator
Copy link
Contributor Author

I'm super down for that 👍 I think both icons mean the same thing and they're similar enough we're probably good to go reusing the existing one 👍

@shawnborton
Copy link
Contributor

Love it!

@Beamanator
Copy link
Contributor Author

Making good progress - Web-E PR is ready for review, in App PR I'm just working on the green & red brick roads getting all the way to the new pages

@Beamanator
Copy link
Contributor Author

PR ready for review!! Note @rushatgabhane - @mollfpr was assigned to review the PR b/c my PR also resolves this issue - maybe @mollfpr should also be assigned to this one?

@Beamanator Beamanator assigned mollfpr and unassigned rushatgabhane Mar 2, 2023
@Beamanator
Copy link
Contributor Author

Switching C+ to @mollfpr since you got auto-assigned on the PR 🙏

@rushatgabhane
Copy link
Member

sounds good. weird that my comment didn't go through :/

@Beamanator
Copy link
Contributor Author

PR was merged, there's a few cleanup tasks:

@twisterdotcom
Copy link
Contributor

twisterdotcom commented Mar 8, 2023

Do we want long email addresses to look like this in the page title?

This is more of a question for @Expensify/design - but I also think this is unavoidable in our design where email is the header.

We don't want to show "Magic code" if the user is not on passwordless, right?

Passwordless is so close to release, I don't think this matters.

@shawnborton
Copy link
Contributor

Hmm yeah I could see us making the title something generic like "Edit contact method" and then moving the email address down into the content area?

Otherwise it's probably fine for v1.

@melvin-bot melvin-bot bot removed the Weekly KSv2 label Mar 9, 2023
@melvin-bot melvin-bot bot added the Overdue label Apr 10, 2023
@MelvinBot
Copy link

@shawnborton, @Beamanator, @mollfpr Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

@Beamanator
Copy link
Contributor Author

Woohoo this has been no prod for 1 week w/out a revert! Oh shucks I need a BugZero person to help with payment 😬

@melvin-bot melvin-bot bot removed the Overdue label Apr 10, 2023
@Beamanator Beamanator added Internal Requires API changes or must be handled by Expensify staff and removed Internal Requires API changes or must be handled by Expensify staff labels Apr 10, 2023
@melvin-bot melvin-bot bot changed the title [HOLD for payment 2023-04-07] Show all Expensify logins in "Contact methods" page, add "Contact details" page, allow "Resend validation" logic [$1000] [HOLD for payment 2023-04-07] Show all Expensify logins in "Contact methods" page, add "Contact details" page, allow "Resend validation" logic Apr 10, 2023
@MelvinBot
Copy link

Current assignee @mollfpr is eligible for the Internal assigner, not assigning anyone new.

@Beamanator Beamanator added the Bug Something is broken. Auto assigns a BugZero manager. label Apr 10, 2023
@MelvinBot
Copy link

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

@MelvinBot

This comment was marked as off-topic.

@Beamanator
Copy link
Contributor Author

@MitchExpensify you are the lucky BZ to be assigned this delightful issue! 😅

Basically the work has already been finished and here's what happened:

  1. First PR (reviewed by @mollfpr) was merged here: [$1000] [HOLD for payment 2023-04-07] Show all Expensify logins in "Contact methods" page, add "Contact details" page, allow "Resend validation" logic #15203 (comment)
  2. That PR ^ got reverted because this bug was reported: [HOLD for payment 2023-04-07] [HOLD for payment 2023-03-17] Web - Profile - If user deletes the number without validating, page becomes unresponsive #15725
    • @aldo-expensify did some delicious investigation here and basically proved the bug was not in my original PR, but in another one so in the below PR we reverted a different change while re-implementing my original changes
  3. A second PR (reviewed by @parasharrajat) with all the original changes was merged here: [$1000] [HOLD for payment 2023-04-07] Show all Expensify logins in "Contact methods" page, add "Contact details" page, allow "Resend validation" logic #15203 (comment)

In my opinion both @mollfpr and @parasharrajat should get $1000 for their reviews, what do y'all think?

@parasharrajat
Copy link
Member

I would like to request an increase in the payment for my review. I had to review and test both PRs which were merged together. And I still found many issues with the original PR. Out of which many were fixed and some were moved to follow ups.

@MitchExpensify
Copy link
Contributor

What do you think is fair given that @parasharrajat ?

@parasharrajat
Copy link
Member

I think 2K-3K whatever you think is aligned with my argument.

@MitchExpensify
Copy link
Contributor

Cool I think given you reviewed two PRs that 2k seems fair. Seeing as @mollfpr reviewed one PR, 1K seems fair.

@melvin-bot melvin-bot bot added the Overdue label Apr 14, 2023
@MelvinBot
Copy link

@shawnborton, @Beamanator, @mollfpr, @MitchExpensify Huh... This is 4 days overdue. Who can take care of this?

@Beamanator
Copy link
Contributor Author

Bump @MitchExpensify , mind paying this out soon? 🙏 👍

@melvin-bot melvin-bot bot removed the Overdue label Apr 17, 2023
@MitchExpensify
Copy link
Contributor

Sorry about that! Not sure how it slipped by me and thanks for the bump @Beamanator

I had to create a new Upwork job which I've invited you both for payment to @parasharrajat & @mollfpr

Will issue payment based on this once you both accept the offer on the new job :)

@parasharrajat
Copy link
Member

@MitchExpensify can you please update the offer for me? it is supposed to be 2K.

@MitchExpensify
Copy link
Contributor

I'm aware @parasharrajat, just planned on including it as a bonus

@MitchExpensify
Copy link
Contributor

Paid @mollfpr and contract ended 👍

@MitchExpensify
Copy link
Contributor

Paid @parasharrajat and contract ended

We good to close @Beamanator ?

@melvin-bot melvin-bot bot added the Overdue label Apr 20, 2023
@MitchExpensify
Copy link
Contributor

Closing, we can reopen if @Beamanator disagrees :)

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

No branches or pull requests

8 participants