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

[HOLD for payment 2023-08-14] [HOLD for payment 2023-08-08] [HOLD for payment 2023-08-03] [$1000] Polish admins-only policy room feature #21456

Closed
4 tasks
amyevans opened this issue Jun 23, 2023 · 56 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Daily KSv2 External Added to denote the issue can be worked on by a contributor NewFeature Something to build that is a new item.

Comments

@amyevans
Copy link
Contributor

amyevans commented Jun 23, 2023

Background

We recently added a configurable setting to all default and user-created policy rooms, available to admins only, that allows admins to define who can post in that room: All members or Admins only. This enables admins to "control the conversation" in rooms that have large membership, or for whatever other reason should be lower-noise.

While the bulk of the work is complete, there's some remaining polish to be done on the FE.

Tasks

  • The default form value should be all
  • Send the writeCapability value when calling the AddWorkspaceRoom endpoint
  • Ensure the value is included in the optimisticData (to support offline creation)
  • Note: BE PRs supporting the writeCapability value in the AddWorkspaceRoom endpoint aren't live yet, but don't need to block work here
  • Filter out admin-only rooms for members in the "Share somewhere" selector of the "Assign task" action (accessible via the FAB menu) - so that members cannot share a task to the room
  • Reduce the padding between the last message and the bottom of the page when the composer isn't present (e.g. for the "member" view in an admin-only room)
  • Update the room description to be Use ${roomName} to hear about important announcements related to ${workspaceName} for admin-only rooms
Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~012ddadecb1f0a54b9
  • Upwork Job ID: 1675048364496478208
  • Last Price Increase: 2023-07-01
@amyevans amyevans added Weekly KSv2 NewFeature Something to build that is a new item. labels Jun 23, 2023
@amyevans amyevans self-assigned this Jun 23, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jun 23, 2023

@melvin-bot
Copy link

melvin-bot bot commented Jun 23, 2023

Triggered auto assignment to Design team member for new feature review - @shawnborton (NewFeature)

@amyevans
Copy link
Contributor Author

Internal issue for reference: https://github.com/Expensify/Expensify/issues/282865

I shared in Slack to see if a Callstack team member has capacity for this.

@jliexpensify
Copy link
Contributor

LMK if you need any Upworks stuff sorted!

@rezkiy37
Copy link
Contributor

Hi, I'm Michael (Mykhailo) from Callstack and I would like to work in this issue.

@rezkiy37
Copy link
Contributor

rezkiy37 commented Jun 26, 2023

@amyevans Hi!
Is it possible to add my Expensify accounts to the white list of Create Policy Room and tasks?

Screenshot 2023-06-26 at 17 07 42

@amyevans
Copy link
Contributor Author

Done

@rezkiy37
Copy link
Contributor

rezkiy37 commented Jun 27, 2023

@amyevans I have a question about the last task.
Do you mean to replace entirely?

  • Before: Collaboration among ${workspaceName} admins starts here! 🎉 Use ${roomName} to chat about topics such as workspace configurations and more.

  • After: Use ${roomName} to hear about important announcements related to ${workspaceName}.

Screenshot 2023-06-27 at 17 25 00

@amyevans
Copy link
Contributor Author

Collaboration among ${workspaceName} admins starts here! 🎉 Use ${roomName} to chat about topics such as workspace configurations and more.

This is the copy for the #admins room, and the #admins room is something different - it is one of 2 rooms created by default when you create a new workspace aka policy (#announce being the other default room). The purpose of the #admins room is for collaboration between admins. Users who are members of the policy (but not admins of the policy) are not in the #admins room.

Then we have this feature we're working on here, which is any room that is restricted to only admins posting (but plenty of non-admins are in the room). Check out the screen recordings/code in #19094 for more context. So we'll want to insert a new condition here-ish:

} else if (isDomainRoom(report)) {

that checks if it's a room where only admins can post and then spits out Use ${roomName} to hear about important announcements related to ${workspaceName}.

Does that make sense?

@rezkiy37
Copy link
Contributor

rezkiy37 commented Jun 27, 2023

@amyevans Thank you for your answer.
Just to clarify:

  1. The #admins room is the policyAdmins chat type.
  2. isAdminRoom verifies it of getRoomWelcomeMessage.

    App/src/libs/ReportUtils.js

    Lines 574 to 576 in 5c6b751

    } else if (isAdminRoom(report)) {
    welcomeMessage.phrase1 = Localize.translateLocal('reportActionsView.beginningOfChatHistoryAdminRoomPartOne', {workspaceName});
    welcomeMessage.phrase2 = Localize.translateLocal('reportActionsView.beginningOfChatHistoryAdminRoomPartTwo');
  3. phrase1 should be Use . Because ${roomName} should be presseble to navigate.
    {isChatRoom && (
    <>
    <Text>{roomWelcomeMessage.phrase1}</Text>
    <Text
    style={[styles.textStrong]}
    onPress={() => Navigation.navigate(ROUTES.getReportDetailsRoute(props.report.reportID))}
    >
    {ReportUtils.getReportName(props.report)}
    </Text>
    <Text>{roomWelcomeMessage.phrase2}</Text>
    </>
    )}
  4. phrase2 should be to hear about important announcements related to ${workspaceName}.

Correct me if I am wrong 🙂

Screenshot 2023-06-27 at 18 39 28

@amyevans
Copy link
Contributor Author

  1. The #admins room is the policyAdmins chat type.

Yep! (But again, that isn't the room we're talking about in this feature at all.)

  1. isAdminRoom verifies it of getRoomWelcomeMessage.

    App/src/libs/ReportUtils.js

    Lines 574 to 576 in 5c6b751

    } else if (isAdminRoom(report)) {
    welcomeMessage.phrase1 = Localize.translateLocal('reportActionsView.beginningOfChatHistoryAdminRoomPartOne', {workspaceName});
    welcomeMessage.phrase2 = Localize.translateLocal('reportActionsView.beginningOfChatHistoryAdminRoomPartTwo');

This doesn't directly answer your question, but more to point of what to do here, we'll want to create a new helper function:

function isAdminsOnlyPostingRoom(report) {
    const capability = lodashGet(report, 'writeCapability', CONST.REPORT.WRITE_CAPABILITIES.ALL) || CONST.REPORT.WRITE_CAPABILITIES.ALL;
    return capability === CONST.REPORT.WRITE_CAPABILITIES.ADMINS;
}

and use that function in a new condition in this logic:

} else if (isAdminsOnlyPostingRoom(report)) {
...
}
  1. phrase1 should be Use . Because ${roomName} should be presseble to navigate.
    {isChatRoom && (
    <>
    <Text>{roomWelcomeMessage.phrase1}</Text>
    <Text
    style={[styles.textStrong]}
    onPress={() => Navigation.navigate(ROUTES.getReportDetailsRoute(props.report.reportID))}
    >
    {ReportUtils.getReportName(props.report)}
    </Text>
    <Text>{roomWelcomeMessage.phrase2}</Text>
    </>
    )}
  2. phrase2 should be to hear about important announcements related to ${workspaceName}.

Good point about why we separate out the phrases. That looks correct, thanks!

@rezkiy37
Copy link
Contributor

@amyevans Got it. Thank you!

@rezkiy37
Copy link
Contributor

rezkiy37 commented Jun 29, 2023

Proposal

Please re-state the problem that we are trying to solve in this issue.

This is a feature issue. It has 4 tasks. Anyway, it resolves some user problems too:

  1. Provides new functionality - "Who can post" setting during creating a new room.
  2. Restricts to share "Assign task" in admin-only rooms if a current user is not admin itself.
  3. Reduces the spacing between the last message and the bottom of the page when Composer isn't present.
  4. Updates message the welcome message for admin-only rooms.

What is the root cause of that problem?

  1. Add the "Who can post" setting to the "New Room" form.
    • The default form value should be all.
    • Send the writeCapability value when calling the AddWorkspaceRoom endpoint.
    • Ensure the value is included in the optimisticData (to support offline creation).
    • Note: BE PRs supporting the writeCapability value in the AddWorkspaceRoom endpoint aren't live yet, but don't need to block work here.
  2. Filter out admin-only rooms for members in the "Share somewhere" selector of the "Assign task" action (accessible via the FAB menu) - so that members cannot share a task to the room.
  3. Reduce the padding between the last message and the bottom of the page when Composer isn't present (e.g. for the "member" view in an admin-only room).
  4. Update the room description to be Use ${roomName} to hear about important announcements related to ${workspaceName} for admin-only rooms.

What changes do you think we should make in order to solve the problem?

  1. Add the “Who can post” setting to the “New Room” form.
    a. WorkspaceNewRoomPage component.

    1. Add writeCapabilityOptions of render method. The array is mapped of all write capabilities. The map returns the array of objects: value (capability itself) and label (reuse already existed strings).
    const writeCapabilityOptions = _.map(CONST.REPORT.WRITE_CAPABILITIES, (value) => ({
        value,
        label: this.props.translate(`writeCapabilityPage.writeCapability.${value}`),
    }));
    1. Add one more Picker after RoomNameInput, because I consider to follow the same order of pickers as in ReportSettingsPage. It should reuse already existed title (Who can post). The default value should be - CONST.REPORT.WRITE_CAPABILITIES.ALL (all). It takes items (values) from writeCapabilityOptions.
    2. Change bottom spacing of workspaces Picker Wrapper (View) from mb5 to mb2 to keep consistency between them.
    3. Apply destructuring assignment of values of submit method. Because, the method always uses chains to get each value.
    4. Take and pass writeCapability to Report.addPolicyReport(…, writeCapability) in the submit method.

    b. addPolicyReport function of Report.js.

    1. Takes writeCapability as the last argument.
    2. Passes the argument after visibility to ReportUtils.buildOptimisticChatReport(…, writeCapability, ...).
    3. Passes the argument to API.write(‘AddWorkspaceRoom’, {…, writeCapability}).

    c. buildOptimisticChatReport function of ReportUtils.js.

    1. Takes writeCapability after visibility with the default value - CONST.REPORT.WRITE_CAPABILITIES.ALL (all).
    2. Returns writeCapability inside the returned object.
  2. Filter out admin-only rooms for members in the “Share somewhere” selector of the “Assign task” action (accessible via the FAB menu) - so that members cannot share a task to the room.
    a. shouldReportBeInOptionList function of ReportUtils.js.

    1. Takes one more argument - isShareDestination (the default value - false). Since, this function is being used across other places and other list of reports, I should apply changes only for the “Share somewhere” selector.
    2. Add one more condition before the final return. When isShareDestination is true and writeCapability equals admin-only, the app should verify that a current user can post to a report. Find linkedWorkspace by policy.id and report.policyID. Then, check a role of the current user for this workspace. The role should be equal admin to allow the user to share to this report, otherwise this report is out of the list.
    if (isShareDestination && report.writeCapability === CONST.REPORT.WRITE_CAPABILITIES.ADMINS) {
        const linkedWorkspace = _.find(policies, (policy) => policy && policy.id === report.policyID);
        const shouldAllowShare = lodashGet(linkedWorkspace, 'role', '') === CONST.POLICY.ROLE.ADMIN;
    
        return shouldAllowShare;
    }

    b. getOptions function of OptionsListUtils.js.

    1. Takes one more value of options object. It is isShareDestination (the default value - false). Since, this function is being used across other places and other list of reports, I should apply changes only for the “Share somewhere” selector.
    2. Passes the value to ReportUtils.shouldReportBeInOptionList(…, isShareDestination).

    c. getShareDestinationOptions function of OptionsListUtils.js.

    1. Passes isShareDestination to getOptions(..., {..., isShareDestination: true}).
  3. Reduce the padding between the last message and the bottom of the page when Composer isn’t present (e.g. for the “member” view in an admin-only room).
    a. Add pb0 (paddingBottom: 0) to spacing.js.
    b. Apply styles.pb0 to View with styles.p5 of ReportActionItemCreated, when no other messages and Composer is hidden.

    1. To understand that there is no messages yet, I would use lastMessageText of report and compare with an empty string.
    2. To understand that Composer is hidden, I would apply the same conditions as inside ReportFooter.
    const errors = lodashGet(props.report, 'errorFields.addWorkspaceRoom') || lodashGet(props.report, 'errorFields.createChat');
    const isArchivedRoom = ReportUtils.isArchivedRoom(props.report);
    const hideComposer = ReportUtils.shouldHideComposer(props.report, errors);
    const shouldOmitBottomSpace = props.report.lastMessageText === '' && (isArchivedRoom || hideComposer);
    <View
        accessibilityLabel={props.translate('accessibilityHints.chatWelcomeMessage')}
        // Apply the condition here
        style={[styles.p5, shouldOmitBottomSpace && styles.pb0, StyleUtils.getReportWelcomeTopMarginStyle(props.isSmallScreenWidth)]}
    >
  4. Update the room description to be Use ${roomName} to hear about important announcements related to ${workspaceName} for admin-only rooms.
    a. reportActionsView of en.js.

    1. Add beginningOfChatHistoryAdminOnlyPostingRoomPartOne - "Use ".
    2. Add beginningOfChatHistoryAdminOnlyPostingRoomPartTwo like a method to pass workspaceName - " to hear about important announcements related to ${workspaceName}".
      b. ReportUtils.js
    3. Create a function - isAdminsOnlyPostingRoom. It takes report and gets writeCapability or use default value - all. It compares writeCapability with admins to verify that only admins can post here.
    function isAdminsOnlyPostingRoom(report) {
        const writeCapability = lodashGet(report, 'writeCapability', CONST.REPORT.WRITE_CAPABILITIES.ALL);
    
        return writeCapability === CONST.REPORT.WRITE_CAPABILITIES.ADMINS;
    }
    1. Add one more else if to getRoomWelcomeMessage. It should use the new isAdminsOnlyPostingRoom function. When it returns true, it sets the first string (beginningOfChatHistoryAdminOnlyPostingRoomPartOne) to phrase1 and the second string (beginningOfChatHistoryAdminOnlyPostingRoomPartTwo) to phrase2.

What alternative solutions did you explore?

  1. Reduce the padding between the last message and the bottom of the page when Composer isn’t present (e.g. for the “member” view in an admin-only room).
    a. Remove wrapper of OfflineIndicator of ReportFooter. Remove styles.offlineIndicatorRow, because no wrapper anymore. The wrapper has a static height and is being rendered always on large screens. No sense to keep this empty wrapper when users are online.

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Jun 29, 2023
@rezkiy37
Copy link
Contributor

The 1st task example:

1.mov
1 1 1 2

@rezkiy37
Copy link
Contributor

The 2nd task example:

2.mp4

@rezkiy37
Copy link
Contributor

The 3rd task example:

3 3 1

@rezkiy37
Copy link
Contributor

The 4th task example:

4

@amyevans
Copy link
Contributor Author

amyevans commented Jun 29, 2023

Thanks for writing that out as a detailed proposal! I haven't had the opportunity to review it yet and will eventually want a C+ to help with the PR review, so going to pull 1 in now for proposal review as well, so that you won't be blocked.

Asked in Slack for a volunteer.

@mananjadhav
Copy link
Collaborator

Thanks for the detailed proposal @rezkiy37. Took sometime to go through all the points. Your proposal looks good.

cc - @amyevans

🎀 👀 🎀 C+ reviewed.

@melvin-bot
Copy link

melvin-bot bot commented Jun 30, 2023

Triggered auto assignment to @NikkiWines, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

@rezkiy37
Copy link
Contributor

@amyevans Hi!
Could you please provide me translations for Use ${roomName} to hear about important announcements related to ${workspaceName} in Spanish.

@melvin-bot melvin-bot bot added Weekly KSv2 and removed Weekly KSv2 labels Jul 31, 2023
@rezkiy37
Copy link
Contributor

@amyevans, @mananjadhav, I've opened a PR for review - #23919.

@jliexpensify
Copy link
Contributor

jliexpensify commented Aug 1, 2023

Payment summary:

  • Contributor: @rezkiy37 $500 (due to regression) - Callstack, to invoice serparately
  • C+: @mananjadhav $125 (due to regression) - New.Dot payment

No Upworks job created

@mananjadhav
Copy link
Collaborator

mananjadhav commented Aug 1, 2023

@jliexpensify The payout for me will be 125$, as there are two regressions and the payout date will change as the PR for the second regression is still open.

@melvin-bot melvin-bot bot added Weekly KSv2 and removed Weekly KSv2 labels Aug 1, 2023
@melvin-bot melvin-bot bot changed the title [HOLD for payment 2023-08-03] [$1000] Polish admins-only policy room feature [HOLD for payment 2023-08-08] [HOLD for payment 2023-08-03] [$1000] Polish admins-only policy room feature Aug 1, 2023
@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Aug 1, 2023
@melvin-bot
Copy link

melvin-bot bot commented Aug 1, 2023

Reviewing label has been removed, please complete the "BugZero Checklist".

@melvin-bot
Copy link

melvin-bot bot commented Aug 1, 2023

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.3.48-5 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2023-08-08. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

  • External issue reporter
  • Contributor that fixed the issue
  • Contributor+ that helped on the issue and/or PR

For reference, here are some details about the assignees on this issue:

  • @mananjadhav does not require payment (Eligable for Manual Requests)
  • @rezkiy37 does not require payment (Contractor)

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

  • Merged PR within 3 business days of assignment - 50% bonus
  • Merged PR more than 9 business days after assignment - 50% penalty

@melvin-bot melvin-bot bot added Daily KSv2 Overdue Weekly KSv2 and removed Weekly KSv2 Daily KSv2 labels Aug 3, 2023
@melvin-bot melvin-bot bot changed the title [HOLD for payment 2023-08-08] [HOLD for payment 2023-08-03] [$1000] Polish admins-only policy room feature [HOLD for payment 2023-08-14] [HOLD for payment 2023-08-08] [HOLD for payment 2023-08-03] [$1000] Polish admins-only policy room feature Aug 7, 2023
@melvin-bot melvin-bot bot removed the Overdue label Aug 7, 2023
@melvin-bot
Copy link

melvin-bot bot commented Aug 7, 2023

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.3.50-3 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2023-08-14. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

  • External issue reporter
  • Contributor that fixed the issue
  • Contributor+ that helped on the issue and/or PR

For reference, here are some details about the assignees on this issue:

  • @mananjadhav does not require payment (Eligable for Manual Requests)
  • @rezkiy37 does not require payment (Contractor)

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

  • Merged PR within 3 business days of assignment - 50% bonus
  • Merged PR more than 9 business days after assignment - 50% penalty

@mananjadhav
Copy link
Collaborator

There won't be any offending PR because this was more of a feature request. About regressions steps, @amyevans Can you confirm if these are added during the feature design?

@amyevans
Copy link
Contributor Author

About regressions steps, @amyevans Can you confirm if these are added during the feature design?

Good reminder, I haven't put in the request for new regression tests yet but I will Monday!

@jliexpensify
Copy link
Contributor

jliexpensify commented Aug 14, 2023

Payment summary here (to save me/JMills/Anu scrolling up)

@amyevans
Copy link
Contributor Author

Regression tests GH created: https://github.com/Expensify/Expensify/issues/308182

I think we're set to proceed with payment today!

@amyevans amyevans added Daily KSv2 and removed Weekly KSv2 Help Wanted Apply this label when an issue is open to proposals by contributors labels Aug 14, 2023
@jliexpensify
Copy link
Contributor

Cool, so there isn't anything to pay in Upworks - @mananjadhav is getting paid in New.Dot and @rezkiy37 is invoicing us.

@amyevans are you happy to close this one, if everything is done?

@amyevans
Copy link
Contributor Author

Sure, I wasn't sure if we had to keep it open until the payment was made in NewDot. Closing now!

@JmillsExpensify
Copy link

Reviewed the details for @mananjadhav. Approved for payment in NewDot based on BZ summary above.

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 Daily KSv2 External Added to denote the issue can be worked on by a contributor NewFeature Something to build that is a new item.
Projects
None yet
Development

No branches or pull requests

8 participants