User typeahead enabled for non-admin project managers #1237
Conversation
|
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
|
There was a problem hiding this comment.
looks good, one minor change. For testing I would actually suggest moving the query code into UsersService and then calling it directly, that way you are not doing an e2e gql test, just a simple db query test, take a look at the ProjectServiceTest as an example which interacts with the database
Now the test can be a proper unit test rather than an integration test.
This avoids having to use `try ... finally` blocks in the tests, making them more reliable.
|
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 2d0a7c5 should turn the unit test checks green again. |
There was a problem hiding this comment.
I agree with Kevin's request for the tests to be improved a bit, but otherwise this looks good to me 👍.
I pushed a few minor improvements:
- There were still a few things that wanted a rename
- Adapted the GenerateGqlSchema task/method so it keeps working
- Started hiding typeahead results that are duplicates of existing members (not quite specific to this PR, but relevant)
|
Commit e037ab9 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 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.) |
|
Commit 3ac2011 adds a single failing test: a user who's a member of one private project but no orgs. You'd expect 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 |
A member of private projects should be able to see himself, but currently he cannot. He could if he was in an org, but he cannot see himself if he's in one private project but no orgs.
rmunn
force-pushed
the
feat/typeahead-for-non-admins
branch
from
096d825 to
3ac2011
Compare
|
@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? |
|
LGTM 👍 |
Fix #1152.
This PR adds a new
usersICanSeeGQL 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.