(Tweaks to the Create Expense Flow) - follow-up - change workspace subtitle#55863
Merged
grgia merged 8 commits intoFeb 6, 2025
Conversation
melvin-bot
Bot
requested review from
abdulrahuman5196
and removed request for
a team
|
@abdulrahuman5196 Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
Contributor
|
🚧 @trjExpensify has triggered a test build. You can view the workflow run here. |
Contributor
|
Running a build and couldn't see it in the screenie, but just checking @JKobrynski, when you tap on the participant does the submitsTo carry through to the subtitle of the |
Contributor
|
Assigning this to @mananjadhav as he's been on the PRs for this issue. |
trjExpensify
requested review from
mananjadhav
and removed request for
abdulrahuman5196
Contributor
|
Why does this one say 
|
This comment has been minimized.
This comment has been minimized.
Contributor
|
Okay, two things from me:
|
Collaborator
|
@JKobrynski Can you check the comment from @trjExpensify ? |
Contributor
|
Ah shit, also spotted another one for the list. We've updated it in the sub-header and in details: 
|
Contributor
Author
I added it as a fallback, just so it doesn't show Besides that, the PR was updated, all comments have been addressed, feel free to give it another go! |
grgia
requested changes
Jan 30, 2025
grgia
left a comment
grgia
left a comment
Contributor
There was a problem hiding this comment.
Changes LGTM, could you fix lint?
grgia
previously approved these changes
Jan 30, 2025
Collaborator
|
Just finished the code review, will test this now. |
Collaborator
|
@JKobrynski I can see still the workspaces that were archived. Am I missing something? 
|
Contributor
Author
|
@mananjadhav I tried to reproduce this but I couldn't. I also searched through the code and did a bit of debugging to try and find possible causes of this but I think it's not an efficient way to do this so here are a couple solutions that I'd like to suggest:
if ((isPolicyExpenseChat(report) && !!report?.isOwnPolicyExpenseChat) || isExpenseReport(report)) {
const submitToAccountID = getSubmitToAccountID(getPolicy(report?.policyID), report);
const submitsToAccountDetails = allPersonalDetails?.[submitToAccountID];
const subtitle = submitsToAccountDetails?.displayName ?? submitsToAccountDetails?.login;
+ console.log('POLICY ID', report?.policyID);
+ console.log('POLICY', getPolicy(report?.policyID));
+ console.log('SUBMIT TO ACCOUNT ID', submitToAccountID);
+ console.log('SUBMIT TO ACCOUNT DETAILS', submitsToAccountDetails);
+ console.log('SUBTITLE', subtitle);
if (!subtitle || !config.isCreateExpenseFlow) {
return translateLocal('workspace.common.workspace');
}
return translateLocal('iou.submitsTo', {name: subtitle ?? ''});
}and look at the console to locate the data of the report that has the missing subtitle, then paste a screenshot of it here so we can discuss where exactly it fails to retrieve it. Let me know what you think! |
Collaborator
|
@JKobrynski Here's an data of one the policies. Now I am not sure why the data is different for both the policies on two different platforms. **Web****iOS** |
Contributor
Author
|
@mananjadhav thank you for uploading these objects, I really appreciate it!! I'm taking a look at them now and I see the |
…ticipants-selector-workspace-row
Contributor
Author
|
Conflicts resolved. |
Collaborator
|
Yes all the |
grgia
approved these changes
Feb 5, 2025
Contributor
|
🚧 @grgia has triggered a test build. You can view the workflow run here. |
Contributor
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪 |
Contributor
|
What's the latest here, are we ready to merge? |
Collaborator
|
As I've said I can still reproduce this issue. @JKobrynski I am also not the admin of these workspaces. Can you probably post on the #expensify-open-source channel to ask someone from the QA team to add you? @grgia Any thoughts on this? I am not sure if this could be a follow up or it's a workspace specific change. |
Contributor
Author
|
@mananjadhav it might be some kind of Onyx issue, where the object just doesn't fully load as one of its properties is an array with like 100 records 🤔 I'm going to post on that channel so someone can add me. If you'd like, we can merge this and start working on a follow-up to fix this |
Collaborator
|
I think we should go ahead with the PR, but will let @grgia decide. |
Collaborator
Reviewer Checklist
Screenshots/VideosAndroid: NativeAndroid: mWeb Chromemweb-chrome-submits-to.moviOS: Nativeios-submits-to.moviOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop
|
Collaborator
|
We can definitely rule out this PR as an issue, because if I select the workspace and then go back to the list + search you'll end up seeing the @grgia @JKobrynski I think we should create a separate issue for the same. |
mananjadhav
approved these changes
Feb 6, 2025
grgia
approved these changes
Feb 6, 2025
Contributor
@mananjadhav could you help me out with the description of the issue |
Contributor
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
Contributor
|
🚀 Deployed to staging by https://github.com/grgia in version: 9.0.95-0 🚀
|
Collaborator
|
Sorry I missed it. @grgia let me know if this is fine: Problem:
Repro Steps:
Debug Findings:
|
8 tasks
Contributor
Contributor
|
🚀 Deployed to production by https://github.com/Julesssss in version: 9.0.95-6 🚀
|
dukenv0307
reviewed
May 8, 2025
| const ownerPersonalDetails = OptionsListUtils.getPersonalDetailsForAccountIDs(report?.ownerAccountID ? [report.ownerAccountID] : [], personalDetails); | ||
| const displayNamesWithTooltips = ReportUtils.getDisplayNamesWithTooltips(Object.values(ownerPersonalDetails), false); | ||
| const shouldShowSubscriptAvatar = ReportUtils.shouldReportShowSubscript(report); | ||
| const title = getReportName(report, undefined, undefined, undefined, invoiceReceiverPolicy); |
Contributor
There was a problem hiding this comment.
Coming from: #61070. We should add reportAttributes here so the title will be updated
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Explanation of Change
Check this comment for details.
Fixed Issues
$ #52981
PROPOSAL: N/A
Tests
Disclaimer:
I have a workspace on my account that is linked to an account that can't be found, for that I implemented a fallback that just displays "Workspace", same as before.
Offline tests
N/A
QA Steps
Same as Tests section above
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
MacOS: Chrome / Safari
Screen.Recording.2025-01-28.at.15.32.19.mov