App
App copied to clipboard
Expensify
[$500] Workspace - Unable to select members after adding them to the workspace
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: 1.4.28-0 Reproducible in staging?: Y Reproducible in production?: Y 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: Applause - Internal Team Slack conversation:
Action Performed:
1,Navigate to Settings > Profile > Contact Method > New Contact Method > Add Email 2, change the new email as the default contact 3, Copy the main email and remove it 4, Go to Settings > Workspace > Members > click on Invite> then paste the main email or search for the email used to create the account 5, Add the email, click on the checkbox, and observe that it remains unselected
Expected Result:
After changing the default email and removing the main contact email (used to create the Expensify account), adding the main contact address to the workspace as a member should either be selected or not allowed
Actual Result:
After changing the default email and removing the main contact email, attempting to select the main contact address as a member in the workspace by clicking on the checkbox is not work
Workaround:
Unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
- [ ] Android: Native
- [x] Android: mWeb Chrome
- [ ] iOS: Native
- [ ] iOS: mWeb Safari
- [x] MacOS: Chrome / Safari
- [ ] MacOS: Desktop
Screenshots/Videos
Add any screenshot/video evidence
https://github.com/Expensify/App/assets/78819774/75ded569-c226-4f1d-b8e1-64c6c54eea5d
Upwork Automation - Do Not Edit
- Upwork Job URL: https://www.upwork.com/jobs/~01285e5476dd516a43
- Upwork Job ID: 1748410004851982336
- Last Price Increase: 2024-01-26
Job added to Upwork: https://www.upwork.com/jobs/~01285e5476dd516a43
![melvin-bot[bot] avatar](https://avatars.githubusercontent.com/in/179547?v=4 "Published by a pub.dev verified publisher"){kind=small}
Triggered auto assignment to @greg-schroeder (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.
Triggered auto assignment to Contributor-plus team member for initial proposal review - @0xmiroslav (External)
Proposal
Please re-state the problem that we are trying to solve in this issue.
After changing the default email and removing the main contact email, attempting to select the main contact address as a member in the workspace by clicking on the checkbox is not work
What is the root cause of that problem?
In here, we still use the policy.owner, we should use policy.ownerAccountID instead.
When the user changes the default contact method, the policy.owner is currently not changed to the new default contact method. So when selecting member in WorkspaceMembersPage, it will be disabled because the login of the email, that the account is created from, is still the policy.owner.
What changes do you think we should make in order to solve the problem?
Comparing logins is not a good way to check if 2 users are the same, we should compare by accountID instead, because 1 user can have multiple contact methods, but only have 1 single accountID.
We should compare accountID and policy.ownerAccountID here instead.
accountID === props.policy.ownerAccountID ||
We can also consider replacing the usage of policy.owner throughout that page (or elsewhere in the app) by policy.ownerAccountID. Some extra places and what we should do with them have been identified here. By the way, when creating the workspace, we need to set the ownerAccountID optimistically (currently we don't).
We've already moved to use accountID similarly throughout the app so we should do it here as well.
What alternative solutions did you explore? (Optional)
We can also fix by updating the policy.owner optimistically to the primary user's email when setting another contact method as default, and make sure it's also updated similarly in the back-end.
Triggered auto assignment to Contributor-plus team member for initial proposal review - @sobitneupane (External)
@sobitneupane reassigning, please take over as C+. If you don't have bandwidth, unassign yourself. Thanks
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸
@sobitneupane said he's working partially this week, let's try to get this one reviewed ASAP once you're back though!
I can take over this issue as C+ contributor if @sobitneupane didn't start on it
Thanks for the proposal @tienifr.
Are we using policy.owner else where? If so, we should update policy.owner optimistically. Additionally, after refreshing or signing in again, does the policy.owner received from the API reflect the updated information, or does it still show the old data?
Are we using policy.owner else where?
@sobitneupane yes, I see there're 2 other places we're using it:
-
here I don't see this method
getPolicyExpenseChatReportIDByOwnerused anywhere, we can safely remove it -
here This is using the
ownerto get theownerAccountID, so we can just use theownerAccountIDdirectly
So I think in both cases we can change to use ownerAccountID, not owner any more. By the way, when creating the workspace, we need to set the ownerAccountID optimistically (currently we don't)
Additionally, after refreshing or signing in again, does the policy.owner received from the API reflect the updated information, or does it still show the old data?
@sobitneupane It will reflect the updated information
Triggered auto assignment to @youssef-lr, see https://stackoverflow.com/c/expensify/questions/7972 for more details.
@youssef-lr @sobitneupane @greg-schroeder this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!
@youssef-lr can you please confirm the C assignment as soon as you're able? Thanks!
https://github.com/Expensify/App/issues/34828#issuecomment-1921328469
@youssef-lr @sobitneupane @greg-schroeder this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ and will need to go internal. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks!
While fixing in the frontend does work, I think it will be fixing a symptom as this should be fixed in the backend. We should be sending an updated owner once a default contact method has been changed. This will ensure wherever other parts of the code using policy.owner will also work.
By the way, when creating the workspace, we need to set the ownerAccountID optimistically (currently we don't)
I'm not sure this is possible, the accountID is an ID generated from the backend, if we generate it in the frontend it will just be replaced once we get a response from the backend. It's not similar to reportIDs or reportActionIDs that are random IDs. The accountID is sequential.
@youssef-lr Thanks for your feedback!
I'm not sure this is possible, the accountID is an ID generated from the backend, if we generate it in the frontend it will just be replaced once we get a response from the backend. It's not similar to reportIDs or reportActionIDs that are random IDs. The accountID is sequential.
When we "create a workspace", the ownerAccountID will be the accountID of the current user that is creating the workspace (this accountID is an existing one and is not optimistic), we'll not generate any new ID here.
While fixing in the frontend does work, I think it will be fixing a symptom as this should be fixed in the backend. We should be sending an updated owner once a default contact method has been changed. This will ensure wherever other parts of the code using policy.owner will also work.
Fixing only in back-end will not work in offline mode, you can refer to my alternative solution, we can update the policy.owner optimistically to the primary user's email when setting another contact method as default, once we do that we can send back updated value from the back-end in online mode, but that would be optional since policy.owner was already set correctly in optimistic data.
I think it's fine if we go with updating the policy.owner like this, but I prefer using the ownerAccountID since we're generally moving to use accountID for these use cases elsewhere since the user can have many contact methods but can only have 1 accountID.
This will ensure wherever other parts of the code using policy.owner will also work.
Regarding your concern here, I've already listed out all places we're still using policy.owner here, there're only a few minor places and all of them can be refactored a bit to use ownerAccountID without issue.
Sounds good @tienifr, let's go with using the ownerAccountID as well as updating the policy owner optimistically.
📣 @tienifr 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!
Offer link Upwork job Please accept the offer 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 📖
@youssef-lr @sobitneupane @greg-schroeder @tienifr this issue is now 4 weeks old and preventing us from maintaining WAQ, can you:
- Decide whether any proposals currently meet our guidelines and can be approved as-is today
- If no proposals meet that standard, please take this issue internal and treat it as one of your highest priorities
- If you have any questions, don't hesitate to start a discussion in #expensify-open-source
Thanks!