Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.56k stars 2.9k forks source link

[$1000] Copy update for #announce rooms #23819

Closed kavimuru closed 1 year ago

kavimuru commented 1 year ago

If you havenโ€™t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Action Performed:

  1. Open a tab where account A is signed in
  2. Create workspace > invite account B to the workspace > navigate to announce room of the workspace
  3. Observe the copy above the composer 'Use #announce to chat about anything'
  4. Click on announce room header > click on 'Settings' > click on 'Who can post' link > click on 'Admins only'
  5. Observe the copy above the composer for admin account A & member account B

    Expected Result:

    Copy change (approved here https://github.com/Expensify/App/issues/23819#issuecomment-1661146413): Update Say Hello to Welcome to #announce! Update: Use #announce to chat about anything to Only admins can send messages in this channel

    Actual Result:

    'Use #announce to hear about important announcements message is displayed even though members can't send messages since setting is changed to admins only

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

Platforms:

Which of our officially supported platforms is this issue occurring on?

Version Number: 1.3.47-2 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 Notes/Photos/Videos: Any additional supporting documentation

https://github.com/Expensify/App/assets/43996225/d8776185-679f-45cd-9159-96b343df5f5a

Expensify/Expensify Issue URL: Issue reported by: @natnael-guchima Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1690466219403929

View all open jobs on GitHub Snip - (1) New Expensify - Google Chrome (2)

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01b1525bd4d7a0e8a8
  • Upwork Job ID: 1686714666554503168
  • Last Price Increase: 2023-08-02
  • Automatic offers:
    • samh-nl | Contributor | 25996931
    • natnael-guchima | Reporter | 25996933
    • ishpaul777 | Contributor | 26136150
melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

Bug0 Triage Checklist (Main S/O)

samh-nl commented 1 year ago

Proposal

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

Contradicting instruction in announce room

What is the root cause of that problem?

For displaying the room welcome message when the room is admin-only, we implement this logic:

https://github.com/Expensify/App/blob/24ed1bebf19539d27878ed419f7cc10ae942531f/src/libs/ReportUtils.js#L704-L706

However, we do not make a distinction with if the user is an admin, so that we can present the user a different welcome message.

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

We should define a new welcome message here that we should display if it is an admin-only room and the user is an admin.

Changes:

  1. In ReportActionItemCreated, pass props.policy as a prop to ReportWelcomeText, and subsequently to getRoomWelcomeMessage via a new argument.
  2. Conditionally display the new welcome message here, checking PolicyUtils.isPolicyAdmin. To target it specifically to the #announce room, we should also check isAnnounceRoom(report).

What alternative solutions did you explore? (Optional)

N/A

melvin-bot[bot] commented 1 year ago

Triggered auto assignment to @joaniew (Waiting for copy), see https://stackoverflow.com/c/expensify/questions/7025/ for more details.

sonialiap commented 1 year ago

@joaniew tagging you in for copy regarding updating the #announce room description. Should we update it? Imo the reporter does have a point that saying "Use announce to make announcements" doesn't make sense as a statement for employees.

If we do want to change it, do we want two different copies, one for users and one for admins, or one that works for both user types?

If we want two do we want something along the lines of,

ghost commented 1 year ago

What if we did something like

sonialiap commented 1 year ago

Looks good, thanks, Joanie

melvin-bot[bot] commented 1 year ago

Job added to Upwork: https://www.upwork.com/jobs/~01b1525bd4d7a0e8a8

melvin-bot[bot] commented 1 year ago

Current assignee @sonialiap is eligible for the External assigner, not assigning anyone new.

melvin-bot[bot] commented 1 year ago

Triggered auto assignment to Contributor-plus team member for initial proposal review - @eVoloshchak (External)

ishpaul777 commented 1 year ago

Proposal

Problem

Copy update for announce room

Root cause

no Root cause since its a copy update only

Changes

  1. if the change is specific to Announce room only we need to update the conditions here
else if (isAnnounceRoom(report)) {
        if (!isAdminsOnlyPostingRoom(report)) {
            welcomeMessage.phrase1 = Localize.translateLocal('reportActionsView.beginningOfChatHistoryAnnounceRoomPartOne', {workspaceName});
            welcomeMessage.phrase2 = Localize.translateLocal('reportActionsView.beginningOfChatHistoryAnnounceRoomPartTwo', {workspaceName});
        } else {
            welcomeMessage.phrase1 = Localize.translateLocal('reportActionsView.beginningOfChatHistoryAdminOnlyPostingAnnounceRoomPartOne');
            welcomeMessage.phrase2 = Localize.translateLocal('reportActionsView.beginningOfChatHistorAdminOnlyPostingAnnounceRoomPartTwo', {workspaceName});
        } 
    }
    else if (isAdminsOnlyPostingRoom(report)) {
        welcomeMessage.phrase1 = Localize.translateLocal('reportActionsView.beginningOfChatHistoryAdminOnlyPostingRoomPartOne');
        welcomeMessage.phrase2 = Localize.translateLocal('reportActionsView.beginningOfChatHistoryAdminOnlyPostingRoomPartTwo', {workspaceName});
    }
  1. modify here to change "SayHello".
   // if we are making change to all admins only posting announce rooms, we need to update
    // const isAdminsOnlyPostingAnnounceRoom = ReportUtils.isAdminsOnlyPostingRoom(props.report) && ReportUtils.isAnnounceRoom(props.report);
    // if we are making change to all admins only posting rooms, we need to update
    const isAdminsOnlyPostingRoom = ReportUtils.isAdminsOnlyPostingRoom(props.report);
    const isAnnounceRoom = ReportUtils.isAnnounceRoom(props.report);

    return (
        <>
            <View>
                {/* if we are making change to all admins only posting announce rooms
                <Text style={[styles.textHero]}>
                    {isAdminsOnlyPostingAnnounceRoom ? props.translate('reportActionsView.welcomeToAnnounceRoom') : props.translate('reportActionsView.sayHello')}
                </Text> */}
                {/* if we are making change to all admins only posting rooms */}
                <Text style={[styles.textHero]}>
                    {isAdminsOnlyPostingRoom || isAnnounceRoom ? props.translate('reportActionsView.welcomeToRoom', ReportUtils.getReportName(props.report)) : props.translate('reportActionsView.sayHello')}
                </Text>
            </View>
               //  ......... rest of code
                {isChatRoom && (
                    <>
                        <Text>{roomWelcomeMessage.phrase1}</Text>
                        {!isAdminsOnlyPostingRoom && (
                            <Text
                                style={[styles.textStrong]}
                                onPress={() => Navigation.navigate(ROUTES.getReportDetailsRoute(props.report.reportID))}
                            >
                                {ReportUtils.getReportName(props.report)}
                            </Text>
                        )}
                        <Text>{roomWelcomeMessage.phrase2}</Text>
                    </>
                )}
          // ... rest of code 

IMO, we should target all rooms in which only admins can post and change welcomeText to Only admins can send message in this room. instead of channel for all rooms in which only admins can post. We won't need to make the first change on have to update translations in en.js and es.js.

 beginningOfChatHistoryAdminOnlyPostingRoomPartOne: 'Only admins can send message in ',
 beginningOfChatHistoryAdminOnlyPostingRoomPartTwo: 'this room.',
 welcomeToRoom: ({workspaceName}) => `Welcome to ${workspaceName}!`,

Result -

Hero Text -

Body text -

https://github.com/Expensify/App/assets/104348397/4f197873-1a77-4333-aceb-2a5cc801d9e2

melvin-bot[bot] commented 1 year ago

@joaniew, @eVoloshchak, @sonialiap Huh... This is 4 days overdue. Who can take care of this?

eVoloshchak commented 1 year ago

Both proposals are quite similar, since it's just a simple text label update. I think we can proceed with the first proposal, which was posted by @samh-nl

๐ŸŽ€๐Ÿ‘€๐ŸŽ€ C+ reviewed!

melvin-bot[bot] commented 1 year ago

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

marcochavezf commented 1 year ago

Thanks for the review @eVoloshchak, assigning @samh-nl ๐Ÿš€

melvin-bot[bot] commented 1 year ago

๐Ÿ“ฃ @eVoloshchak Please request via NewDot manual requests for the Reviewer role ($1000)

melvin-bot[bot] commented 1 year ago

๐Ÿ“ฃ @samh-nl ๐ŸŽ‰ 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 ๐Ÿ“–

melvin-bot[bot] commented 1 year ago

๐Ÿ“ฃ @natnael-guchima ๐ŸŽ‰ An offer has been automatically sent to your Upwork account for the Reporter role ๐ŸŽ‰ Thanks for contributing to the Expensify app!

Offer link Upwork job

ishpaul777 commented 1 year ago

@eVoloshchak I still think both proposals are quite different in implementation my proposal was detailed enough with a Plan of action. Requesting you to reevaluate the proposal.Also fixing text only for announce room isn't fix the root cause of contradicting welcome message. (Other room created by user in which only admin can post) still has "say hello"

ishpaul777 commented 1 year ago

@marcochavezf @eVoloshchak please take a look here.

samh-nl commented 1 year ago

Update Say Hello to Welcome to #announce! Update: Use #announce to chat about anything to Only admins can send messages in this channel

Thank you, I have requested Spanish translations in this Slack thread. If there is an expedited internal way of having them translated, please do so.

samh-nl commented 1 year ago

Already translated in Slack! Just an update to prevent double work from being done.

ishpaul777 commented 1 year ago

@eVoloshchak i am expecting a clarification i have gone through the raised PR none of the proposed changes are added by @samh-nl rather he added changes i proposed this feels unfair

eVoloshchak commented 1 year ago

I'm on it right now @ishpaul777, apologies for the delay

eVoloshchak commented 1 year ago

The proposal doesn't have to contain the exact code snippet or an exhaustive list of cases where a fix needs to be applied. So generally, the first proposal to identify the root cause and propose a general idea on how to fix it - is a winning proposal. In this case it's a bit simpler, since the task was just to update a text label. That's why initially I chose @samh-nl proposal, even though it lacked the full technical details. However, raised PR does indeed contain the changes proposed by @ishpaul777's proposal Moving forward I would recommend sticking to the original proposals while implementing the solution, so that we're fair in any process without any conflicts

This is my mistake, since @ishpaul777's proposal is the correct one, proposing to use isAdminsOnlyPostingRoom initially. @marcochavezf, could you reassign @ishpaul777 to this job please? Sorry for the confusion everyone

ishpaul777 commented 1 year ago

thanks @eVoloshchak

samh-nl commented 1 year ago

@eVoloshchak While I will abide by any decision you make, the exact language edits only become apparent after my proposal was made, and those edits can only be implemented in one way.

ishpaul777 commented 1 year ago

@marcochavezf can you assign me please i'll raise a PR immediately

ishpaul777 commented 1 year ago

Gentle Bump to @marcochavezf. Thanks

marcochavezf commented 1 year ago

Thank you to @eVoloshchak for taking the time to re-evaluate the proposal, and thanks to @ishpaul777 for seeking clarification. Reviews can sometimes be challenging, and the details might not always be clear until the PR is raised. I appreciate everyone's hard work on this, and I'll assign this to @ishpaul777. Let's keep up the great teamwork!

melvin-bot[bot] commented 1 year ago

๐Ÿ“ฃ @eVoloshchak Please request via NewDot manual requests for the Reviewer role ($1000)

melvin-bot[bot] commented 1 year ago

๐Ÿ“ฃ @ishpaul777 ๐ŸŽ‰ 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 ๐Ÿ“–

ishpaul777 commented 1 year ago

Thanks @marcochavezf will raise a PR in few hours. Just want to confirm copy -

Hero Text -

if announce room (anyone can send or admin can send only) or a room in which only Admin can send - "Welcome to #room_name!"

Body text -

if announce room all can send (not changed) if any room all member can send (not changed) if announce room or any other room only admins can send - "Only admins can send messages in this room."

also can i get spanish copy confirmation?

ishpaul777 commented 1 year ago

cc - @joaniew @sonialiap for confirmation of copy and spanish translation.

marcochavezf commented 1 year ago

@ishpaul777

"Welcome to #room_name!" -> "Bienvenido a #room_name!"

"Only admins can send messages in this room." -> "Solo los administrators pueden enviar mensajes en esta sala"

ishpaul777 commented 1 year ago

Thanks @marcochavezf. Again I want to confirm if the announce room copy is "Welcome to #announce!" for both cases (admin only and all members). can you confirm this too?

also please confirm if "Welcome to #room_name!" -> "Bienvenido a #room_name!" or "ยกBienvenido a #room_name!" because i noticed "Say Hello!" -> "ยกSaluda!"

eVoloshchak commented 1 year ago

@ishpaul777, according to this slack thread

ishpaul777 commented 1 year ago

Thanks @eVoloshchak. Can you confirm the other query too??

eVoloshchak commented 1 year ago

"Welcome to #announce!" for both cases (admin only and all members)

Yes, I believe it should be Welcome to #announce! for both cases

ishpaul777 commented 1 year ago

Okay Thanks! @eVoloshchak

I just noticed "Only admins can send messages in this channel" is different in slack thread and what @marcochavezf provided

notice the word administradores and administrators. I am going with administradores because thats what we are using here:

Screenshot 2023-08-17 at 11 40 02 PM
eVoloshchak commented 1 year ago

The automation for this didn't work, The payment date should be 31 Aug This also has caused one regression: https://github.com/Expensify/App/pull/24643#issuecomment-1688999930

ishpaul777 commented 1 year ago

@marcochavezf gentle bump for pay reminder

ishpaul777 commented 1 year ago

@sonialiap @marcochavezf gentle bump

eVoloshchak commented 1 year ago

@marcochavezf, friendly bump on the above

eVoloshchak commented 1 year ago

@marcochavezf, @sonialiap, gentle bump for payment

Natnael-Guchima commented 1 year ago

@sonialiap A gentle bump for payment

eVoloshchak commented 1 year ago

@sonialiap, bump on the above

eVoloshchak commented 1 year ago

Bumping on Slack

sonialiap commented 1 year ago

Thank you for the slack bump, Eugene! My apologies for the delay, everyone ๐Ÿ™‡

@Natnael-Guchima report $250 - paid โœ”๏ธ @eVoloshchak review -regression $500 - ~offer sent~ offer canceled, to be paid via NewDot @ishpaul777 fix -regression $500 - paid โœ”๏ธ

No time bonus due to regression. Regression found within 24hr of PR merge

image
Natnael-Guchima commented 1 year ago

Accepted the offer. Thanks @sonialiap