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

#announce room is not created when creating a new workspace - Reported by: @Santhosh-Sellavel #5946

Closed
isagoico opened this issue Oct 19, 2021 · 28 comments
Assignees
Labels
Daily KSv2 Engineering Reviewing Has a PR in review

Comments

@isagoico
Copy link

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. Log in with a new account
  2. Create a workspace
  3. Add a member to the workspace
  4. Search for the #announce room

Expected Result:

An announce room should be created for the workspace.

Actual Result:

Announce room was not created for the workspace.

Workaround:

Unknown.

Platform:

Where is this issue occurring?

  • Web
  • iOS
  • Android
  • Desktop App
  • Mobile Web

Version Number: 1.1.8-0

Reproducible in staging?: Yes
Reproducible in production?: Yes

Logs: https://stackoverflow.com/c/expensify/questions/4856

Notes/Photos/Videos: Any additional supporting documentation

Recording.229.mp4

Expensify/Expensify Issue URL:

Issue reported by: @Santhosh-Sellavel
Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1634233681292300

View all open jobs on GitHub

@MelvinBot
Copy link

Triggered auto assignment to @Dal-Papa (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

@Dal-Papa
Copy link

Early investigation says that it exists :

[chatReportIDAnnounce] => 81995234

Looked at the logs of creation of your workspace and it's looking like it worked. Will see if the frontend is not showing it for some reason.

@Santhosh-Sellavel
Copy link
Collaborator

Even after created announce room is not visible in the chat list.
Still, I can find them in search but still not a visible chat list.

@Dal-Papa
Copy link

Haven't had time to look at this, can you confirm this bug is still reproducible on v1.1.8-9 ?

@MelvinBot MelvinBot added Overdue and removed Overdue labels Oct 25, 2021
@Dal-Papa
Copy link

Bump @Santhosh-Sellavel

@MelvinBot MelvinBot removed the Overdue label Oct 28, 2021
@Santhosh-Sellavel
Copy link
Collaborator

Yes, still the created room is not visible in the chat list.

@Santhosh-Sellavel
Copy link
Collaborator

Also could not find the #announce chat room after workspace creation.

Screen.Recording.2021-10-28.at.12.39.53.PM.mov

@Santhosh-Sellavel
Copy link
Collaborator

cc: @Dal-Papa

@Dal-Papa
Copy link

Dal-Papa commented Nov 1, 2021

I'm going to have to pass that one on to someone else as I have some external deadline commitment.

@MelvinBot MelvinBot removed the Overdue label Nov 1, 2021
@Dal-Papa Dal-Papa added Weekly KSv2 and removed Daily KSv2 labels Nov 1, 2021
@Dal-Papa Dal-Papa removed their assignment Nov 1, 2021
@MelvinBot MelvinBot added Monthly KSv2 and removed Weekly KSv2 labels Nov 24, 2021
@MelvinBot
Copy link

This issue has not been updated in over 15 days. eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

@K4tsuki
Copy link
Contributor

K4tsuki commented Dec 10, 2021

const logins = lodashGet(report, ['participants'], []);
// Report data can sometimes be incomplete. If we have no logins or reportID then we will skip this entry.
if (!report || !report.reportID || _.isEmpty(logins)) {
return;
}

Some #announce room doesn't have participants. So it get filtered out by this code.


If some other announces do have participants, then they would get filtered out by:
const hasDraftComment = hasReportDraftComment(report);
const iouReportOwner = lodashGet(report, 'hasOutstandingIOU', false)
? lodashGet(iouReports, [`${ONYXKEYS.COLLECTION.REPORT_IOUS}${report.iouReportID}`, 'ownerEmail'], '')
: '';
const reportContainsIOUDebt = iouReportOwner && iouReportOwner !== currentUserLogin;
const shouldFilterReportIfEmpty = !showReportsWithNoComments && report.lastMessageTimestamp === 0;
const shouldFilterReportIfRead = hideReadReports && report.unreadActionCount === 0;
const shouldShowReportIfHasDraft = showReportsWithDrafts && hasDraftComment;
const shouldFilterReport = shouldFilterReportIfEmpty || shouldFilterReportIfRead;
if (report.reportID !== activeReportID
&& !report.isPinned
&& !shouldShowReportIfHasDraft
&& shouldFilterReport
&& !reportContainsIOUDebt) {
return;
}

basically because announce room is created without any message. This code filters out those rooms that don't have any message.

@neil-marcellini
Copy link
Contributor

I would like to pick up this issue. It is still occurring. Would a solution be to send a default "welcome to the announce channel" message so that the room doesn't get filtered out due to having no messages?

@neil-marcellini neil-marcellini self-assigned this Jan 14, 2022
@K4tsuki
Copy link
Contributor

K4tsuki commented Jan 14, 2022

Yes maybe or how about don't filter default room:

function isDefaultRoom(report) {
return _.contains([
CONST.REPORT.CHAT_TYPE.POLICY_ADMINS,
CONST.REPORT.CHAT_TYPE.POLICY_ANNOUNCE,
CONST.REPORT.CHAT_TYPE.DOMAIN_ALL,
], lodashGet(report, ['chatType'], ''));
}

Add && !ReportUtils.isDefaultRoom in the if that filter announce room.
Or Make additonal parameter for getOption for mustDisplayDefaultRoom then add this condition. Just my opinion.

@neil-marcellini
Copy link
Contributor

That seems like a better idea. I'll start working on it. Thanks!

@neil-marcellini
Copy link
Contributor

After allowing default rooms to pass through the 0 participants filter I had rooms showing up with (deleted) in the title. I have now filtered out archived rooms so that these are hidden. Please let me know if you disagree with that approach.

Now when creating a new workspace the default rooms do not show up until the page is refreshed so the next step is to eliminate the need to refresh the page. Please take a look at my WIP PR for more context.

@K4tsuki
Copy link
Contributor

K4tsuki commented Jan 18, 2022

I think that is good.
But I am just external contributor, shouldn't we ask other internal engineer?

@neil-marcellini
Copy link
Contributor

Ok. I'll get some more feedback internally.

@neil-marcellini
Copy link
Contributor

After reading the design doc I found out that default rooms are supposed to stick around with (deleted) in the title after their workspace has been deleted. I got the default rooms to show up without needing a refresh by fetching the default chat reports. Thanks for pointing me in the right direction to get started @K4tsuki.

@neil-marcellini neil-marcellini added the Reviewing Has a PR in review label Jan 21, 2022
@K4tsuki
Copy link
Contributor

K4tsuki commented Jan 22, 2022

You're welcome @neil-marcellini and thank you for the additional information.

@botify botify closed this as completed Jan 24, 2022
@Santhosh-Sellavel
Copy link
Collaborator

@neil-marcellini Can you help set up a job for reporting bonus?

@Santhosh-Sellavel
Copy link
Collaborator

Also, @K4tsuki has guided us to the solution for this issue. So I think he qualifies for job payment.

cc: @neil-marcellini

@mallenexpensify
Copy link
Contributor

@neil-marcellini , did this issue get 'fixed'? If so we want to compensate @Santhosh-Sellavel for reporting it. Also, if so, is @K4tsuki eligible for compensation for contributing to the fix? ie. did you use any code from @K4tsuki ?

@mallenexpensify mallenexpensify self-assigned this Jan 29, 2022
@mallenexpensify mallenexpensify added Daily KSv2 and removed Monthly KSv2 labels Jan 29, 2022
@K4tsuki
Copy link
Contributor

K4tsuki commented Jan 29, 2022

Thank you @Santhosh-Sellavel.
@mallenexpensify there is no code from me. I am just helping out. I think I am not eligible for the rewards.

@neil-marcellini
Copy link
Contributor

Yes I believe this issue is fixed. It has been deployed to staging and will soon be deployed to production. I didn't use any code directly from @K4tsuki but he did point me in the right direction.

@mallenexpensify
Copy link
Contributor

Thanks @K4tsuki . @Santhosh-Sellavel , can you please apply here and confirm once you have? https://www.upwork.com/jobs/~017d6d330e04d78fbe

@mallenexpensify
Copy link
Contributor

Hired, @Santhosh-Sellavel , please let me know when you've accepted

@Santhosh-Sellavel
Copy link
Collaborator

Accepted, thanks! @mallenexpensify

@mallenexpensify
Copy link
Contributor

Paid, thanks @Santhosh-Sellavel

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Daily KSv2 Engineering Reviewing Has a PR in review
Projects
None yet
Development

No branches or pull requests

8 participants