Feat : show Invite button on Members page for money request reports#88459
Feat : show Invite button on Members page for money request reports#88459M00rish wants to merge 20 commits into
Conversation
melvin-bot
Bot
requested review from
abzokhattab and
flaviadefaria
and removed request for
a team
|
@abzokhattab 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] |
M00rish
changed the title
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| const {translate, formatPhoneNumber} = useLocalize(); | ||
| const [countryCode = CONST.DEFAULT_COUNTRY_CODE] = useOnyx(ONYXKEYS.COUNTRY_CODE); | ||
| const [didScreenTransitionEnd, setDidScreenTransitionEnd] = useState(false); | ||
| const reportAttributes = useReportAttributes(); |
There was a problem hiding this comment.
❌ PERF-11 (docs)
useReportAttributes() subscribes to the entire ONYXKEYS.DERIVED.REPORT_ATTRIBUTES record (all reports' attributes). This component only needs reportAttributes?.[report.reportID]?.reportName — a single string. Every time any report's attributes change anywhere in the app, this component will re-render unnecessarily.
Consider a targeted useOnyx call with a selector that returns the primitive reportName string for just this report:
const [reportName] = useOnyx(ONYXKEYS.DERIVED.REPORT_ATTRIBUTES, {
selector: (data) => data?.reports?.[report.reportID]?.reportName,
});
// Then use: StringUtils.lineBreaksToSpaces(reportName ?? '')A selector returning a primitive (string) is cheap to compare and prevents re-renders from unrelated report attribute changes.
Reviewed at: fbfb102 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
is this ready for review or still waiting for backend work? @M00rish |
|
There's some backend work to be done |
|
PR is ready for review again @abzokhattab thanks! |
|
@codex review |
|
@MelvinBot review |
PR ReviewOverall: Clean, well-scoped PR with good test coverage. A few items to address: 1. Selector re-render risk (agrees with PERF-11 bot flag)
const [reportName = ''] = useOnyx(ONYXKEYS.DERIVED.REPORT_ATTRIBUTES, {
selector: (data) => data?.reports?.[reportID]?.reportName,
});
// Then: StringUtils.lineBreaksToSpaces(reportName)This eliminates the 2. Good catch on optimistic
|
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
|
@abzokhattab can you review it please? |
|
on my list for today |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari |
|
I noticed a potential issue: the Invite member button is visible to all participants of the expense report, including users who were just invited — not just the submitter or admins. Should we limit visibility to the submitter or admins only? @lakchote @flaviadefaria what's the intended permission model here? Untitled.mov |
Testing this is Classic, it seems like anyone who has access to the report can share the report with someone else: A few thoughts that came to mind while reading the issue and proposed solution:
Before we move forward, I’d love to hear thoughts from @trjExpensify and @Expensify/design on this. Here is a screenshot of the sharing page in Classic: 
|
I feel like we labeled it Personally though I agree with you, Curious for more thoughts from the design team and @Expensify/product though. |
|
Yeah, we might want to have a broader conversation about our current "Share" feature and if users actually use it or know what it means: That is likely a separate conversation though. Definitely curious about Flavia's points above and agree with Danny's comments too. |
That's my recollection too, because this "member" management is available on chat reports well: 
I think even if we improve "Share" and add another alt option there for manual sharing instead of just the URL option, I'd probably still keep |
|
Thanks for the context. Personally, I think we’re optimizing this for chat, even though we know many of our customers don’t like the chat-centric interface. I also wonder how many users actually invite someone to a workspace chat. We can move forward with the current approach, but I think this will end up being a feature that’s not intuitive and will consistently require users to reach out for help to figure out where it is. I’d rather we break the pattern here if it makes a commonly requested feature easier to discover than stick to a pattern that users don’t really understand. The real goal isn’t to invite someone into the report chat, it’s to share the report with them so they can view the expenses and export it. I also think the email copy will be confusing if we use “invited” instead of “shared.” 
But that’s just my two cents. If everyone feels consistency is the higher priority here, I’m okay with that, it is a relatively small feature after all. |
|
I agree with Flavia personally. I think we kinda just got this wrong the first time around. Just my 2 cents of course, but for basically all other things that I want people to be able to see and/or edit, the verbiage is share (Google Doc, Figma file, grocery list, note, etc.) But I also feel the same as:
|
|
I don't think anyone here is disputing we should still consider bringing "Share" out of details and into the More menu, adding an option in it for a manual share with a bespoke share message etc. In fact, didn't Jason say here that he was going to create an issue for that? 🤔 I can't find it, but I think it can be progressed. Equally though, this is just a PR to fix a bug with the existing Members screen in the product, so I still think that's out of scope here and we should proceed to fix this in the meantime. |
|
Sorry I did not create an issue. That said, I still feel the same. I like:
|
|
Just to make sure we’re aligned, are we considering this a bug? @trjExpensify, I thought your comment in the main issue was that it wasn’t:
My understanding is that we’re implementing functionality from Classic in NewDot, rather than fixing something that’s currently broken. With that in mind, I wonder if it would make more sense to close this issue, first align on our broader approach to Share in a separate issue, and then implement the requested changes as part of that work. That said, I’m happy to go with whatever approach the team thinks is best. I mainly want to make sure we’re solving the right problem before we start implementing a solution. |
|
Yeah, the first part of that quote is referring to not having a "CC" option on submission as we don't support that in NewDot (therefore not a bug). The second part of the quote is referencing the bug this PR fixes - which is the invite button was missing atop the Members page. I did point out a few comments below that one how the OP of that parent issue is confusing and not reflective of what we were actually fixing here in the first order:
|
|
Ok cool, I'm following - down with what you and Jason are suggesting 👍 |
|
Flavia - do you want to lead up the next steps with Share? |
|
Thanks for clarifying, @trjExpensify! The OP was definitely misleading. I’m on board with fixing the bug here and discussing sharing options as a separate initiative.
I'm not passionate about it but I'll add it to my list. |
Back to this comment from @abzokhattab, yes we should limit it to submitter and admins only to keep the same behavior as we have in the workspace chat. |
|
@M00rish lets the invitation for submitter and admins only |
|
@abzokhattab would you review pls ? |
There was a problem hiding this comment.
this file change is not related to the issue ... is it intended?
There was a problem hiding this comment.
yes it is, it solves messaging the invited member error
There was a problem hiding this comment.
sorry i am not following. What messaging error? It would be nice if you could share can video
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9eba2e9c1f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const isGroupChat = isGroupChatUtils(report); | ||
| const policy = usePolicy(report.policyID); | ||
| const isReportSubmitterOrAdmin = isCurrentUserSubmitter(report) || isPolicyAdmin(policy); | ||
| const shouldShowInviteButton = isReportSubmitterOrAdmin && (isGroupChat || (isMoneyRequestReport(report) && isOpenExpenseReport(report))); |
There was a problem hiding this comment.
When the report is a group chat, this new isReportSubmitterOrAdmin requirement hides the entire action container for group-chat admins who are not also the report owner/workspace admin. The table still enables selection from isCurrentUserGroupChatAdmin, so those admins can select members but no longer see Remove / Make admin actions, and non-owner group chat members also lose the invite button that existed before this change. Gate the group-chat case on the group-chat permissions, or apply the submitter/policy-admin check only to open expense reports.
Useful? React with 👍 / 👎.
| selector: reportAttributesSelector, | ||
| }); | ||
| const backPath = useDynamicBackPath(DYNAMIC_ROUTES.REPORT_PARTICIPANTS_INVITE.path); | ||
| const shouldShowInvitePage = isGroupChat(report) || (isMoneyRequestReport(report) && isOpenExpenseReport(report)); |
There was a problem hiding this comment.
For open expense reports, the participants page hides the invite button unless the current user is the submitter or a policy admin, but this route-level check allows any open expense report. A non-submitter member who opens the invite URL directly can still search users and submit inviteToGroupChat, causing optimistic participant changes/API calls despite the action being hidden in the normal flow. Mirror the same submitter/policy-admin permission here for the money-request case.
Useful? React with 👍 / 👎.
|
could you review again @abzokhattab ? the error checks are from main will merge main once resolved. |
|
Left a comment here #88459 (comment) @M00rish |
|
please check the video |
9 participants
Explanation of Change
Fixed Issues
$ #80957
PROPOSAL: comment
Tests
Offline tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)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
Android: Native
Android.webm
Android: mWeb Chrome
Android.webm
iOS: Native
ios.webm
iOS: mWeb Safari
iosWEB.webm
MacOS: Chrome / Safari
web.webm