SF-2574 Provide project data to NMT forward drafting approval team

[pmachapman]

This PR:

• Adds a Serval Administration Panel where users with the role ServalAdmin can activate pre-translation drafting for projects.
• Adds the ability to download Paratext projects as a zip file in the Serval Administration Area.

Developer testing notes
If you want to add the ability to view this panel, open your user in Auth0, and change xf_role in app meta data to be:

"xf_role": "serval_admin"

Or:

"xf_role": [
    "serval_admin"
  ]

---

This change is 

[codecov]

Codecov Report

Attention: Patch coverage is 88.75740% with 19 lines in your changes are missing coverage. Please review.

Project coverage is 76.66%. Comparing base (29b4ff6) to head (64b97d8).
Report is 23 commits behind head on master.

❗ Current head 64b97d8 differs from pull request most recent head 691ed7f. Consider uploading reports for the commit 691ed7f to get more accurate results

Files	Patch %	Lines
.../serval-administration/serval-project.component.ts	65.85%	14 Missing ⚠️
...serval-administration/serval-projects.component.ts	92.98%	2 Missing and 2 partials ⚠️
...al-administration/serval-administration.service.ts	85.71%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2373      +/-   ##
==========================================
- Coverage   76.83%   76.66%   -0.17%     
==========================================
  Files         508      510       +2     
  Lines       28942    28859      -83     
  Branches     4708     4706       -2     
==========================================
- Hits        22237    22126     -111     
- Misses       5966     5990      +24     
- Partials      739      743       +4     

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

[nigel-wells]

Reviewed 28 of 34 files at r1, 6 of 6 files at r2, all commit messages.
Reviewable status: all files reviewed, 52 unresolved discussions (waiting on @pmachapman)

a discussion (no related file):
Nice work on this Peter - this will provide a great way for admins to get insight into Serval. Question: Is Serval Admin > System Admin?

There will need to be some offline notices added to the serval admin project page if someone was on that page when they went offline i.e. downloading is not available.

---

---

src/SIL.XForge.Scripture/ClientApp/src/app/app.component.spec.ts line 355 at r2 (raw file):

      env.init();

      // Show the user menu

For simplicity across all the tests, how about we create a env.showUserMenu() and env.closeUserMenu() - will save some repeating lines across all the tests.

---

src/SIL.XForge.Scripture/ClientApp/src/app/app.component.spec.ts line 360 at r2 (raw file):

      // Verify the menu item is visible
      expect(env.component.isServalAdmin).toBeTruthy();

nit change these to toBeTrue() as toBeTruthy() can apply to lots of variables which we wouldn't want to pass if it did.

---

src/SIL.XForge.Scripture/ClientApp/src/app/app.component.spec.ts line 362 at r2 (raw file):

      expect(env.component.isServalAdmin).toBeTruthy();
      expect(env.userMenu).not.toBeNull();
      expect(env.userMenu.query(By.css('#serval-admin-btn'))).not.toBeNull();

Make a getter for these css queries to keep the logic in the TestEnvironment which will make future changes easier if needed

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/serval-admin-auth.guard.spec.ts line 26 at r2 (raw file):

    env.service.canActivate({} as ActivatedRouteSnapshot, {} as RouterStateSnapshot).subscribe(result => {
      expect(result).toBeTruthy();

nit make these toBeTrue() as well

---

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

<div fxLayout="column" fxLayoutAlign="center start" class="body-content">

Is this <div> adding any value? I think it can just be removed as it doesn't need to make use of flex in anyway

---

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

   * @returns An observable.
   */
  downloadProject(projectId: string): Observable<Blob> {

Are these methods serval specific or would the be better in the ProjectService? They sound generic at first glance.

---

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

   */
  downloadProject(projectId: string): Observable<Blob> {
    return this.httpClient.get(`paratext-api/projects/${projectId}/download`, {

Perhaps we could add paratext-api to our list of consts in url-constants.ts for consistency

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/serval-administration.service.spec.ts line 28 at r2 (raw file):

      const env = new TestEnvironment();
      const id = '1234567890abcdef';
      expect(env.service.isResource(id)).toBeTruthy();

Change to toBeTrue() and below

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/serval-administration.service.spec.ts line 31 at r2 (raw file):

    });

    it('should return true for a project id', () => {

should return "false"

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/serval-project.component.html line 1 at r2 (raw file):

<div fxLayout="column" fxLayoutAlign="center start" class="body-content">

Is this <div> needed?

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/serval-project.component.html line 2 at r2 (raw file):

<div fxLayout="column" fxLayoutAlign="center start" class="body-content">
  <h1>{{ projectName }}</h1>

I wonder if this isn't needed. The route has already switched to the project and the name can be seen in the nav bar so its obvious which one we're on. Maybe this should be "Serval Settings" or something like that? Is it worth having this in the main nav menu as well otherwise you're always forced to go to the serval admin page and then back to the project you were already on.

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/serval-project.component.html line 3 at r2 (raw file):

<div fxLayout="column" fxLayoutAlign="center start" class="body-content">
  <h1>{{ projectName }}</h1>
  <div>

Is this <div> needed?

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/serval-project.component.html line 4 at r2 (raw file):

  <h1>{{ projectName }}</h1>
  <div>
    <a mat-raised-button color="primary" [appRouterLink]="'/serval-administration'">Back to Projects</a>

Not translated

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/serval-project.component.html line 6 at r2 (raw file):

    <a mat-raised-button color="primary" [appRouterLink]="'/serval-administration'">Back to Projects</a>
  </div>
  <h2>Pre-Translation Configuration</h2>

Not translated

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/serval-project.component.html line 7 at r2 (raw file):

  </div>
  <h2>Pre-Translation Configuration</h2>
  <div>

Is this <div> needed?

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/serval-project.component.html line 9 at r2 (raw file):

  <div>
    <mat-checkbox [checked]="preTranslate" (change)="onUpdatePreTranslate($event.checked)">
      Pre-Translation Drafting Enabled

Not translated

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/serval-project.component.html line 12 at r2 (raw file):

    </mat-checkbox>
  </div>
  <h2>Downloads</h2>

Not translated

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/serval-project.component.html line 14 at r2 (raw file):

  <h2>Downloads</h2>
  <app-notice icon="info">
    The Zip archives contain the Paratext files for the project at the time of last sync.

Not translated.
Also, is it easy to add the date/time of the last sync to save the user having to figure it out?

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/serval-project.component.html line 30 at r2 (raw file):

        <td mat-cell *matCellDef="let row">
          <button mat-raised-button color="primary" (click)="downloadProject(row['id'], row['fileName'])">
            Download

Not translated

---

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

  projectName = '';

  headingsToDisplay = { category: 'Category', name: 'Project', id: '' };

Not translated

---

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

            id: projectDoc.id,
            name: this.projectName,
            category: 'Target Project',

Not translated

---

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

              id: project.translateConfig.source.projectRef,
              name: project.translateConfig.source.shortName + ' - ' + project.translateConfig.source.name,
              category: 'Source Project',

Not translated

---

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

                ' - ' +
                project.translateConfig.draftConfig.alternateSource.name,
              category: 'Alternate Source',

Not translated

---

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

                ' - ' +
                project.translateConfig.draftConfig.alternateTrainingSource.name,
              category: 'Alternate Training Source',

Not translated

---

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

    // Trigger a click that downloads the blob
    // NOTE: This code should not be unit tested, as it will trigger downloads in the test browser
    const url = window.URL.createObjectURL(blob);

Why are we creating a blob in the client to download the file? I would have expected the server to respond with the appropriate header information to trigger a file download.

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/serval-projects.component.html line 8 at r2 (raw file):

</div>
<ng-container *ngIf="!isLoading">
  <ng-container *ngIf="length > 0">

Can this be combined with the *ngIf above

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/serval-projects.component.html line 11 at r2 (raw file):

    <table mat-table id="projects-table" ngClass.gt-xs="fixed-layout-table" [dataSource]="rows">
      <ng-container matColumnDef="name">
        <th mat-header-cell *matHeaderCellDef>Name</th>

Not translated

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/serval-projects.component.html line 17 at r2 (raw file):

      </ng-container>
      <ng-container matColumnDef="preTranslate">
        <th mat-header-cell *matHeaderCellDef>Pre-Translation</th>

Not translated

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/serval-projects.component.html line 19 at r2 (raw file):

        <th mat-header-cell *matHeaderCellDef>Pre-Translation</th>
        <td mat-cell *matCellDef="let row">
          <strong *ngIf="row.preTranslate">Enabled</strong>

Not translated

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/serval-projects.component.html line 20 at r2 (raw file):

        <td mat-cell *matCellDef="let row">
          <strong *ngIf="row.preTranslate">Enabled</strong>
          <span *ngIf="!row.preTranslate">Disabled</span>

Not translated

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/serval-projects.component.html line 24 at r2 (raw file):

      </ng-container>
      <ng-container matColumnDef="source">
        <th mat-header-cell *matHeaderCellDef>Source</th>

Not translated

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/serval-projects.component.html line 33 at r2 (raw file):

      </ng-container>
      <ng-container matColumnDef="alternateSource">
        <th mat-header-cell *matHeaderCellDef>Alternate Source</th>

Not translated

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/serval-projects.component.html line 44 at r2 (raw file):

      </ng-container>
      <ng-container matColumnDef="alternateTrainingSource">
        <th mat-header-cell *matHeaderCellDef>Alternate Training Source</th>

Not translated

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/serval-projects.component.html line 58 at r2 (raw file):

    </table>

    <mat-paginator

There is an overlay issue here:

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/serval-projects.component.html line 67 at r2 (raw file):

    </mat-paginator>
  </ng-container>
  <div *ngIf="length === 0" class="no-projects-label">No projects found</div>

Not translated

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/serval-projects.component.scss line 2 at r2 (raw file):

.projects-controls {
  margin: 32px 0;

Probably not needed

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/serval-projects.component.scss line 14 at r2 (raw file):

.fixed-layout-table {
  table-layout: fixed;

Why was this needed?

---

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

  get alternateSource(): string {
    return this.projectDoc.data?.translateConfig.draftConfig.alternateSource == null
      ? 'None'

Not translated

---

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

  get alternateTrainingSource(): string {
    return this.projectDoc.data?.translateConfig.draftConfig.alternateTrainingSource == null
      ? 'None'

Not translated

---

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

  get name(): string {
    return this.projectDoc.data == null ? 'N/A' : this.projectDoc.data.shortName + ' - ' + this.projectDoc.data.name;

"N/A" Not translated

---

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

  get source(): string {
    return this.projectDoc.data?.translateConfig.source == null
      ? 'None'

Not translated

---

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

})
export class ServalProjectsComponent extends DataLoadingComponent implements OnInit {
  @HostBinding('class') classes = 'flex-column';

Is this needed?

---

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

        this.length = searchResults.unpagedCount;
        this.generateRows();
        this.loadingFinished();

This doesn't appear to resolve the loader when searching or using pagination

---

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

    const termTarget = target as HTMLInputElement;
    if (termTarget?.value != null) {
      this.searchTerm$.next(termTarget.value);

Is an online query needed for this when all the data has already been loaded into the table?

---

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

  }

  private getQueryParameters(): QueryParameters {

Something funny here. I have 7 projects available when per page is set to 10 and it shows "1-7 of 7". If I change items to 5 then it shows "1-5 of 5" and I can't change pages.

---

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

    env.setInputValue(env.filterInput, '02');

    expect(env.rows.length).toEqual(1);

Shouldn't this equal 2 for "P2 - Project 02" and "R2 - Resource 02"?

---

src/SIL.XForge.Scripture/Controllers/ParatextController.cs line 89 at r2 (raw file):

        // Return the zip file stream
        return File(outputStream, "application/zip", fileName);

As per my earlier comment, can this simply provide the correct headers for the browser to trigger a download rather than relying on the client to convert to a blob and do something with it?

---

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

        if (!fileSystemService.DirectoryExists(path))
        {
            throw new DataNotFoundException($"The following directory could not be found: {path}");

We're not giving away too much information here but outputting the path are we i.e. paratext ID?

---

test/SIL.XForge.Scripture.Tests/Services/SFProjectServiceTests.cs line 3057 at r2 (raw file):

            () => env.Service.SetPreTranslateAsync(User03, [SystemRole.SystemAdmin], Project01, false)
        );
        // SUT 3

nit SUT 4

[pmachapman]

Dismissed @Github-advanced-security[bot] from 2 discussions.
Reviewable status: all files reviewed, 43 unresolved discussions (waiting on @nigel-wells)

a discussion (no related file):

Is Serval Admin > System Admin?

No - they are different roles. You can be one, the other, or both.

There will need to be some offline notices added to the serval admin project page if someone was on that page when they went offline i.e. downloading is not available.

Do we need it given this is an special admin only area, and not accessible to the general public? The Serval Admin menu item is disabled when offline, and the red network message appears when I go offline, or there is a connection interruption.

This change is an admin panel - no users will access it, except specific staff of SIL or developers. For this reason, it will not be translated, like how the System Admin page is not translated. (I pasted the message "This page/component will not be translated as it is not for general users." into each of the comments to do with this, as I had to keep track of ones I had not yet relied too - it got a bit confusing :-S)

---

---

src/SIL.XForge.Scripture/ClientApp/src/app/app.component.spec.ts line 355 at r2 (raw file):

Previously, nigel-wells (Nigel Wells) wrote…

For simplicity across all the tests, how about we create a env.showUserMenu() and env.closeUserMenu() - will save some repeating lines across all the tests.

Done. I have created a method called showHideUserMenu() as the logic is precisely the same to show and hide the menu.

---

src/SIL.XForge.Scripture/ClientApp/src/app/app.component.spec.ts line 360 at r2 (raw file):

Previously, nigel-wells (Nigel Wells) wrote…

nit change these to toBeTrue() as toBeTruthy() can apply to lots of variables which we wouldn't want to pass if it did.

Done. I have changed these toBe(true), as toBe(false) was used elsewhere. Also toBeTrue() caused me an error in my intellisense for some reason...

---

src/SIL.XForge.Scripture/ClientApp/src/app/app.component.spec.ts line 362 at r2 (raw file):

Previously, nigel-wells (Nigel Wells) wrote…

Make a getter for these css queries to keep the logic in the TestEnvironment which will make future changes easier if needed

Done.

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/serval-admin-auth.guard.spec.ts line 26 at r2 (raw file):

Previously, nigel-wells (Nigel Wells) wrote…

nit make these toBeTrue() as well

Done.

---

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

Previously, nigel-wells (Nigel Wells) wrote…

Is this <div> adding any value? I think it can just be removed as it doesn't need to make use of flex in anyway

Done. Removed.

---

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

Previously, nigel-wells (Nigel Wells) wrote…

Are these methods serval specific or would the be better in the ProjectService? They sound generic at first glance.

These methods should not be accessible by the general public, so I moved them into this service, to stop project.service from getting cluttered. There will be more methods coming here in future PRs.

---

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

Previously, nigel-wells (Nigel Wells) wrote…

Perhaps we could add paratext-api to our list of consts in url-constants.ts for consistency

Done.

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/serval-administration.service.spec.ts line 28 at r2 (raw file):

Previously, nigel-wells (Nigel Wells) wrote…

Change to toBeTrue() and below

Done.

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/serval-administration.service.spec.ts line 31 at r2 (raw file):

Previously, nigel-wells (Nigel Wells) wrote…

should return "false"

Done.

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/serval-project.component.html line 1 at r2 (raw file):

Previously, nigel-wells (Nigel Wells) wrote…

Is this <div> needed?

Done. Removed.

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/serval-project.component.html line 2 at r2 (raw file):

Previously, nigel-wells (Nigel Wells) wrote…

I wonder if this isn't needed. The route has already switched to the project and the name can be seen in the nav bar so its obvious which one we're on. Maybe this should be "Serval Settings" or something like that? Is it worth having this in the main nav menu as well otherwise you're always forced to go to the serval admin page and then back to the project you were already on.

This is here because the project won't appear in the main nav if you do not have access to this project. That will be the case for most projects opened using this page - the Serval Admin will not have access to the project, and so will configure it here (there will be future PRs adding functionality to do these Serval configurations actions)

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/serval-project.component.html line 3 at r2 (raw file):

Previously, nigel-wells (Nigel Wells) wrote…

Is this <div> needed?

Done. Removed.

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/serval-project.component.html line 4 at r2 (raw file):

Previously, nigel-wells (Nigel Wells) wrote…

Not translated

This page will not be translated as it is not for general users.

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/serval-project.component.html line 6 at r2 (raw file):

Previously, nigel-wells (Nigel Wells) wrote…

Not translated

This page will not be translated as it is not for general users.

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/serval-project.component.html line 7 at r2 (raw file):

Previously, nigel-wells (Nigel Wells) wrote…

Is this <div> needed?

Done. Removed.

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/serval-project.component.html line 9 at r2 (raw file):

Previously, nigel-wells (Nigel Wells) wrote…

Not translated

This page will not be translated as it is not for general users.

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/serval-project.component.html line 12 at r2 (raw file):

Previously, nigel-wells (Nigel Wells) wrote…

Not translated

This page will not be translated as it is not for general users.

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/serval-project.component.html line 14 at r2 (raw file):

Previously, nigel-wells (Nigel Wells) wrote…

Not translated.
Also, is it easy to add the date/time of the last sync to save the user having to figure it out?

It would be a separate timestamp for each project (i.e. each download link), and the last sync timestamp includes the last failed sync - which may not reflect the time the files were actually synced to disk.

I could add it, but I don't think it is really necessary as the last updated timestamp will be visible in the files when downloaded and extracted from the zip file?

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/serval-project.component.html line 30 at r2 (raw file):

Previously, nigel-wells (Nigel Wells) wrote…

Not translated

This page will not be translated as it is not for general users.

---

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

Previously, nigel-wells (Nigel Wells) wrote…

Not translated

This component will not be translated as it is not for general users.

---

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

Previously, nigel-wells (Nigel Wells) wrote…

Not translated

This component will not be translated as it is not for general users.

---

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

Previously, nigel-wells (Nigel Wells) wrote…

Not translated

This component will not be translated as it is not for general users.

---

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

Previously, nigel-wells (Nigel Wells) wrote…

Not translated

This component will not be translated as it is not for general users.

---

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

Previously, nigel-wells (Nigel Wells) wrote…

Not translated

This component will not be translated as it is not for general users.

---

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

Previously, nigel-wells (Nigel Wells) wrote…

Why are we creating a blob in the client to download the file? I would have expected the server to respond with the appropriate header information to trigger a file download.

The HttpInterceptor we use could not be used with a traditional download link, as download actions have to be triggered by the user clicking on an <a> element and that element directly downloading the file. I would presume this is to stop websites throwing random downloads at you without you wanting them.

For this reason, the code gets the download as a blob, then presents it to the user an in anchor tag which is clicked in the background.

I could have gotten around it by passing the token in the URL in an anchor tag, but I wasn't really keen on that for security reasons.

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/serval-projects.component.html line 8 at r2 (raw file):

Previously, nigel-wells (Nigel Wells) wrote…

Can this be combined with the *ngIf above

If I do that, I would have to duplicate the inverse of the logic in the "No projects found" div at the bottom. Nesting I think is simpler?

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/serval-projects.component.html line 11 at r2 (raw file):

Previously, nigel-wells (Nigel Wells) wrote…

Not translated

This component will not be translated as it is not for general users.

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/serval-projects.component.html line 17 at r2 (raw file):

Previously, nigel-wells (Nigel Wells) wrote…

Not translated

This component will not be translated as it is not for general users.

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/serval-projects.component.html line 19 at r2 (raw file):

Previously, nigel-wells (Nigel Wells) wrote…

Not translated

This component will not be translated as it is not for general users.

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/serval-projects.component.html line 20 at r2 (raw file):

Previously, nigel-wells (Nigel Wells) wrote…

Not translated

This component will not be translated as it is not for general users.

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/serval-projects.component.html line 24 at r2 (raw file):

Previously, nigel-wells (Nigel Wells) wrote…

Not translated

This component will not be translated as it is not for general users.

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/serval-projects.component.html line 33 at r2 (raw file):

Previously, nigel-wells (Nigel Wells) wrote…

Not translated

This component will not be translated as it is not for general users.

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/serval-projects.component.html line 44 at r2 (raw file):

Previously, nigel-wells (Nigel Wells) wrote…

Not translated

This component will not be translated as it is not for general users.

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/serval-projects.component.html line 58 at r2 (raw file):

Previously, nigel-wells (Nigel Wells) wrote…

There is an overlay issue here:

I have no idea what is causing that. It is with the Paginator component and affects it where ever it is used, based on my testing. I will file a separate bug for the paginator issues.

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/serval-projects.component.html line 67 at r2 (raw file):

Previously, nigel-wells (Nigel Wells) wrote…

Not translated

This component will not be translated as it is not for general users.

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/serval-projects.component.scss line 2 at r2 (raw file):

Previously, nigel-wells (Nigel Wells) wrote…

Probably not needed

Done. Removed.

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/serval-projects.component.scss line 14 at r2 (raw file):

Previously, nigel-wells (Nigel Wells) wrote…

Why was this needed?

Done. I don't remember why. Removed.

---

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

Previously, nigel-wells (Nigel Wells) wrote…

Not translated

This component will not be translated as it is not for general users.

---

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

Previously, nigel-wells (Nigel Wells) wrote…

Not translated

This component will not be translated as it is not for general users.

---

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

Previously, nigel-wells (Nigel Wells) wrote…

"N/A" Not translated

This component will not be translated as it is not for general users.

---

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

Previously, nigel-wells (Nigel Wells) wrote…

Not translated

This component will not be translated as it is not for general users.

---

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

Previously, nigel-wells (Nigel Wells) wrote…

Is this needed?

Done. Removed.

---

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

Previously, nigel-wells (Nigel Wells) wrote…

This doesn't appear to resolve the loader when searching or using pagination

Done. I think the events were happening way too close together?

---

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

Previously, nigel-wells (Nigel Wells) wrote…

Is an online query needed for this when all the data has already been loaded into the table?

Pagination values are passed to the query, so not all data will be on the client.

---

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

Previously, nigel-wells (Nigel Wells) wrote…

Something funny here. I have 7 projects available when per page is set to 10 and it shows "1-7 of 7". If I change items to 5 then it shows "1-5 of 5" and I can't change pages.

I have recreated this bug in other places the mat-paginator is used. I will look at this as a separate issue with the other paginator bugs.

---

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

Previously, nigel-wells (Nigel Wells) wrote…

Shouldn't this equal 2 for "P2 - Project 02" and "R2 - Resource 02"?

No - only projects are shown on this page, not resources.

---

src/SIL.XForge.Scripture/Controllers/ParatextController.cs line 89 at r2 (raw file):

Previously, nigel-wells (Nigel Wells) wrote…

As per my earlier comment, can this simply provide the correct headers for the browser to trigger a download rather than relying on the client to convert to a blob and do something with it?

Discussed in the previous comment.

---

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

Previously, nigel-wells (Nigel Wells) wrote…

We're not giving away too much information here but outputting the path are we i.e. paratext ID?

Done. I have sanitized it to just be the Paratext Id.

---

test/SIL.XForge.Scripture.Tests/Services/SFProjectServiceTests.cs line 3057 at r2 (raw file):

Previously, nigel-wells (Nigel Wells) wrote…

nit SUT 4

Done.

[nigel-wells]

Reviewed 3 of 15 files at r3, 12 of 12 files at r5, 1 of 1 files at r6, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @pmachapman)

a discussion (no related file):

Previously, pmachapman (Peter Chapman) wrote…

Is Serval Admin > System Admin?

No - they are different roles. You can be one, the other, or both.

There will need to be some offline notices added to the serval admin project page if someone was on that page when they went offline i.e. downloading is not available.

Do we need it given this is an special admin only area, and not accessible to the general public? The Serval Admin menu item is disabled when offline, and the red network message appears when I go offline, or there is a connection interruption.

This change is an admin panel - no users will access it, except specific staff of SIL or developers. For this reason, it will not be translated, like how the System Admin page is not translated. (I pasted the message "This page/component will not be translated as it is not for general users." into each of the comments to do with this, as I had to keep track of ones I had not yet relied too - it got a bit confusing :-S)

If we're confident that the users on the screen are savvy enough to know things may break if they're offline then I'm happy to not worry about it. You'll need to add a note to the testers to not worry about testing the components in offline mode.

---

---

src/SIL.XForge.Scripture/ClientApp/src/app/app.component.spec.ts line 360 at r2 (raw file):

Previously, pmachapman (Peter Chapman) wrote…

Done. I have changed these toBe(true), as toBe(false) was used elsewhere. Also toBeTrue() caused me an error in my intellisense for some reason...

Yes, good call. I forgot toBeTrue() also does an optional check for a boolean object which we wouldn't want to be true for these. Good discussion here around the three options : https://stackoverflow.com/questions/32615713/tobetrue-vs-tobetruthy-vs-tobetrue

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/serval-project.component.html line 2 at r2 (raw file):

Previously, pmachapman (Peter Chapman) wrote…

This is here because the project won't appear in the main nav if you do not have access to this project. That will be the case for most projects opened using this page - the Serval Admin will not have access to the project, and so will configure it here (there will be future PRs adding functionality to do these Serval configurations actions)

Ah, ok, that makes sense. What will the main nav show for projects you do not have access to?

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/serval-project.component.html line 14 at r2 (raw file):

Previously, pmachapman (Peter Chapman) wrote…

It would be a separate timestamp for each project (i.e. each download link), and the last sync timestamp includes the last failed sync - which may not reflect the time the files were actually synced to disk.

I could add it, but I don't think it is really necessary as the last updated timestamp will be visible in the files when downloaded and extracted from the zip file?

All good - only do if there is value in knowing what that date/time is without having to look i.e. would seeing the date/time on the screen change/influence someone's decision around downloading the documents.

---

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

Previously, pmachapman (Peter Chapman) wrote…

The HttpInterceptor we use could not be used with a traditional download link, as download actions have to be triggered by the user clicking on an <a> element and that element directly downloading the file. I would presume this is to stop websites throwing random downloads at you without you wanting them.

For this reason, the code gets the download as a blob, then presents it to the user an in anchor tag which is clicked in the background.

I could have gotten around it by passing the token in the URL in an anchor tag, but I wasn't really keen on that for security reasons.

Yes, a user needs to click on the link but my question is around why the backend doesn't then serve up the appropriate headers for downloading a file. The backend is already fetching the file to be downloaded but it is returned in a request to the client to process rather than headers to trigger the browser to download it. Should be a lot less code if the backend does it. Something along the lines of:

httpResponseMessage.Content.Headers.ContentDisposition = new System.Net.Http.Headers.ContentDispositionHeaderValue("attachment");
httpResponseMessage.Content.Headers.ContentDisposition.FileName = FileName;
httpResponseMessage.Content.Headers.ContentType = new System.Net.Http.Headers.MediaTypeHeaderValue("application/octet-stream");

It may be that our requests to the backend are configured to allow for a response like that?

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/serval-projects.component.html line 8 at r2 (raw file):

Previously, pmachapman (Peter Chapman) wrote…

If I do that, I would have to duplicate the inverse of the logic in the "No projects found" div at the bottom. Nesting I think is simpler?

Agreed - this is the nature of code reviewing from the top down :)

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/serval-projects.component.html line 58 at r2 (raw file):

Previously, pmachapman (Peter Chapman) wrote…

I have no idea what is causing that. It is with the Paginator component and affects it where ever it is used, based on my testing. I will file a separate bug for the paginator issues.

Thanks - happy for it to be resolved in another PR if not specific to this one.

---

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

Previously, pmachapman (Peter Chapman) wrote…

Pagination values are passed to the query, so not all data will be on the client.

I guess if we're potentially dealing with 1000s of records then this makes sense. Normally I'd expect pagination in a SPA just to go through the data in memory. This page is also only for admins and is online only.

---

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

Previously, pmachapman (Peter Chapman) wrote…

I have recreated this bug in other places the mat-paginator is used. I will look at this as a separate issue with the other paginator bugs.

Thanks

[pmachapman]

Dismissed @Github-advanced-security[bot] from a discussion.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @nigel-wells)

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/serval-project.component.html line 2 at r2 (raw file):

Previously, nigel-wells (Nigel Wells) wrote…

Ah, ok, that makes sense. What will the main nav show for projects you do not have access to?

It didn't display at all in my tests.

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/serval-project.component.html line 14 at r2 (raw file):

Previously, nigel-wells (Nigel Wells) wrote…

All good - only do if there is value in knowing what that date/time is without having to look i.e. would seeing the date/time on the screen change/influence someone's decision around downloading the documents.

No, I don't think it will affect it. There will be a need to view them anyway. I plan to add a "sync now" type feature in future (as a sync is run before a build is run), but I haven't thought through exactly how that would work (permissions make this interesting...).

---

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

Previously, nigel-wells (Nigel Wells) wrote…

Yes, a user needs to click on the link but my question is around why the backend doesn't then serve up the appropriate headers for downloading a file. The backend is already fetching the file to be downloaded but it is returned in a request to the client to process rather than headers to trigger the browser to download it. Should be a lot less code if the backend does it. Something along the lines of:

httpResponseMessage.Content.Headers.ContentDisposition = new System.Net.Http.Headers.ContentDispositionHeaderValue("attachment");
httpResponseMessage.Content.Headers.ContentDisposition.FileName = FileName;
httpResponseMessage.Content.Headers.ContentType = new System.Net.Http.Headers.MediaTypeHeaderValue("application/octet-stream");

It may be that our requests to the backend are configured to allow for a response like that?

That method doesn't work, as the HttpInterceptor cannot intercept links that are clicked by the user. Downloads can only be triggered by links that are clicked. Its kind of a weird paradox which I find the solution to be unusual:

The solution is that we download the data as a blob in memory, create a link to a that blob in the DOM, and script a "click" on it.

This stack overflow question explains the sort of problem I encountered, and has a similar solution: https://stackoverflow.com/questions/54753021/how-can-i-pass-an-auth-token-when-downloading-a-file

Also, the return File(outputStream, "application/zip", fileName); in the C# code does cause those headers to be set (see https://learn.microsoft.com/en-us/dotnet/api/microsoft.aspnetcore.mvc.controllerbase.file), but they obviously aren't really used (I kept them for completeness' sake).

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/serval-projects.component.html line 58 at r2 (raw file):

Previously, nigel-wells (Nigel Wells) wrote…

Thanks - happy for it to be resolved in another PR if not specific to this one.

I have created https://jira.sil.org/browse/SF-2689 to document these issues.

---

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

Previously, nigel-wells (Nigel Wells) wrote…

I guess if we're potentially dealing with 1000s of records then this makes sense. Normally I'd expect pagination in a SPA just to go through the data in memory. This page is also only for admins and is online only.

I think the problem is related to the second bug documented at https://jira.sil.org/browse/SF-2689, which occurs on the system administration page. Not sure of a solution, as I haven't bug through the commit history which lead to the bug yet.

[nigel-wells]

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @pmachapman)

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/serval-project.component.html line 2 at r2 (raw file):

Previously, pmachapman (Peter Chapman) wrote…

It didn't display at all in my tests.

Oh! So the side effect is the menu shows on projects you do have access to? Should it? I guess I found it confusing but, as this is an admin only thing, I'm not overly fussed. It would be nice if it was consistent though. I won't hold this as a blocker.

---

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

Previously, pmachapman (Peter Chapman) wrote…

That method doesn't work, as the HttpInterceptor cannot intercept links that are clicked by the user. Downloads can only be triggered by links that are clicked. Its kind of a weird paradox which I find the solution to be unusual:

The solution is that we download the data as a blob in memory, create a link to a that blob in the DOM, and script a "click" on it.

This stack overflow question explains the sort of problem I encountered, and has a similar solution: https://stackoverflow.com/questions/54753021/how-can-i-pass-an-auth-token-when-downloading-a-file

Also, the return File(outputStream, "application/zip", fileName); in the C# code does cause those headers to be set (see https://learn.microsoft.com/en-us/dotnet/api/microsoft.aspnetcore.mvc.controllerbase.file), but they obviously aren't really used (I kept them for completeness' sake).

I'm with you now - seems like there should be a "normal" work around as at the end of the day it is still a request taking place. Browsers are being picky though for security reasons so we have work with that.

[nigel-wells]

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @pmachapman)

a discussion (no related file):
Just the one comment there around the menu. If you're happy for that to hold anything up then feel free to move it to testing.

---

[pmachapman]

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @pmachapman)

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/serval-project.component.html line 2 at r2 (raw file):

Should it?

I don't know. I think for the purposes of this admin area, it is acceptable, as there is no need to access anything in the menu.

[Nateowami]

Reviewed 4 of 8 files at r7.
Reviewable status: 33 of 37 files reviewed, 1 unresolved discussion (waiting on @nigel-wells and @pmachapman)

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/serval-project.component.spec.ts line 25 at r7 (raw file):

const mockServalAdministrationService = mock(ServalAdministrationService);

fdescribe('ServalProjectComponent', () => {

Should be describe

[pmachapman]

Reviewable status: 33 of 37 files reviewed, 1 unresolved discussion (waiting on @Nateowami and @nigel-wells)

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/serval-project.component.spec.ts line 25 at r7 (raw file):

Previously, Nateowami (Nathaniel Paulus) wrote…

Should be describe

Done. Thank you so much!

[Nateowami]

Reviewable status: 33 of 37 files reviewed, 1 unresolved discussion (waiting on @nigel-wells and @pmachapman)

---

src/SIL.XForge.Scripture/ClientApp/src/xforge-common/error-dialog/error-dialog.component.spec.ts line 55 at r8 (raw file):

    env.closeButton.click();
    flush();

The tests pass for me without flush(). Is it still needed?

[pmachapman]

Reviewable status: 33 of 37 files reviewed, 1 unresolved discussion (waiting on @Nateowami and @nigel-wells)

---

src/SIL.XForge.Scripture/ClientApp/src/xforge-common/error-dialog/error-dialog.component.spec.ts line 55 at r8 (raw file):

Previously, Nateowami (Nathaniel Paulus) wrote…

The tests pass for me without flush(). Is it still needed?

I think so - when rebased on last week's master, these tests failed and required the flush() statements. I think that bug will occur again sometime in future. It has stopped with the new test order based on the 12345 seed with the current tests in master.

If you think we should revert it and wait for the bug to reappear in future, that is OK too, and I can do that.

Dismissing

[Nateowami]

Reviewable status: 27 of 37 files reviewed, all discussions resolved (waiting on @nigel-wells)

---

src/SIL.XForge.Scripture/ClientApp/src/xforge-common/error-dialog/error-dialog.component.spec.ts line 55 at r8 (raw file):

Previously, pmachapman (Peter Chapman) wrote…

I think so - when rebased on last week's master, these tests failed and required the flush() statements. I think that bug will occur again sometime in future. It has stopped with the new test order based on the 12345 seed with the current tests in master.

If you think we should revert it and wait for the bug to reappear in future, that is OK too, and I can do that.

Jasmine always reports what the seed was for each test run, so it should be possible to get the seed whenever they fail:

Of course, that only helps if the failure depends on the seed.

Looking at the error dialog, it is an extremely simple component, with nothing that even creates any kind of timer. The only thing I can think of that might create a timer of some sort would be the Material dialog component.