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
| u.Organizations.Any(orgMember => myOrgIds.Contains(orgMember.OrgId)) || | ||
| u.Projects.Any(projMember => | ||
| myManagedProjectIds.Contains(projMember.ProjectId) || | ||
| (projMember.Project != null && projMember.Project.IsConfidential == false && myProjectIds.Contains(projMember.ProjectId)))); |
There was a problem hiding this comment.
I think we want IsConfidential != true because we decided to show users for unknown projects.
There was a problem hiding this comment.
Done in commit 44db0e3.
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. |
| manager.ShouldNotBeNull(); | ||
| var editor = await _lexBoxDbContext.Users.Include(u => u.Organizations).Include(u => u.Projects).FirstOrDefaultAsync(user => user.Email == "editor@test.com"); | ||
| editor.ShouldNotBeNull(); | ||
| await using var privateProject = await TempProjectWithoutRepo.Create(_lexBoxDbContext, true, manager.Id); |
There was a problem hiding this comment.
this test passes even if the project is not confidential, maybe the editor needs to be added to the project? So we probably want a couple tests, non manager members, seeing and not seeing members of projects based on if they're confidential or not, and then a test for a non member and they shouldn't see anything even for a non confidential project.
There was a problem hiding this comment.
Yeah, that's not actually testing what I intended it to test until the editor is in the project. So what we want is some tests to verify the following:
- Admin can see everyone (which is a little pointless since they can already just query
usersand the frontend takes that into account, so maybe this test isn't necessary? It would require a code change anyway) - Org members can see everyone in their org, and also anyone they share a public project with
- Project managers can see everyone in their projects, confidential or not, and also have the same rights as "normal" users in the projects they don't manage (can see public, can not see private)
- Non-managers can only see people they share public projects with (and org membership)
I think instead of using the users and projects from the seed data for this, I should have a test class constructor that sets up a specific set of projects and users, and a Dispose method on the test class that tears down all those projects and users. Then I can write the tests to the specific set of projects I created, without worrying about any changes I made to the seed data messing things up. (I once added some users to the sena-3 project and managed to make my tests start failing until I reset the data to the original seeded state. We don't want the tests to be that fragile).
There was a problem hiding this comment.
Commit af79417 tests more scenarios for projects, but doesn't actually include org membership in the test. So I'll need to add a few more users in a couple different orgs to really make these tests complete.
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? |
| @@ -27,6 +27,7 @@ | |||
| </Otherwise> | |||
| </Choose> | |||
| <ItemGroup> | |||
| <PackageReference Include="FluentAssertions" Version="6.12.0"/> | |||
There was a problem hiding this comment.
we're already using Shouldly in this project, we should just stick with one assertion package. Maybe we should discuss the 2 different packages we're using and settle on one.
There was a problem hiding this comment.
Fluent assertions gave the nice convenient Should().BeEquivalentTo method which worked on enumerables, whereas the Shouldly assertions didn't have that. However now that I have my helper method I can probably rewrite it to use Shouldly.
|
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.