serval-admin: serval-builds: show requester email in build details

[marksvc]

This patch modifies the Requesting user area in the expanded
information of a Serval build so it shows the user's email address.

I have further plans for this "Requesting user" card after this change.

Screenshot:

---

This change is 

[Copilot]

Pull request overview

Updates the Serval Administration “Serval Builds” expanded build details to display the requesting user’s email address by fetching full user docs (instead of profile docs) and expanding server-side read permissions accordingly.

Changes:

• Client: Replace profile-based requester lookup with user-doc-based lookup and add email display (with mailto + copy) in the build details UI.
• Server: Allow SystemRole.ServalAdmin to read other users’ user docs.
• Tests/styles: Add tests for requester display name/email lookups and adjust component styling for links.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/serval-builds.component.ts	Fetch requester identity (display name + email) from UserDoc and cache results.
src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/serval-builds.component.html	Render requester email address with mailto link and copy control in expanded details.
src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/serval-builds.component.scss	Add anchor styling affecting link decoration across the component.
src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/serval-builds.component.spec.ts	Add tests for requester display name/email and update mocks for user lookups.
src/RealtimeServer/common/services/user-service.ts	Expand allowRead to include SystemRole.ServalAdmin.
src/RealtimeServer/common/services/user-service.spec.ts	Add coverage ensuring Serval admins can read other users’ docs.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[marksvc]

It works fine when running the tests.

[marksvc]

Our link underlines are rare. Every link on this page is without underline.

[marksvc]

This is something I have / am considering. There is a trade-off between tightening up this fetch and muddying our models. At present I lean to let Serval admins see the doc.

[codecov]

Codecov Report

❌ Patch coverage is 83.33333% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.05%. Comparing base (6f65a0e) to head (09004ca).
⚠️ Report is 1 commits behind head on master.
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
...p/serval-administration/serval-builds.component.ts	78.57%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3860      +/-   ##
==========================================
+ Coverage   81.03%   81.05%   +0.02%     
==========================================
  Files         630      630              
  Lines       40683    40690       +7     
  Branches     6601     6579      -22     
==========================================
+ Hits        32968    32983      +15     
- Misses       6681     6686       +5     
+ Partials     1034     1021      -13     

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

[Nateowami]

@Nateowami made 1 comment.
Reviewable status: 0 of 6 files reviewed, 4 unresolved discussions (waiting on marksvc).

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/serval-builds.component.ts line 410 at r2 (raw file):
Devin states:

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/serval-builds.component.ts:R410

merge called with array argument instead of spread arguments, preventing reactive updates

merge([userDoc.changes$, userDoc.remoteChanges$]) passes an array as a single argument to merge. In rxjs 7, merge uses rest parameters, so this is interpreted as subscribing to a single ObservableInput (the array itself). Since arrays are iterable, rxjs converts [obs1, obs2] via from(...), which emits the two Observable objects as values and then completes. It does not subscribe to changes$ or remoteChanges$. The stream completes immediately after the startWith(undefined) and two Observable-object emissions, making the cached identity observable unable to reflect real-time user doc changes. Every other usage of merge() in the codebase passes observables as separate arguments (e.g. checking.component.ts:675, sync-progress.component.ts:136).

I wasn't quite sure whether this was correct, so I looked at the definition of merge (via Ctrl+Click), and we have this:

export function merge(...args: (ObservableInput<unknown> | number | SchedulerLike)[]): Observable<unknown> {
  const scheduler = popScheduler(args);
  const concurrent = popNumber(args, Infinity);
  const sources = args as ObservableInput<unknown>[];
  return !sources.length
    ? // No source provided
      EMPTY
    : sources.length === 1
    ? // One source? Just return it.
      innerFrom(sources[0])
    : // Merge all sources
      mergeAll(concurrent)(from(sources, scheduler));
}

I'm not quite sure how passing an array impacts the ... parameters, so I wanted to test it by trimming the function down to just this:

export function merge(...args: (ObservableInput<unknown> | number | SchedulerLike)[]): Observable<unknown> {
  console.log(args)
}

Of course that won't "compile", but we can fix it be defining meaningless types:

type ObservableInput<T> = any;
type SchedulerLike = any;
type Observable<T> = any;

export function merge(...args: (ObservableInput<unknown> | number | SchedulerLike)[]): Observable<unknown> {
  console.log(args)
}

Now paste that into a Deno REPL to define the function, and then run this:

> merge(['a', 'b'])
[ [ "a", "b" ] ]

Looks like it gets an array with a single element, the array that is passed in. I would say Devin is right.

Or at least the situation is so confusing that we should probably be consistent all the time.

[marksvc]

@marksvc made 1 comment.
Reviewable status: 0 of 6 files reviewed, 4 unresolved discussions (waiting on Nateowami).

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/serval-builds.component.ts line 410 at r2 (raw file):

Previously, Nateowami wrote…

Devin states:

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/serval-builds.component.ts:R410

merge called with array argument instead of spread arguments, preventing reactive updates

merge([userDoc.changes$, userDoc.remoteChanges$]) passes an array as a single argument to merge. In rxjs 7, merge uses rest parameters, so this is interpreted as subscribing to a single ObservableInput (the array itself). Since arrays are iterable, rxjs converts [obs1, obs2] via from(...), which emits the two Observable objects as values and then completes. It does not subscribe to changes$ or remoteChanges$. The stream completes immediately after the startWith(undefined) and two Observable-object emissions, making the cached identity observable unable to reflect real-time user doc changes. Every other usage of merge() in the codebase passes observables as separate arguments (e.g. checking.component.ts:675, sync-progress.component.ts:136).

I wasn't quite sure whether this was correct, so I looked at the definition of merge (via Ctrl+Click), and we have this:

export function merge(...args: (ObservableInput<unknown> | number | SchedulerLike)[]): Observable<unknown> {
  const scheduler = popScheduler(args);
  const concurrent = popNumber(args, Infinity);
  const sources = args as ObservableInput<unknown>[];
  return !sources.length
    ? // No source provided
      EMPTY
    : sources.length === 1
    ? // One source? Just return it.
      innerFrom(sources[0])
    : // Merge all sources
      mergeAll(concurrent)(from(sources, scheduler));
}

I'm not quite sure how passing an array impacts the ... parameters, so I wanted to test it by trimming the function down to just this:

export function merge(...args: (ObservableInput<unknown> | number | SchedulerLike)[]): Observable<unknown> {
  console.log(args)
}

Of course that won't "compile", but we can fix it be defining meaningless types:

type ObservableInput<T> = any;
type SchedulerLike = any;
type Observable<T> = any;

export function merge(...args: (ObservableInput<unknown> | number | SchedulerLike)[]): Observable<unknown> {
  console.log(args)
}

Now paste that into a Deno REPL to define the function, and then run this:

> merge(['a', 'b'])
[ [ "a", "b" ] ]

Looks like it gets an array with a single element, the array that is passed in. I would say Devin is right.

Or at least the situation is so confusing that we should probably be consistent all the time.

Thank you for the catch and additional information. I added tests that show that the current code is not working as intended and fixed the call to merge.

[marksvc]

@Nateowami Nudge, when you have a chance to take a look.

[marksvc]

Resolved conflicts.

[Nateowami]

@Nateowami reviewed 2 files and made 1 comment.
Reviewable status: 2 of 6 files reviewed, 4 unresolved discussions (waiting on marksvc).

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/serval-builds.component.ts line 410 at r2 (raw file):

Previously, marksvc wrote…

Thank you for the catch and additional information. I added tests that show that the current code is not working as intended and fixed the call to merge.

userDoc.changes$ includes all changes, including remote changes. Not having to enumerate a list of different types of changes to listen for is its entire purpose for existing.

[Nateowami]

@Nateowami reviewed 4 files and all commit messages, made 1 comment, and resolved 1 discussion.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on marksvc).

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/serval-builds.component.ts line 428 at r4 (raw file):

  ): Observable<{ displayName: string; emailAddress?: string }> {
    if (requesterSFUserId == null) {
      return of({ displayName: 'Unknown' });

Slightly odd that email is nullable (and template decides what to do when null), but displayName is required (and component picks a string)

[marksvc]

@marksvc made 2 comments.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on Nateowami).

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/serval-builds.component.ts line 410 at r2 (raw file):

Previously, Nateowami wrote…

userDoc.changes$ includes all changes, including remote changes. Not having to enumerate a list of different types of changes to listen for is its entire purpose for existing.

Hmm. I just confirmed that in sharedb-realtime-remote-store.ts and memory-realtime-remote-store.ts. I seem to recall that recently we had a bug where some code was listening to one and not the other and we needed to change the code to listen to both observables, and that you mentioned that we should make an observable that fires on any changes.

Hmm, that may have had something to do with usage of realtime-query.ts. It has localChanges$, remoteChanges$, remoteDocChanges$, and docs$. I don't remember.

I removed remoteChanges$. Interesting that now there's no merge() :). It's good that we caught the merge() usage problem earlier and added it to our checks.

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/serval-builds.component.ts line 428 at r4 (raw file):

Previously, Nateowami wrote…

Slightly odd that email is nullable (and template decides what to do when null), but displayName is required (and component picks a string)

Hmm. Yes. I decided to have the component return undefined for display name, and the template can decide what to do in that situation. Display name and email address should now be alike in that respect.
Changed.

[Nateowami]

@Nateowami reviewed 3 files and all commit messages, made 1 comment, and resolved 2 discussions.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on marksvc).