sillsdev / languageforge-lexbox

Lexbox, SIL linguistic data hub
MIT License
7 stars 2 forks source link

User typeahead enabled for non-admin project managers #1237

Open rmunn opened 1 week ago

rmunn commented 1 week ago

Fix #1152.

This PR adds a new usersICanSee GQL query, which looks for:

The "Add Project Member" typeahead is now updated to use that query, which allows project managers who aren't site admins to use the typeahead to find users whose email address they don't know. E.g. if Test Manager has Test Editor as part of project A, and he also manages project B, then when he clicks on "Add Members" in project B, he can type Test Editor's name and select him to add, without knowing his email address.

github-actions[bot] commented 1 week ago

UI unit Tests

12 tests  ±0   12 :white_check_mark: ±0   0s :stopwatch: ±0s  4 suites ±0    0 :zzz: ±0   1 files   ±0    0 :x: ±0 

Results for commit 8930e88c. ± Comparison against base commit c23a0bc9.

:recycle: This comment has been updated with latest results.

rmunn commented 1 week ago

I've done local testing and the typeahead is working. I want to add some unit tests before calling this PR done, to make sure that the usersICanSee query is returning exactly the results it should. But the code is complete and working, so it's probably ready to review. The only changes I expect to make are:

github-actions[bot] commented 1 week ago

C# Unit Tests

98 tests  +8   98 :white_check_mark: +8   5s :stopwatch: ±0s 15 suites +1    0 :zzz: ±0   1 files   ±0    0 :x: ±0 

Results for commit 8930e88c. ± Comparison against base commit c23a0bc9.

:recycle: This comment has been updated with latest results.

rmunn commented 1 week ago

VS Code told me that IEmailService was unused, so I removed it. Turns out it was used after all, just not in that file so the code analysis didn't spot it. Commit 2d0a7c56 should turn the unit test checks green again.

rmunn commented 3 days ago

Commit e037ab97 tries to say "we don't care about the order, just that the user list matches". But this is still flaky, because WithoutStrictOrdering is only applying to the top level of the expected array. It's still enforcing strict ordering lower down, with the result that sometimes a test passes, and other times that same test fails because user Marian was expected to be in projects [Sherwood, Nottingham] but was in projects [Nottingham, Sherwood] instead. Even though we don't actually care about the projects list of the returned user object; this test only cares that the users returned include Maid Marian but not the Sheriff of Nottingham.

Instead of passing increasingly-complex test options, I'll write a new helper function to extract only the names from the expected list and just compare two lists of strings. (Which will also help with output when a test fails: right now it's spitting out dozens of properties, including the nested properties of the projects in a user's membership list and so on, resulting in test output that's truly hard to read.)

rmunn commented 3 days ago

Commit 3ac20114 adds a single failing test: a user who's a member of one private project but no orgs. You'd expect usersICanSee to return yourself, but currently it doesn't.

Right now that doesn't matter because the frontend filters out existing project members (including the member who can't see himself in the query), so he wouldn't be able to see himself in the frontend typeahead results even if the query was returning the right thing. But we probably do want the usersICanSee query to include yourself no matter what, otherwise it's not living up to its name. So I wrote a failing test for that edge case: Alan a Dale can't see himself in the usersICanSee query because he's not an org. (Little John could see himself because he could see the membership of the Outlaws organization, which includes him. Which is why Alan a Dale was not included in the Outlaws org, in order to exercise this edge case).

rmunn commented 3 days ago

@hahn-kev @myieye - I've rewritten the UserServiceTests pretty extensively, to set up a set of projects, orgs, and users used only for those tests. Would you let me know if the tests are clear and easy to understand, or if there's anything unclear about them? In particular, I'm hoping that the project and org memberships are pretty obvious when you're reading the tests, without having to scroll up to look at the InitializeAsync method to remind yourself of who is a member of which project. Did I achieve that goal?

myieye commented 3 days ago

LGTM 👍 So, I'll step away from this PR and let @hahn-kev approve when he's happy with the package choice.