[HOLD for payment 2023-08-10] [$1000] Chat - Account B will have a pencil saved when only the admin can write in the announcement room #21835

lanitochka17 · 2023-06-28T19:25:50Z

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

Issue foung when executing PR #21376

Action Performed:

Login with user A --> create new workspace--> invite user B
Open announce room of above workspace --> detail chat --> settting --> who can post --> choose All member
Login with user B on another device --> Open announce chat of above workspace --> Write something
On user A choose who can post --> admin only

Expected Result:

The pencil should disappear if only admin A can send messages

Actual Result:

When only the administrator can write in the message room, and user B has left a message in the compose box, the pencil does not disappear

Workaround:

Unknown

Platforms:

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

Version Number: 1.3.34.1

Reproducible in staging?: Yes

Reproducible in production?: Yes

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

Recording.2268.mp4

Expensify/Expensify Issue URL:

Issue reported by: Applause - Internal Team

Slack conversation:

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~01db389064c4161941
Upwork Job ID: 1676726165094825984
Automatic offers:

| | 0
Nikhil-Vats | Contributor | 25719953

melvin-bot · 2023-06-28T19:25:55Z

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

melvin-bot · 2023-06-28T19:25:57Z

Bug0 Triage Checklist (Main S/O)

This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
This bug is not a duplicate report (check E/App issues and #expensify-bugs)
- If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
This bug is reproducible using the reproduction steps in the OP. S/O
- If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
- If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
This issue is filled out as thoroughly and clearly as possible
- Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

jeet-dhandha · 2023-06-28T20:11:05Z

Proposal

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

Problem is that hasDraftComment in OptionRowLHN remain's true even after the permission to write is given only to admins.

What is the root cause of that problem?

The root cause of the problem is that on permission change the hasDraft key in report is not updated and remains true.

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

We can use the isAllowedToComment function to check if the user is allowed to comment or not and then render Draft icon accordingly. (This is similar to what we did with ReportActionCompose. )

const report = ReportUtils.getReport(props.reportID);
const isAllowedToComment = ReportUtils.isAllowedToComment(report);

App/src/components/LHNOptionsList/OptionRowLHN.js

Lines 246 to 253 in 19ca306

    
           {optionItem.hasDraftComment && ( 
        
               <View 
        
                   style={styles.ml2} 
        
                   accessibilityLabel={props.translate('sidebarScreen.draftedMessage')} 
        
               > 
        
                   <Icon src={Expensicons.Pencil} /> 
        
               </View> 
        
           )}

-  {optionItem.hasDraftComment && (
+  {optionItem.hasDraftComment && isAllowedToComment && (

What alternative solutions did you explore? (Optional)

Other solution could be we handle changes in backend to set the hasDraft key to false when the permission is changed for a report.

Nikhil-Vats · 2023-06-28T20:52:54Z

Proposal

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

If a user has a draft message in a room and the room permission is changed to admin only then the draft indicator (pencil icon) is not removed.

What is the root cause of that problem?

We show the pencil icon (draft message indicator) without checking if room is archived, has errors or permissions -

App/src/components/LHNOptionsList/OptionRowLHN.js

Lines 246 to 253 in 53e4154

    
           {optionItem.hasDraftComment && ( 
        
               <View 
        
                   style={styles.ml2} 
        
                   accessibilityLabel={props.translate('sidebarScreen.draftedMessage')} 
        
               > 
        
                   <Icon src={Expensicons.Pencil} /> 
        
               </View> 
        
           )}

So if the room is archived, has any errors, or permission is changed to admin only then composer will be hidden but draft icon will show if user had a draft message before.

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

We use the following function to hide the composer when room is archived, report has any errors or permissions are changed to admin only -

App/src/libs/ReportUtils.js

Lines 2319 to 2327 in 53e4154

    
           /** 
        
            * Return true if the composer should be hidden 
        
            * @param {Object} report 
        
            * @param {Object} reportErrors 
        
            * @returns {Boolean} 
        
            */ 
        
           function shouldHideComposer(report, reportErrors) { 
        
               return isArchivedRoom(report) || !_.isEmpty(reportErrors) || !isAllowedToComment(report); 
        
           }

We should use the same function to remove the draft icon dynamically in all 3 cases.

In the OptionRowLHN component, we have only reportID. To get report, reportErrors we can use the getReport function in ReportUtils and then we can get the reportErrors from the report object's errorFields key in the same way we are getting it for composer.

App/src/libs/ReportUtils.js

Lines 1134 to 1136 in 53e4154

    
           function getReport(reportID) { 
        
               return lodashGet(allReports, `${ONYXKEYS.COLLECTION.REPORT}${reportID}`, {}); 
        
           }

const hideDraftIcon = ReportUtils.shouldHideComposer(report, reportErrors);
// ... rest of the code
{optionItem.hasDraftComment && !hideDraftIcon && (
    <View
        style={styles.ml2}
        accessibilityLabel={props.translate('sidebarScreen.draftedMessage')}
    >
        <Icon src={Expensicons.Pencil} />
    </View>
)}

We can also move the optionItem.hasDraftComment logic to hideDraftIcon variable and render the draft icon using -

{!hideDraftIcon && ...
...
}

Additionally, we can rename the shouldHideComposer to something more generic if needed.

What alternative solutions did you explore? (Optional)

NA

mallenexpensify · 2023-06-30T03:34:12Z

Interesting. Having troubles testing cuz, when I invite use B to the workspace, I don't see them as a member but User B has access to the workspace and announce room. Anyone know what might be going wrong?

User B vie

User A view

melvin-bot · 2023-07-03T20:11:54Z

@mallenexpensify Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

Nikhil-Vats · 2023-07-04T21:38:25Z

@mallenexpensify I just tested what you said locally and it didn't happen for me. Would you mind trying again?

jeet-dhandha · 2023-07-05T03:50:40Z

@Nikhil-Vats - Still reproducible.

Nikhil-Vats · 2023-07-05T03:52:11Z

Yes @jeet-dhandha . The issue is still reproducible. I meant the other issue that @mallenexpensify was talking about was not reproducible for me.

melvin-bot · 2023-07-05T20:10:29Z

@mallenexpensify Huh... This is 4 days overdue. Who can take care of this?

melvin-bot · 2023-07-05T22:54:06Z

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

melvin-bot · 2023-07-05T22:54:11Z

Triggered auto assignment to Contributor Plus for review of internal employee PR - @parasharrajat (Internal)

mallenexpensify · 2023-07-05T22:54:48Z

Thanks @jeet-dhandha and @Nikhil-Vats , I was able to reproduce.
@parasharrajat , do you think this can be external? If so, will you please comment and also review the proposals above?

mallenexpensify · 2023-07-11T04:36:27Z

@parasharrajat , do you think this can be external?

parasharrajat · 2023-07-11T07:35:58Z

Yes. It can be. I reviewed both proposals. @Nikhil-Vats We don't have to check for errors and archived status that is already handled.

This problem can be solved both from the front end and back end. There are two ways to solve it frontend.

Either we do as suggested in the above proposals.
Or we can detect when the write permission has changed and clear the drafts.

The best solution for this problem is to clear the draft instead of hiding it. I think can be done on the backend when we are sending the updated write capabilities.

IMO, it will be best to do that on the backend if possible.

@mallenexpensify There is no engineer assigned. I think we should assign an engineer immediately for internal issues.

Nikhil-Vats · 2023-07-11T10:04:25Z

@Nikhil-Vats We don't have to check for errors and archived status that is already handled.

@parasharrajat in the code, we are not checking for errors or archived status before showing the draft icon. So, it is possible to show draft icon with error status when composer is hidden. So, for completeness and consistency we should also hide it in these cases.

I could also reproduce it if I entered an invalid email like - a_@_.com.

We are already using shouldHideComposer to hide composer, emoji picker, split bill modals, etc. in RHP (if a user is splitting the bill in a room and permissions are changed). So, we can use the same function for hiding draft icon too.

jeet-dhandha · 2023-07-11T10:07:55Z

@parasharrajat we can go with both changes as clearing drafts might take some time so till than the UI handles the rendering of draft icon.

melvin-bot · 2023-07-12T20:07:46Z

@mallenexpensify @parasharrajat this issue was created 2 weeks ago. Are we close to a solution? Let's make sure we're treating this as a top priority. Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

melvin-bot · 2023-07-14T20:08:53Z

@mallenexpensify, @parasharrajat Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

parasharrajat · 2023-07-27T19:57:30Z

@Nikhil-Vats Any update on the PR?

Nikhil-Vats · 2023-07-27T20:00:06Z

Sorry @parasharrajat, I had mild fever these past two days so couldn't dedicate a lot of time but I am better now and working on this right now and will have the PR ready for review in morning.

parasharrajat · 2023-07-27T20:01:34Z

No need to be sorry but it is always better to leave an update.

Nikhil-Vats · 2023-07-27T20:37:09Z

Sure, I will remember that going forward.

melvin-bot · 2023-08-01T15:21:09Z

Based on my calculations, the pull request did not get merged within 3 working days of assignment. Please, check out my computations here:

when @Nikhil-Vats got assigned: 2023-07-24 14:36:06 Z
when the PR got merged: 2023-08-01 15:21:09 UTC
days elapsed: 6

On to the next one 🚀

melvin-bot · 2023-08-03T17:05:27Z

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

melvin-bot · 2023-08-03T17:05:31Z

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

Fix: hide draft icon when composer hides #23641

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

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:

@parasharrajat does not require payment (Eligable for Manual Requests)
@Nikhil-Vats requires payment offer (Contributor)

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 · 2023-08-03T17:05:33Z

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

[@parasharrajat] The PR that introduced the bug has been identified. Link to the PR:
[@parasharrajat] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
[@parasharrajat] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
[@parasharrajat] Determine if we should create a regression test for this bug.
[@parasharrajat] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
[@mallenexpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon: https://github.com/Expensify/Expensify/issues/308702

parasharrajat · 2023-08-09T08:50:53Z

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

[@parasharrajat] The PR that introduced the bug has been identified. Link to the PR: Add ability for admins to restrict who can post within rooms #19094
[@parasharrajat] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment: Add ability for admins to restrict who can post within rooms #19094 (comment)
[@parasharrajat] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion: https://expensify.slack.com/archives/C049HHMV9SM/p1691570805036839
[@parasharrajat] Determine if we should create a regression test for this bug. Yes
[@parasharrajat] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.

Regression Test Steps

Open the app and log in using an account which is an admin of a room with permissions set to all members.
On the second tab/device, log in using an account that is a member of the same room above.
On the second tab/device, navigate to the room chat and draft a message in the composer. Verify that the pencil icon is visible.
On the first tab/device, open the room settings and change permission to admins only.
On the second tab/device, verify that the composer and the pencil icon are not visible anymore.
Now on the first device, change permissions back to all members.
Go back to the second tab/device, verify that the composer and draft icon reappear and you can see your draft message.

Note: If you log out on a second device after step 5, then you won't see drafts after step 7 because drafts are not persisted after logout.

Do you agree 👍 or 👎 ?

parasharrajat · 2023-08-09T08:52:11Z

Payment requested.

madmax330 · 2023-08-14T22:10:46Z

Not overdue

JmillsExpensify · 2023-08-16T03:13:03Z

@mallenexpensify Can you please provide a payment summary so I can get @parasharrajat paid via NewDot?

mallenexpensify · 2023-08-16T21:12:05Z

Issue reporter: Applause
Contributor: @Nikhil-Vats paid $1000 via Upwork
Contributor+: @parasharrajat is due $1000

JmillsExpensify · 2023-08-17T02:45:54Z

Reviewed the details for @parasharrajat. $1,000 approved for payment in NewDot based on the BZ summary above.

parasharrajat · 2023-08-18T08:12:35Z

We are good to close this issue. @madmax330

lanitochka17 added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Jun 28, 2023

melvin-bot bot assigned mallenexpensify Jun 28, 2023

melvin-bot bot added the Overdue label Jul 3, 2023

mallenexpensify added the Internal Requires API changes or must be handled by Expensify staff label Jul 5, 2023

melvin-bot bot assigned parasharrajat Jul 5, 2023

melvin-bot bot added Overdue and removed Overdue labels Jul 5, 2023

melvin-bot bot removed the Overdue label Jul 11, 2023

melvin-bot bot added the Overdue label Jul 13, 2023

mallenexpensify added the Engineering label Jul 17, 2023

melvin-bot bot added Weekly KSv2 and removed Daily KSv2 labels Jul 26, 2023

melvin-bot bot added Weekly KSv2 and removed Weekly KSv2 labels Jul 27, 2023

melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Aug 3, 2023

melvin-bot bot changed the title ~~[$1000] Chat - Account B will have a pencil saved when only the admin can write in the announcement room~~ [HOLD for payment 2023-08-10] [$1000] Chat - Account B will have a pencil saved when only the admin can write in the announcement room Aug 3, 2023

melvin-bot bot removed the Reviewing Has a PR in review label Aug 3, 2023

parasharrajat mentioned this issue Aug 9, 2023

Add ability for admins to restrict who can post within rooms #19094

Merged

57 tasks

melvin-bot bot added Daily KSv2 Overdue and removed Weekly KSv2 labels Aug 9, 2023

melvin-bot bot removed the Overdue label Aug 14, 2023

madmax330 closed this as completed Aug 18, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[HOLD for payment 2023-08-10] [$1000] Chat - Account B will have a pencil saved when only the admin can write in the announcement room #21835

[HOLD for payment 2023-08-10] [$1000] Chat - Account B will have a pencil saved when only the admin can write in the announcement room #21835

lanitochka17 commented Jun 28, 2023 •

edited by melvin-bot bot

Loading

melvin-bot bot commented Jun 28, 2023

melvin-bot bot commented Jun 28, 2023

jeet-dhandha commented Jun 28, 2023 •

edited

Loading

Nikhil-Vats commented Jun 28, 2023 •

edited

Loading

mallenexpensify commented Jun 30, 2023

melvin-bot bot commented Jul 3, 2023

Nikhil-Vats commented Jul 4, 2023

jeet-dhandha commented Jul 5, 2023

Nikhil-Vats commented Jul 5, 2023

melvin-bot bot commented Jul 5, 2023

melvin-bot bot commented Jul 5, 2023

melvin-bot bot commented Jul 5, 2023

mallenexpensify commented Jul 5, 2023

mallenexpensify commented Jul 11, 2023

parasharrajat commented Jul 11, 2023 •

edited

Loading

Nikhil-Vats commented Jul 11, 2023 •

edited

Loading

jeet-dhandha commented Jul 11, 2023

melvin-bot bot commented Jul 12, 2023

melvin-bot bot commented Jul 14, 2023

parasharrajat commented Jul 27, 2023

Nikhil-Vats commented Jul 27, 2023

parasharrajat commented Jul 27, 2023

Nikhil-Vats commented Jul 27, 2023

melvin-bot bot commented Aug 1, 2023

melvin-bot bot commented Aug 3, 2023

melvin-bot bot commented Aug 3, 2023

melvin-bot bot commented Aug 3, 2023 •

edited by mallenexpensify

Loading

parasharrajat commented Aug 9, 2023

parasharrajat commented Aug 9, 2023 •

edited

Loading

madmax330 commented Aug 14, 2023

JmillsExpensify commented Aug 16, 2023

mallenexpensify commented Aug 16, 2023

JmillsExpensify commented Aug 17, 2023

parasharrajat commented Aug 18, 2023

[HOLD for payment 2023-08-10] [$1000] Chat - Account B will have a pencil saved when only the admin can write in the announcement room #21835

[HOLD for payment 2023-08-10] [$1000] Chat - Account B will have a pencil saved when only the admin can write in the announcement room #21835

Comments

lanitochka17 commented Jun 28, 2023 • edited by melvin-bot bot Loading

Action Performed:

Expected Result:

Actual Result:

Workaround:

Platforms:

melvin-bot bot commented Jun 28, 2023

melvin-bot bot commented Jun 28, 2023

Bug0 Triage Checklist (Main S/O)

jeet-dhandha commented Jun 28, 2023 • edited Loading

Proposal

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

What is the root cause of that problem?

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

What alternative solutions did you explore? (Optional)

Nikhil-Vats commented Jun 28, 2023 • edited Loading

Proposal

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

What is the root cause of that problem?

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

What alternative solutions did you explore? (Optional)

mallenexpensify commented Jun 30, 2023

melvin-bot bot commented Jul 3, 2023

Nikhil-Vats commented Jul 4, 2023

jeet-dhandha commented Jul 5, 2023

Nikhil-Vats commented Jul 5, 2023

melvin-bot bot commented Jul 5, 2023

melvin-bot bot commented Jul 5, 2023

melvin-bot bot commented Jul 5, 2023

mallenexpensify commented Jul 5, 2023

mallenexpensify commented Jul 11, 2023

parasharrajat commented Jul 11, 2023 • edited Loading

Nikhil-Vats commented Jul 11, 2023 • edited Loading

jeet-dhandha commented Jul 11, 2023

melvin-bot bot commented Jul 12, 2023

melvin-bot bot commented Jul 14, 2023

parasharrajat commented Jul 27, 2023

Nikhil-Vats commented Jul 27, 2023

parasharrajat commented Jul 27, 2023

Nikhil-Vats commented Jul 27, 2023

melvin-bot bot commented Aug 1, 2023

melvin-bot bot commented Aug 3, 2023

melvin-bot bot commented Aug 3, 2023

melvin-bot bot commented Aug 3, 2023 • edited by mallenexpensify Loading

parasharrajat commented Aug 9, 2023

Regression Test Steps

parasharrajat commented Aug 9, 2023 • edited Loading

madmax330 commented Aug 14, 2023

JmillsExpensify commented Aug 16, 2023

mallenexpensify commented Aug 16, 2023

JmillsExpensify commented Aug 17, 2023

parasharrajat commented Aug 18, 2023

lanitochka17 commented Jun 28, 2023 •

edited by melvin-bot bot

Loading

jeet-dhandha commented Jun 28, 2023 •

edited

Loading

Nikhil-Vats commented Jun 28, 2023 •

edited

Loading

parasharrajat commented Jul 11, 2023 •

edited

Loading

Nikhil-Vats commented Jul 11, 2023 •

edited

Loading

melvin-bot bot commented Aug 3, 2023 •

edited by mallenexpensify

Loading

parasharrajat commented Aug 9, 2023 •

edited

Loading