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 2022-01-25] Rooms are duplicated in results list when searching the room by name - @Kidroca #5699

Closed
isagoico opened this issue Oct 6, 2021 · 38 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Engineering External Added to denote the issue can be worked on by a contributor Weekly KSv2

Comments

@isagoico
Copy link

isagoico commented Oct 6, 2021

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. In a account that has at least 1 workspace
  2. Open search
  3. Search for #announce room
  4. Delete a character and write it again

Expected Result:

After triggering the search, there's no duplicates in the results list.

Actual Result:

After triggering the search several times there's a bunch of duplicates in the results list.

Workaround:

None needed, visual issue.

Platform:

Where is this issue occurring?

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

Version Number: 1.1.6-0

Reproducible in staging?: yes
Reproducible in production?: yes

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

Notes/Photos/Videos: Any additional supporting documentation

image

Grabando.219.mp4

Expensify/Expensify Issue URL:
Upwork Job: https://www.upwork.com/jobs/~018be34f339157da58

Issue reported by: @kidroca
Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1633507668430400

View all open jobs on GitHub

@MelvinBot
Copy link

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

@parasharrajat
Copy link
Member

@kidroca can you check if you have multiple announce named reports in localhost. In my case, I have multiple reports with announce name.

@timszot timszot added the External Added to denote the issue can be worked on by a contributor label Oct 8, 2021
@timszot timszot removed their assignment Oct 8, 2021
@MelvinBot
Copy link

Triggered auto assignment to @kevinksullivan (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

@MelvinBot MelvinBot added Overdue and removed Overdue labels Oct 8, 2021
@MelvinBot
Copy link

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

@kevinksullivan kevinksullivan added Weekly KSv2 and removed Daily KSv2 labels Oct 13, 2021
@MelvinBot MelvinBot removed the Overdue label Oct 13, 2021
@kevinksullivan
Copy link
Contributor

I am also seeing duplicates and can reproduce the issue.

image

@kevinksullivan
Copy link
Contributor

@kevinksullivan kevinksullivan removed their assignment Oct 16, 2021
@MelvinBot MelvinBot added Monthly KSv2 and removed Weekly KSv2 labels Nov 8, 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!

@kevinksullivan kevinksullivan added Weekly KSv2 and removed Monthly KSv2 labels Nov 15, 2021
@parasharrajat
Copy link
Member

It seems to be lost. I think I know why this is happening. so let's fix it.

Proposal

  1. We have to divide the existing
    const sections = [{
    into two.
    like
   const sections = [{
            title: this.props.translate('common.recents'),
            data: this.state.recentReports,
            shouldShow: true,
            indexOffset: 0,
        },
        {
            title: this.props.translate('common.contacts'),
            data: this.state.personalDetails,
            shouldShow: true,
            indexOffset: this.state.recentReports.length,
        }];

It fixes it.

@kevinksullivan Could you please reapply the exported label?

@Jag96
Copy link
Contributor

Jag96 commented Dec 16, 2021

As an update, there's a discussion happening in #6627 (comment), the fix for that should fix this as well

@K4tsuki
Copy link
Contributor

K4tsuki commented Dec 20, 2021

proposal

Simple explanation:
So basically the cause is defaultRoom with one participant will create wrong personalDetail option. This must be filtered when detecting personal detail option, using if conditional check from logins.length <= 1 to logins.length <= 1 && !ReportUtils.isDefaultRoom(report) on this line of code:

if (logins.length <= 1) {

This solve the issue for me. More test must be done, but the solution is along the line.


Detailed explanation:
Cause is we are treated default room with only one participant: (default room is here)

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'], ''));

with only one participant as personal details room.

with only one participant, the getOptions function will make option for personal details and include it in allPersonalDetailsOptions.
Detection is here, this detect if room is have only one participant and then include it in reportMapForLogins:

if (logins.length <= 1) {

And then make personalDetailsOptions in here using that reportMapForLogins map:

const allPersonalDetailsOptions = _.map(personalDetails, personalDetail => (
createOption([personalDetail], reportMapForLogins[personalDetail.login], {
showChatPreviewLine,
forcePolicyNamePreview,
})
));

reportMapForLogins then will contain map with key login email and value the default report report with one participant.

When creating personalDetail option on createOption, pair of reporMapAbove on that pair, createOption will detect option as hasMultipleParticipants because result of ReportUtils.isDefaultRoom(report); and assigning login value to null.

login: !hasMultipleParticipants ? personalDetail.login : null,

And key value list of the option is same as original (default reportIID): because there is report (when calling createOption)pair in there:

keyForList: report ? String(report.reportID) : personalDetail.login,

So there will be double option with same keyForList.

The option must be excluded or filtered later, but:

Because login value is null, the room is cannot be excluded in list of excluded option for personal details. This code generate loginOptionsToExclude:

if (reportOption.login) {

Because the login value is null, it would not be included.

That excluded list is executed here to exclude personal details option that has room:

if (reportOption.login && _.some(loginOptionsToExclude, option => option.login === reportOption.login)) {

The option keep exist with same keyForList of original default report option.

The solution is explained in simple explanation above.

@parasharrajat
Copy link
Member

@K4tsuki As you are a new contributor, you are not allowed to work on multiple issues simultaneously so we can't accept this proposal. Also, there is a discussion already happening above which is yet to be concluded. Thanks for your patience.

@K4tsuki
Copy link
Contributor

K4tsuki commented Dec 20, 2021

Why? I cannot see any guidelines about not allowed to work simultaneously. And there is Help Wanted and Exported label. Please review my proposal @Jag96. Thank you.
@mallenexpensify @puneetlath am I not allowed to work simultaneously. I have been working hard on some issues.

@parasharrajat
Copy link
Member

parasharrajat commented Dec 20, 2021

@K4tsuki Please read the contribution guidelines carefully New contributors are limited to working on one job at a time, however, experienced contributors may work on numerous jobs simultaneously.

This rule is the same for all of the contributors and everyone including me has to abide by it.

I have been working hard on some issues.

Were you assigned to those issues? Please don't work on any issues until you are assigned to work on any.

@K4tsuki
Copy link
Contributor

K4tsuki commented Dec 20, 2021

This rule is the same for all of the contributors and everyone including me has to abide by it.

So after my first PR I can apply for many issues?

Were you assigned to those issues? Please don't work on any issues until you are assigned to work on any.

Making proposal.

@parasharrajat
Copy link
Member

So after my first PR I can apply for many issues?

Initially, one at a time and after some time as much as you can. Usually, after 1-2 PR we will hire you on multiple and you will not even have to mention this.

@Jag96
Copy link
Contributor

Jag96 commented Dec 20, 2021

Since @parasharrajat is already working on fixing this in #6627 (comment), removing the Help Wanted label, sorry for the confusion!

@Jag96 Jag96 removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Dec 20, 2021
@NicMendonca
Copy link
Contributor

@Jag96 should I be closing this job in Upwork?

@MelvinBot MelvinBot removed the Overdue label Jan 3, 2022
@Jag96
Copy link
Contributor

Jag96 commented Jan 3, 2022

@NicMendonca I think we should hire @parasharrajat for this one since technically this isn't a dupe.

@NicMendonca
Copy link
Contributor

@parasharrajat sent you the job in Upwork, thank you!

@Jag96
Copy link
Contributor

Jag96 commented Jan 10, 2022

PR still in the works here: #6971

@parasharrajat
Copy link
Member

Just trying to get screenshots of Mac...

@botify botify added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Jan 18, 2022
@botify
Copy link

botify commented Jan 18, 2022

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.1.30-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 2022-01-25. 🎊

@botify botify changed the title Rooms are duplicated in results list when searching the room by name - @Kidroca [HOLD for payment 2022-01-25] Rooms are duplicated in results list when searching the room by name - @Kidroca Jan 18, 2022
@NicMendonca
Copy link
Contributor

NicMendonca commented Jan 25, 2022

@kidroca did you log this issue reporting in your logs? Or are we issuing "regular" payment for this one? Thanks!

@kidroca
Copy link
Contributor

kidroca commented Jan 25, 2022

Regular - I don't log the time for bug/feature reports

@NicMendonca
Copy link
Contributor

NicMendonca commented Jan 25, 2022

Thanks for clarifying! When you get the chance, can you please accept the job that I sent over, or apply here? I'll issue payment after that.

@parasharrajat issued payment for you! thank you!

@kidroca
Copy link
Contributor

kidroca commented Jan 26, 2022

@NicMendonca
Thanks, I've accepted

@NicMendonca
Copy link
Contributor

Perfect, all set here. Thanks everyone!

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 Engineering External Added to denote the issue can be worked on by a contributor Weekly KSv2
Projects
None yet
Development

No branches or pull requests