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 75.00000% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.07%. Comparing base (1feb822) to head (d2d2ee5).
✅ All tests successful. No failed tests found.

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

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3860      +/-   ##
==========================================
+ Coverage   81.04%   81.07%   +0.02%     
==========================================
  Files         630      630              
  Lines       40592    40601       +9     
  Branches     6588     6587       -1     
==========================================
+ Hits        32898    32916      +18     
+ Misses       6661     6653       -8     
+ Partials     1033     1032       -1     

☔ 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.