SF-2472 Show Paratext users who have not joined the project

[RaymondLuong3]

Paratext users who have not joined the project can be displayed on the users page to represent the users that have access to the project. This involved adding a property to the paratext project that lists the members of the project. Additionally, some features of the users page have been updated based on the balsamiq.

Updated view

---

This change is 

[codecov]

Codecov Report

❌ Patch coverage is 97.00000% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.27%. Comparing base (de136f1) to head (a62cd24).
⚠️ Report is 1 commits behind head on master.
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
...app/users/collaborators/collaborators.component.ts	93.61%	0 Missing and 3 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3517      +/-   ##
==========================================
+ Coverage   82.25%   82.27%   +0.02%     
==========================================
  Files         615      615              
  Lines       37142    37175      +33     
  Branches     6071     6050      -21     
==========================================
+ Hits        30550    30586      +36     
- Misses       5688     5695       +7     
+ Partials      904      894      -10     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[pmachapman]

@pmachapman reviewed 12 of 13 files at r1, 3 of 3 files at r2, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @pmachapman and @RaymondLuong3)

---

src/SIL.XForge.Scripture/ClientApp/src/app/users/collaborators/collaborators.component.ts line 221 at r2 (raw file):

    } catch {
      this.noticeService.show(this.i18n.translateStatic('collaborators.problem_loading_paratext_users'));
    }

I am not sure we need to retrieve every project to get the details of just one - this makes the page really slow to load on my computer as I have a lot of projects.

I notice this component uses private projectDoc?: SFProjectProfileDoc; - if this was SFProject instead (revert the line this.projectDoc = projectDoc; to this.projectDoc = await this.projectService.get(projectId);), you could use the information from paratextUsers and calculate the new information just from the project model, unless there is a requirement I have missed?

Code quote:

      const paratextMembersNotOnProject =
        (await this.paratextService.getProjects())
          ?.find(p => p.paratextId === project.paratextId)
          ?.members.filter(m => !m.connectedToProject) ?? [];
      otherParatextMemberRows.push(
        ...paratextMembersNotOnProject.map(
          m => ({ id: '', role: m.role, user: { displayName: m.username }, paratextMemberNotConnected: true }) as Row
        )
      );
    } catch {
      this.noticeService.show(this.i18n.translateStatic('collaborators.problem_loading_paratext_users'));
    }

---

src/SIL.XForge.Scripture/Services/ParatextService.cs line 2431 at r2 (raw file):

                : users.Where(u =>
                    u.Sites.ContainsKey(siteId) && u.Sites[siteId].Projects.Contains(correspondingSfProject.Id)
                );

This seems to be really inefficient, and slows down the execution considerably by querying the users collection for every project.

Could you get the similar data by combining the userRoles and paratextUsers properties from sf_projects?

I also don't think we need this information in the get projects (see comment above).

Code quote:

            string siteId = _siteOptions.Value.Id;
            IEnumerable<User> usersOnProject = correspondingSfProject is null
                ? []
                : users.Where(u =>
                    u.Sites.ContainsKey(siteId) && u.Sites[siteId].Projects.Contains(correspondingSfProject.Id)
                );

[pmachapman]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @RaymondLuong3)

---

src/SIL.XForge.Scripture/ClientApp/src/app/users/collaborators/collaborators.component.ts line 221 at r2 (raw file):

Previously, pmachapman (Peter Chapman) wrote…

I am not sure we need to retrieve every project to get the details of just one - this makes the page really slow to load on my computer as I have a lot of projects.

I notice this component uses private projectDoc?: SFProjectProfileDoc; - if this was SFProject instead (revert the line this.projectDoc = projectDoc; to this.projectDoc = await this.projectService.get(projectId);), you could use the information from paratextUsers and calculate the new information just from the project model, unless there is a requirement I have missed?

Thinking on it a little further, we could populate paratextUsers with the role on sync, I think that's the only additional bit of data you need?

[RaymondLuong3]

Reviewable status: 3 of 22 files reviewed, 3 unresolved discussions (waiting on @pmachapman)

---

src/SIL.XForge.Scripture/ClientApp/src/app/users/collaborators/collaborators.component.ts line 221 at r2 (raw file):

Previously, pmachapman (Peter Chapman) wrote…

Thinking on it a little further, we could populate paratextUsers with the role on sync, I think that's the only additional bit of data you need?

Thanks for the suggestions. I think you are right, it does feel unnecessary to retrieve all the paratext projects just to populate the users from one project.
I like the suggestion to populate the ParatextUsers property on the project model. It does make sense to do that there. I've updated the code. The only downside is that the user must sync their project to see the list of paratext users and the list will not be up-to-date if something changes and the project needs to be synced.

---

src/SIL.XForge.Scripture/Services/ParatextService.cs line 2431 at r2 (raw file):

Previously, pmachapman (Peter Chapman) wrote…

This seems to be really inefficient, and slows down the execution considerably by querying the users collection for every project.

Could you get the similar data by combining the userRoles and paratextUsers properties from sf_projects?

I also don't think we need this information in the get projects (see comment above).

I agree. I've updated the code based on your suggestions.

[pmachapman]

@pmachapman reviewed 16 of 17 files at r3, 3 of 3 files at r4, all commit messages.
@pmachapman dismissed @github-advanced-security[bot] from a discussion.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @RaymondLuong3)

---

src/SIL.XForge.Scripture/ClientApp/src/app/users/collaborators/collaborators.component.ts line 221 at r2 (raw file):

Previously, RaymondLuong3 (Raymond Luong) wrote…

Thanks for the suggestions. I think you are right, it does feel unnecessary to retrieve all the paratext projects just to populate the users from one project.
I like the suggestion to populate the ParatextUsers property on the project model. It does make sense to do that there. I've updated the code. The only downside is that the user must sync their project to see the list of paratext users and the list will not be up-to-date if something changes and the project needs to be synced.

I have made a couple of comments suggesting a way we can have these users appear before the next sync.

---

src/SIL.XForge.Scripture/ClientApp/src/app/users/collaborators/collaborators.component.ts line 105 at r4 (raw file):

      { userType: UserType.Guest, rows: this._userRows?.filter(u => !isParatextRole(u.role)) ?? [] }
    ];
  }

If you make role nullable, you can change this to:

  get projectUsers(): ProjectUserLists[] {
    return [
      {
        userType: UserType.Paratext,
        rows: this._userRows?.filter(u => isParatextRole(u.role) || u.role == null) ?? []
      },
      { userType: UserType.Guest, rows: this._userRows?.filter(u => !isParatextRole(u.role) && u.role != null) ?? [] }
    ];
  }

which will show the unknown role users under Paratext Users

Code quote:

  get projectUsers(): ProjectUserLists[] {
    return [
      { userType: UserType.Paratext, rows: this._userRows?.filter(u => isParatextRole(u.role)) ?? [] },
      { userType: UserType.Guest, rows: this._userRows?.filter(u => !isParatextRole(u.role)) ?? [] }
    ];
  }

---

src/SIL.XForge.Scripture/ClientApp/src/app/users/collaborators/collaborators.component.ts line 254 at r4 (raw file):

    }

    for (const role of userRoles) {

If you make role nullable, you might also need to change this to something like:

for (const role of [undefined, null, ...userRoles]) {

which will show the unknown role users under Paratext Users.

Code quote:

for (const role of userRoles) {

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-sources/draft-sources.stories.ts line 121 at r4 (raw file):

    hasUserRoleChanged: false,
    hasUpdate: false,
    members: []

Should this be removed?

Code quote:

members: []

---

src/RealtimeServer/scriptureforge/models/paratext-user-profile.ts line 5 at r4 (raw file):

  opaqueUserId: string;
  sfUserId?: string;
  role: string;

This should be nullable for backwards compatibility with records created before this PR:

role?: string;

Code quote:

role: string;

---

src/RealtimeServer/scriptureforge/services/sf-project-service.ts line 561 at r4 (raw file):

        items: {
          bsonType: 'object',
          required: ['username', 'opaqueUserId', 'role'],

I think removed, as role should be nullable for backwards compatibility. Implementing this field as required will cause /scripts/db_tools/validate-data.ts to fail.

Code quote:

, 'role'

---

src/SIL.XForge.Scripture/Models/ParatextUserProfile.cs line 16 at r4 (raw file):

    public string OpaqueUserId { get; set; }
    public string? SFUserId { get; set; }
    public string Role { get; set; }

For backwards compatibility, this should be nullable, i.e.

public string? Role { get; set; }

Code quote:

public string Role { get; set; }

---

src/SIL.XForge.Scripture/Services/ParatextSyncRunner.cs line 1904 at r4 (raw file):

                        }

                        string paratextRole = ptUserInRegistry?.Role ?? string.Empty;

I think role should be nullable when it can't be determined (see other comments).

Code quote:

string paratextRole = ptUserInRegistry?.Role ?? string.Empty;

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-sources/draft-sources.component.spec.ts line 765 at r4 (raw file):

          hasUserRoleChanged: false,
          hasUpdate: false,
          members: []

Should this be removed?

Code quote:

members: []

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-sources/draft-sources.component.spec.ts line 792 at r4 (raw file):

        hasUserRoleChanged: false,
        hasUpdate: false,
        members: []

Should this be removed?

Code quote:

members: []

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-sources/draft-sources.component.spec.ts line 814 at r4 (raw file):

        hasUserRoleChanged: false,
        hasUpdate: false,
        members: []

Should this be removed?

Code quote:

members: []

[RaymondLuong3]

Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @pmachapman)

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-sources/draft-sources.stories.ts line 121 at r4 (raw file):

Previously, pmachapman (Peter Chapman) wrote…

Should this be removed?

Done.

---

src/SIL.XForge.Scripture/ClientApp/src/app/users/collaborators/collaborators.component.ts line 221 at r2 (raw file):

Previously, pmachapman (Peter Chapman) wrote…

I have made a couple of comments suggesting a way we can have these users appear before the next sync.

That is a good idea. I think that is the right way to go so that the change takes affect immediately.

---

src/SIL.XForge.Scripture/ClientApp/src/app/users/collaborators/collaborators.component.ts line 105 at r4 (raw file):

Previously, pmachapman (Peter Chapman) wrote…

If you make role nullable, you can change this to:

  get projectUsers(): ProjectUserLists[] {
    return [
      {
        userType: UserType.Paratext,
        rows: this._userRows?.filter(u => isParatextRole(u.role) || u.role == null) ?? []
      },
      { userType: UserType.Guest, rows: this._userRows?.filter(u => !isParatextRole(u.role) && u.role != null) ?? [] }
    ];
  }

which will show the unknown role users under Paratext Users

I made a small adjustment to your suggestion. Since paratext users either have a paratext role or they have the propery paratextUserNotConnected set to true, I can identify paratext users by their role or that property without having to depend on the role being null.

---

src/SIL.XForge.Scripture/ClientApp/src/app/users/collaborators/collaborators.component.ts line 254 at r4 (raw file):

Previously, pmachapman (Peter Chapman) wrote…

If you make role nullable, you might also need to change this to something like:

for (const role of [undefined, null, ...userRoles]) {

which will show the unknown role users under Paratext Users.

Done.

---

src/SIL.XForge.Scripture/Models/ParatextUserProfile.cs line 16 at r4 (raw file):

Previously, pmachapman (Peter Chapman) wrote…

For backwards compatibility, this should be nullable, i.e.

public string? Role { get; set; }

Done.

---

src/SIL.XForge.Scripture/Services/ParatextSyncRunner.cs line 1904 at r4 (raw file):

Previously, pmachapman (Peter Chapman) wrote…

I think role should be nullable when it can't be determined (see other comments).

Yes, I think that is a better way to represent when a user no longer has a role on the project.

[pmachapman]

This works brilliantly - thank you!

@pmachapman reviewed 11 of 11 files at r5, all commit messages.
@pmachapman dismissed @github-advanced-security[bot] from a discussion.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @RaymondLuong3)

[Nateowami]

@RaymondLuong3 Can you reconcile merge conflicts before this PR is tested?