Skip to content

Suggestions search was removing the incorrect user from search results - #19013

Merged
twstokes merged 1 commit into
wordpress-mobile:trunkfrom
salimbraksa:fix/prominent-suggestions
Jul 8, 2022
Merged

Suggestions search was removing the incorrect user from search results#19013
twstokes merged 1 commit into
wordpress-mobile:trunkfrom
salimbraksa:fix/prominent-suggestions

Conversation

@salimbraksa

@salimbraksa salimbraksa commented Jul 8, 2022

Copy link
Copy Markdown
Contributor

Description

This PR fixes a bug from another PR #18979 that was merged recently.

Problem

In the previous PR, there was a code that removes multiple indexes from an array. Like this:

// `suggestionIndexesToRemove` is an array of integers.
suggestionIndexesToRemove.forEach { searchResults.remove(at: $0) }

This implementation is wrong. The following example illustrates the problem:

Let's assume that suggestionIndexesToRemove array is [1, 3].

When the element at index 1 is removed, the element at index 3 is left-shifted to index 2. Then the element at index 3 gets removed, and that's not correct.

Solution

Indexes should be removed from the end of the list to the beginning. In the previous example, the element at index 3 should be removed before the element at index 1.
We can achieve this by sorting suggestionIndexesToRemove.forEach in descending order:

suggestionIndexesToRemove = suggestionIndexesToRemove.sorted(by: >)

We will open an upcoming PR to cover this case and other cases as well.

@twstokes

twstokes commented Jul 8, 2022

Copy link
Copy Markdown
Contributor

PR for running CI.

@twstokes
twstokes enabled auto-merge July 8, 2022 15:20
@peril-wordpress-mobile

Copy link
Copy Markdown
Warnings
⚠️ PR is not assigned to a milestone.

Generated by 🚫 dangerJS

@twstokes
twstokes merged commit ae5a066 into wordpress-mobile:trunk Jul 8, 2022
@twstokes twstokes added this to the 20.3 milestone Jul 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants