Issue 19058 - Sort user-mentions suggestions by prefix first, then alphabetically by igoriols · Pull Request #19218 · wordpress-mobile/WordPress-iOS

igoriols · 2022-08-22T00:49:33Z

Closes #19058

Description

In this PR, I'm addressing the sorting change requested. I'm displaying first the users whose names begin with a prefix provided by the user, then alphabetically. The first criteria are to sort alphabetically by users whose usernames or display names begin with the provided prefix. If both username and display name begin with the prefix, the display name will be prioritized in the sorting.

Details

I’m using localizedCaseInsensitiveCompare to compare displayNames, because the user can have the displayName in different languages. The comparison is localizable and case-insensitive.
I’m using < to compare usernames because they are very restricted, and the lexicographical comparison should work for them.
I'm looping through the UserSuggestions array to create two sub-arrays. The first one has the users whose display names begin with a prefix provided by the user. The second one has the users whose display names don't begin with the prefix provided by the user.
I'm sorting both arrays alphabetically, then I combine the sorted arrays.
I updated the tests to conform with the new sorting criteria.

Preview

Testing Instructions

N.B: The user-mentions suggestions feature is not enabled in self-hosted sites ( wordpress.org ).

Tap the "My Site" tab.
Tap on "Menu"
Tap on "Comments" row.
Tap any comment.
Tap "Reply" button.
Type "@" and you should see the user-mentions suggestions list.
Type a prefix and check if the list is sorted first by users whose usernames or display names begin with the prefix, then alphabetically.

There are other scenarios to display the user-mentions suggestions list described in #18979.

Regression Notes

Potential unintended areas of impact
N/A
What I did to test those areas of impact (or what existing automated tests I relied on)
N/A
What automated tests I added (or what prevented me from doing so)
I updated the existing tests related to the user-mentions suggestions list.

PR submission checklist:

I have completed the Regression Notes.
I have considered adding unit tests for my changes.
I have considered adding accessibility improvements for my changes.
I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

…nsitive

…s sorted correctly

hassaanelgarem

I think we can do two things differently:

Doing a case insensitive search instead of case-sensitive
Look for the prefix in both the username and the display name

@igoriols What do you think?

hassaanelgarem · 2022-08-23T00:23:14Z

WordPress/Classes/Models/UserSuggestion+Comparable.swift

@@ -0,0 +1,7 @@
+extension UserSuggestion: Comparable {
+    public static func < (lus: UserSuggestion, rus: UserSuggestion) -> Bool {


[Nit]

Suggested change

public static func < (lus: UserSuggestion, rus: UserSuggestion) -> Bool {

public static func < (lhs: UserSuggestion, rhs: UserSuggestion) -> Bool {

I'm curious, what do lus and ldn stand for?

lus: left user suggestion
rus: right user suggestion
ldn: left display name
rdn: right display name
I thought it would be better than the standard lhs (left-hand side) and rhs (right-hand side), but I can change to use lhs and rhs. I can apply more meaningful names to the unwrapped display names, also.

hassaanelgarem · 2022-08-23T00:26:26Z

WordPress/Classes/ViewRelated/SuggestionsListViewModel.swift

+        var otherUserSuggestions = [UserSuggestion]()
+
+        userSuggestions.forEach { suggestion in
+            guard let displayName = suggestion.displayName else { return }


This is a corner case, but if the user doesn't have a display name for some reason, they will be filtered out completely. I think we should add them to otherUserSuggestions in that case. Wdyt?

Yes, that is a good idea. I'm going to apply it.

hassaanelgarem · 2022-08-23T00:27:06Z

WordPress/Classes/ViewRelated/SuggestionsListViewModel.swift

        }
    }

+    private static func sort(userSuggestions: [UserSuggestion], by searchQuery: String) -> [UserSuggestion] {


Can you add a brief documentation of what's happening here? 🙏

Sure. It's done. Thank you for reminding me.

…on method

…and add documentation to the method sort

igoriols · 2022-08-23T17:23:41Z

I think we can do two things differently:
* Doing a case insensitive search instead of case-sensitive

* Look for the prefix in both the username and the display name
@igoriols What do you think?

Hi @hassaanelgarem. Just to make sure I understand.

About case insensitive search, do you mean remove the [c] from here?
If the user has the prefix in username and displayName, should we sort by displayName first?
I'm asking because, in my opinion, the displayName is more relevant.

hassaanelgarem · 2022-08-23T23:37:12Z

About case insensitive search, do you mean remove the [c] from here?

@igoriols No, I mean changing the hasPrefix() check here to be case insensitive.

WordPress/WordPressShareExtension/String+Extensions.swift

AliSoftware · 2022-08-24T11:26:06Z

WordPress/Classes/ViewRelated/SuggestionsListViewModel.swift

+            if let displayName = suggestion.displayName,
+               (username.beginsWith(prefix: prefix) || displayName.beginsWith(prefix: prefix)) {
+                prefixedUserSuggestions.append(suggestion)


This will skip users with a nil displayName… even if they have a username that begins with the prefix.

So {"ID":102,"user_login":"carlosdiaz","display_name":null} will not be found even if prefix is carl here, because let displayName = suggestion.displayName will be false and exit the condition already.

Is that intentional? Do we want to prioritize only the users who have a non-nil display name?

If not, this condition will need to be adjusted accordingly 🙂 (and it would be nice to use the occasion to add a test case for this while we're at it 😉 )

Nice catch. The idea was to have users with a username or a displayName beginning with the prefix first. I think your dictionary approach suggestion will cover that. I will add a new entry to test this scenario, also.

WordPress/Classes/ViewRelated/SuggestionsListViewModel.swift

…th null displayName

…-suggestions

WordPress/Classes/Models/UserSuggestion+Comparable.swift

…calling hasPrefix with options

hassaanelgarem

Works as expected 🚀

AliSoftware

Overall good, nice job!

Left a couple of nitpick suggestions (up to you to apply or not), and also a suggestion for additional test cases. But other than that this LGTM 👍

WordPress/Classes/ViewRelated/SuggestionsListViewModel.swift

WordPress/WordPressShareExtension/String+Extensions.swift

WordPress/WordPressTest/Mention/SuggestionsListViewModelTests.swift

AliSoftware · 2022-08-31T12:19:16Z

WordPress/WordPressTest/Mention/SuggestionsListViewModelTests.swift

@@ -119,7 +119,7 @@ class SuggestionsListViewModelTests: CoreDataTestCase {
    func testSearchSuggestionsWithPartialMatch() throws {
        // Given
        let word = "@ca"


💡 I notice that you don't have tests that contains diacritics or uppercase letters as prefix. Which means we don't cover the cases of things like @Ca, or ensure that e.g. @Caa would match user with display name "Caárla Diaz".

Could be nice to add some coverage for that, especially given you were thoughtful in the implementation to implement case-insensitive and diacritics-insensitive comparison for display Name (but not for user name), so would be nice to test for it to ensure there won't be an accidental regression for that aspect in the future.

Nice catch. I added more tests to cover more scenarios. Thank you!

…ic-insensitive

WordPress/WordPressTest/Mention/SuggestionsListViewModelTests.swift

AliSoftware · 2022-08-31T23:31:33Z

Just thought of some edge cases (though that one should probably be for a separate PR rather than this one): how does this whole thing behaves with RTL and non-Latin languages?

I'd assume that thanks to the localizedCaseInsensitiveCompare the comparison will be locale-aware, so we should be good on that front with using the sort order according to the user's locale 👍
But is that also true for our hasPrefix, especially for RTL text? Or using Latin transcription prefix of simplified Chinese characters? 🤔 Not sure that .caseInsensitive, .diacriticsInsensitive covers locale insensitive or awareness 🤔
And how does both the sorting and the filtering fares when the list of suggested users contains a mix of users with Latin displayName and other users with eg Japanese or Chinese characters in their display name? I think it should turn out OK (because Foundation is smart enough to deal with those on our behalf in the localizedCaseInsensitiveCompare and range implementations, but would be nice to confirm 😛
And is the logic of "The user types @ then C then a in the textField and expect the filtering to be by prefix Ca (and not aC or \u{200F}Ca) (U200F being the RTL marker in Unicode, though I doubt it would be present in the String, since the Bidi algorithm would take care of switching the text direction between @ and the text as needed automatically… but worth checking)
If the user types @Cá with the acute accent explicitly typed, should we list suggestions actually having the accent first, then the ones matching in diacritics-insensitive way next, then the rest? This would probably be taking it a bit too far and not worth the effort tho 😅

To be clear, all of those cases/open questions are (1) likely out of scope for this PR (2) might not all be worth actually fixing or even investigating, even in a separate PR — as I'm probably overthinking some of those cases. Still, I just thought I'd braindump those here to raise the question and see if it's worth checking/ investigating those cases or not. 🙃

AliSoftware

Apart from the small 💄 typo in the test comments, this looks ready to merge to me now. Nice work 👍

hassaanelgarem · 2022-09-04T22:54:06Z

@igoriols I updated the milestone to 20.8 since we won't catch the code freeze. Please update the release note accordingly 🙏

mokagio · 2022-10-02T20:38:36Z

@igoriols just wanted to let you know that WordPress 20.8, the first version with your changes from this PR, just started rolling out to users.

Thank you so much for your contribution! 🎉

igoriols changed the title ~~Issue 19058 - Sort user-mentions suggestions by prefix first then alphabetically~~ Issue 19058 - Sort user-mentions suggestions by prefix first, then alphabetically Aug 22, 2022

igoriols added 2 commits August 21, 2022 21:58

Change: sort user suggestions by prefix

6b402b1

Add: release note

3f8c2ce

igoriols force-pushed the feature/19058-sort-users-suggestions-by-prefix branch from 516c39d to 3f8c2ce Compare August 22, 2022 01:00

igoriols added 5 commits August 21, 2022 22:28

Change: improve user-mention sorting using locale-aware and case inse…

975d1ea

…nsitive

Fix: prevent sorting if userSuggestions is empty

7e997d0

Test: add a new entry with a diacritic mark in user-suggestions.json

f361ed7

Refactor: simplify sort user-mentions suggestions

0a90739

Test: add lower case entry to user-suggestions.json and check if it i…

3d23393

…s sorted correctly

hassaanelgarem self-requested a review August 22, 2022 23:07

hassaanelgarem self-assigned this Aug 22, 2022

hassaanelgarem added [Type] Enhancement Mentions labels Aug 22, 2022

hassaanelgarem added this to the 20.7 milestone Aug 22, 2022

hassaanelgarem reviewed Aug 23, 2022

View reviewed changes

igoriols added 2 commits August 23, 2022 12:57

Fix: rename parameters and variable names of user suggestion comparis…

5321edc

…on method

Change: add user suggestions without displayName in the sorted array …

b43a626

…and add documentation to the method sort

Change: sort by username or displayName which begins with the prefix

ec3604f

AliSoftware reviewed Aug 24, 2022

View reviewed changes

igoriols added 2 commits August 24, 2022 20:57

Change: simplify the user-suggestion sorting

f0114e3

Test: update SuggestionListViewModelTests with a new testing entry wi…

f17aff1

…th null displayName

igoriols requested a review from AliSoftware August 25, 2022 00:02

igoriols added 2 commits August 24, 2022 21:19

Fix: user-suggestions sorting method

6033b3c

Change: convert beginsWith to hasPrefix and call it when sorting user…

1972e48

…-suggestions

hassaanelgarem reviewed Aug 25, 2022

View reviewed changes

WordPress/Classes/Models/UserSuggestion+Comparable.swift Show resolved Hide resolved

igoriols requested review from hassaanelgarem and removed request for AliSoftware August 26, 2022 03:11

Merge branch 'trunk' into feature/19058-sort-users-suggestions-by-prefix

3620c78

igoriols requested review from AliSoftware and hassaanelgarem and removed request for AliSoftware and hassaanelgarem August 26, 2022 04:18

igoriols added 3 commits August 29, 2022 07:01

Change: ensure the .anchored option is added to the options set when …

7ffec5a

…calling hasPrefix with options

Merge branch 'trunk' into feature/19058-sort-users-suggestions-by-prefix

2a07763

Fix: adjust release notes

11813e6

hassaanelgarem mentioned this pull request Aug 31, 2022

Temporary PR to run CI on forked branch #19257

Merged

hassaanelgarem approved these changes Aug 31, 2022

View reviewed changes

igoriols requested a review from AliSoftware August 31, 2022 06:00

AliSoftware reviewed Aug 31, 2022

View reviewed changes

igoriols added 4 commits August 31, 2022 17:43

Fix: line break

2c02645

Docs: update documentation of hasPrefix

ad74720

Change: move compareOptions to the sort method

9a8f58c

Change: change NSPredicate to filter for case-insensitive and diacrit…

419eceb

…ic-insensitive

AliSoftware reviewed Aug 31, 2022

View reviewed changes

WordPress/WordPressTest/Mention/SuggestionsListViewModelTests.swift Outdated Show resolved Hide resolved

AliSoftware approved these changes Aug 31, 2022

View reviewed changes

Fix: typos on test descriptions

2150d75

hassaanelgarem modified the milestones: 20.7, 20.8 Sep 4, 2022

igoriols added 2 commits September 5, 2022 11:05

Merge branch 'trunk' into feature/19058-sort-users-suggestions-by-prefix

5a7823b

Fix: add a release note for the 20.8 milestone

89647e0

hassaanelgarem merged commit 0a65c53 into wordpress-mobile:trunk Sep 6, 2022

		@@ -0,0 +1,7 @@
		extension UserSuggestion: Comparable {
		public static func < (lus: UserSuggestion, rus: UserSuggestion) -> Bool {

	public static func < (lus: UserSuggestion, rus: UserSuggestion) -> Bool {
	public static func < (lhs: UserSuggestion, rhs: UserSuggestion) -> Bool {

Conversation

igoriols commented Aug 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Details

Preview

Testing Instructions

Regression Notes

Uh oh!

hassaanelgarem left a comment

Choose a reason for hiding this comment

Uh oh!

hassaanelgarem Aug 23, 2022

Choose a reason for hiding this comment

Uh oh!

hassaanelgarem Aug 23, 2022

Choose a reason for hiding this comment

Uh oh!

igoriols Aug 23, 2022

Choose a reason for hiding this comment

Uh oh!

hassaanelgarem Aug 23, 2022

Choose a reason for hiding this comment

Uh oh!

igoriols Aug 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hassaanelgarem Aug 23, 2022

Choose a reason for hiding this comment

Uh oh!

igoriols Aug 23, 2022

Choose a reason for hiding this comment

Uh oh!

igoriols commented Aug 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hassaanelgarem commented Aug 23, 2022

Uh oh!

Uh oh!

AliSoftware Aug 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

igoriols Aug 24, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hassaanelgarem left a comment

Choose a reason for hiding this comment

Uh oh!

AliSoftware left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AliSoftware Aug 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

igoriols Aug 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

AliSoftware commented Aug 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AliSoftware left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hassaanelgarem commented Sep 4, 2022

Uh oh!

mokagio commented Oct 2, 2022

Uh oh!

igoriols commented Aug 22, 2022 •

edited

Loading

igoriols Aug 23, 2022 •

edited

Loading

igoriols commented Aug 23, 2022 •

edited

Loading

AliSoftware Aug 24, 2022 •

edited

Loading

AliSoftware Aug 31, 2022 •

edited

Loading

igoriols Aug 31, 2022 •

edited

Loading

AliSoftware commented Aug 31, 2022 •

edited

Loading

AliSoftware left a comment •

edited

Loading