fix: Assignments Fixes (#431)

* fix(assignments-service): Flatten openAI and Moss config on patch * fix(frontend): Save selected OpenAI model * fix(frontend): Show toast if student list cannot be loaded * refactor(frontend): Better import success messages * fix(frontend): Z-fighting in solution table * fix(frontend): Handle no assignees edge case in solution table * fix(assignments-service): Invalid Elasticsearch queries for embeddings * fix(frontend): Better solution-table z-index * ci: Remove audit steps
fujaba · Oct 8, 2024 · 685b4d7 · 685b4d7
1 parent bc43236
commit 685b4d7
Show file tree

Hide file tree

Showing 14 changed files with 109 additions and 77 deletions.
diff --git a/.github/workflows/test-frontend.yml b/.github/workflows/test-frontend.yml
@@ -34,8 +34,6 @@ jobs:
           cache-dependency-path: frontend/pnpm-lock.yaml
       - name: Install dependencies
         run: pnpm install --frozen-lockfile
-      - name: Audit
-        run: pnpm audit
       - name: Lint
         run: pnpm lint
       - name: Test

diff --git a/.github/workflows/test-services.yml b/.github/workflows/test-services.yml
@@ -34,8 +34,6 @@ jobs:
           cache-dependency-path: services/pnpm-lock.yaml
       - name: Install dependencies
         run: pnpm install --frozen-lockfile
-      - name: Audit
-        run: pnpm audit
       - name: Lint
         run: pnpm lint
       - name: Test

diff --git a/frontend/src/app/assignment/modules/assignment/solution-table/solution-table.component.html b/frontend/src/app/assignment/modules/assignment/solution-table/solution-table.component.html
@@ -1,4 +1,4 @@
-<div class="p-3 d-flex align-items-center sticky-top bg-body-secondary" style="top: 3.5rem">
+<div class="p-3 d-flex align-items-center sticky-top bg-body-secondary">
   <div class="position-relative me-sm-2 flex-grow-1">
     <input type="search" class="form-control" id="searchInput" [class.is-invalid]="searchError"
            placeholder="Search for Name, Student ID, E-Mail, GitHub Username, Assignee"
@@ -18,23 +18,31 @@
     popoverTitle="Search Help"
     placement="bottom-right"
   ></button>
-  <div *ngFor="let criteria of [
+  @for (criteria of [
     {id: 'assignee', title: 'Assignee', icon: 'bi-person-check', options: assigneeNames},
     {id: 'status', title: 'Status', icon: 'bi-award', options: solutionStatus},
-  ]" ngbDropdown #dropdown=ngbDropdown class="d-inline-block me-2">
-    <button class="btn btn-outline-secondary" [id]="criteria.id + '-dropdown'" ngbDropdownToggle [ngbTooltip]="criteria.title">
-      <i [class]="criteria.icon"></i>
-      <span class="visually-hidden">{{ criteria.title }}</span>
-    </button>
-    <div ngbDropdownMenu [attr.aria-labelledby]="criteria.id + '-dropdown'">
-      @if (dropdown.isOpen()) {
-        <button *ngFor="let option of criteria.options" ngbDropdownItem (click)="toggleSearch(criteria.id, option)"
-                class="dropdown-item-check" [class.checked]="hasSearch(criteria.id, option)">
-          {{ option }}
-        </button>
-      }
+  ]; track criteria.id) {
+    <div ngbDropdown #dropdown=ngbDropdown class="d-inline-block me-2">
+      <button class="btn btn-outline-secondary" [id]="criteria.id + '-dropdown'" ngbDropdownToggle [ngbTooltip]="criteria.title">
+        <i [class]="criteria.icon"></i>
+        <span class="visually-hidden">{{ criteria.title }}</span>
+      </button>
+      <div ngbDropdownMenu [attr.aria-labelledby]="criteria.id + '-dropdown'">
+        @if (dropdown.isOpen()) {
+          @for (option of criteria.options; track option) {
+            <button ngbDropdownItem (click)="toggleSearch(criteria.id, option)"
+                    class="dropdown-item-check" [class.checked]="hasSearch(criteria.id, option)">
+              {{ option }}
+            </button>
+          } @empty {
+            <div class="dropdown-item-text text-muted">
+              No {{ criteria.title }} available
+            </div>
+          }
+        }
+      </div>
     </div>
-  </div>
+  }
   <div style="width: 12rem">
     @if ((selected | keyvalue).length;as length) {
       {{ length }} /
@@ -51,16 +59,18 @@
         <span class="visually-hidden">Bulk Actions</span>
       </button>
       <div ngbDropdownMenu aria-labelledby="actionsDropdown">
-        <div class="dropdown-header">
-          Assign {{ count }} Solutions to...
-        </div>
-        @for (assignee of assigneeNames; track assignee) {
-          <button ngbDropdownItem (click)="assignAll(assignee)">
-            <span class="badge" [style.background-color]="assignee | assigneeColor">{{ assignee | initials }}</span>
-            {{ assignee }}
-          </button>
+        @if (assigneeNames.length) {
+          <div class="dropdown-header">
+            Assign {{ count }} Solutions to...
+          </div>
+          @for (assignee of assigneeNames; track assignee) {
+            <button ngbDropdownItem (click)="assignAll(assignee)">
+              <span class="badge" [style.background-color]="assignee | assigneeColor">{{ assignee | initials }}</span>
+              {{ assignee }}
+            </button>
+          }
+          <div class="dropdown-divider"></div>
         }
-        <div class="dropdown-divider"></div>
         <button ngbDropdownItem (click)="openSelected()">
           Open {{ count }} Solutions in new tabs
         </button>
@@ -228,14 +238,12 @@
               ngbTooltip="View Solution"
             ></a>
             @if (assignment && assignment.classroom?.org && assignment.classroom?.prefix && solution.author.github) {
-              <div class="btn-group me-1">
-                <a
-                  class="btn btn-secondary bi-github"
-                  [href]="assignment|githubLink:solution:true"
-                  target="_blank"
-                  ngbTooltip="View {{solution.commit ? 'Commit' : 'Repository'}} on GitHub"
-                ></a>
-              </div>
+              <a
+                class="btn btn-secondary bi-github me-1"
+                [href]="assignment|githubLink:solution:true"
+                target="_blank"
+                ngbTooltip="View {{solution.commit ? 'Commit' : 'Repository'}} on GitHub"
+              ></a>
             }
             <a
               class="btn btn-primary me-1 bi-check-circle"

diff --git a/frontend/src/app/assignment/modules/assignment/solution-table/solution-table.component.scss b/frontend/src/app/assignment/modules/assignment/solution-table/solution-table.component.scss
@@ -2,3 +2,8 @@
 .table-centered th {
   vertical-align: middle;
 }
+
+.sticky-top {
+  top: 3.5rem;
+  z-index: 990;
+}
diff --git a/frontend/src/app/assignment/modules/edit-assignment/code-search/code-search.component.html b/frontend/src/app/assignment/modules/edit-assignment/code-search/code-search.component.html
@@ -35,7 +35,7 @@
     </label>
     @for (model of embeddingModels; track model.id) {
       <div class="form-check">
-        <input class="form-check-input" type="radio" name="openaiModel" id="openaiModel-{{model.id}}" [value]="model.id" [ngModel]="openAI.model">
+        <input class="form-check-input" type="radio" name="openaiModel" id="openaiModel-{{model.id}}" [value]="model.id" [(ngModel)]="openAI.model">
         <label class="form-check-label" for="openaiModel-{{model.id}}">
           {{ model.id }}
           <span class="badge bg-{{ model.labelBg }}">

diff --git a/frontend/src/app/assignment/modules/import/import-consent/import-consent.component.ts b/frontend/src/app/assignment/modules/import/import-consent/import-consent.component.ts
@@ -10,13 +10,14 @@ import Solution, {
 import {map} from "rxjs/operators";
 import {SolutionService} from "../../../services/solution.service";
 import {ActivatedRoute} from "@angular/router";
+import {ImportTab} from '../import-tab.interface';
 
 @Component({
   selector: 'app-import-consent',
   templateUrl: './import-consent.component.html',
   styleUrls: ['./import-consent.component.scss']
 })
-export class ImportConsentComponent {
+export class ImportConsentComponent implements ImportTab {
   consentText = '';
 
   constructor(
@@ -25,7 +26,7 @@ export class ImportConsentComponent {
   ) {
   }
 
-  import(): Observable<ImportSolution[]> {
+  import() {
     const assignment = this.route.snapshot.params.aid;
     const lines = this.consentText.split('\n');
     const splitter = /[\t,;]/;
@@ -51,6 +52,9 @@ export class ImportConsentComponent {
         updates.push({author, consent});
       }
     }
-    return this.solutionService.updateMany(assignment, updates).pipe(map(results => results.filter(s => s)));
+    return this.solutionService.updateMany(assignment, updates).pipe(
+      map(results => results.filter(s => s)),
+      map(results => `Successfully updated ${results.length} solutions`),
+    );
   }
 }
diff --git a/frontend/src/app/assignment/modules/import/import-embeddings/import-embeddings.component.ts b/frontend/src/app/assignment/modules/import/import-embeddings/import-embeddings.component.ts
@@ -1,15 +1,16 @@
 import {Component, OnInit} from '@angular/core';
 import {EstimatedCosts} from "../../../model/solution";
 import {ActivatedRoute} from "@angular/router";
-import {switchMap, tap} from "rxjs/operators";
+import {map, switchMap, tap} from 'rxjs/operators';
 import {EmbeddingService} from "../../../services/embedding.service";
+import {ImportTab} from '../import-tab.interface';
 
 @Component({
   selector: 'app-import-embeddings',
   templateUrl: './import-embeddings.component.html',
   styleUrls: ['./import-embeddings.component.scss']
 })
-export class ImportEmbeddingsComponent implements OnInit {
+export class ImportEmbeddingsComponent implements OnInit, ImportTab {
   costs?: EstimatedCosts;
   costsAreFinal = false;
 
@@ -36,6 +37,7 @@ export class ImportEmbeddingsComponent implements OnInit {
         this.costs = result;
         this.costsAreFinal = true;
       }),
+      map(result => `Successfully imported ${result.functions.length} function embeddings via OpenAI.`),
     );
   }
 }
diff --git a/frontend/src/app/assignment/modules/import/import-files/import-files.component.ts b/frontend/src/app/assignment/modules/import/import-files/import-files.component.ts
@@ -1,13 +1,15 @@
 import {Component} from '@angular/core';
 import {SolutionService} from "../../../services/solution.service";
 import {ActivatedRoute} from "@angular/router";
+import {ImportTab} from '../import-tab.interface';
+import {map} from 'rxjs/operators';
 
 @Component({
   selector: 'app-import-files',
   templateUrl: './import-files.component.html',
   styleUrls: ['./import-files.component.scss']
 })
-export class ImportFilesComponent {
+export class ImportFilesComponent implements ImportTab {
   files: File[] = [];
 
   constructor(
@@ -22,6 +24,8 @@ export class ImportFilesComponent {
 
   import() {
     const assignmentId = this.route.snapshot.params.aid;
-    return this.solutionService.import(assignmentId, this.files);
+    return this.solutionService.import(assignmentId, this.files).pipe(
+      map(results => `Successfully imported ${results.length} solutions from files`),
+    );
   }
 }
diff --git a/frontend/src/app/assignment/modules/import/import-github/import-github.component.ts b/frontend/src/app/assignment/modules/import/import-github/import-github.component.ts
@@ -2,30 +2,42 @@ import {Component} from '@angular/core';
 import {ImportSolution} from "../../../model/solution";
 import {SolutionService} from "../../../services/solution.service";
 import {ActivatedRoute} from "@angular/router";
+import {ToastService} from '@mean-stream/ngbx';
+import {ImportTab} from '../import-tab.interface';
+import {map} from 'rxjs/operators';
 
 @Component({
   selector: 'app-import-github',
   templateUrl: './import-github.component.html',
   styleUrls: ['./import-github.component.scss']
 })
-export class ImportGithubComponent {
+export class ImportGithubComponent implements ImportTab {
   checkedUsernames: Partial<Record<string, boolean>> = {};
-  previewSolutions: ImportSolution[];
+  previewSolutions?: ImportSolution[];
   reimport = false;
 
   constructor(
     private solutionService: SolutionService,
     private route: ActivatedRoute,
+    private toastService: ToastService,
   ) {
   }
 
   previewGitHubImport() {
-    this.solutionService.previewImport(this.route.snapshot.params.aid).subscribe(solutions => this.previewSolutions = solutions);
+    this.solutionService.previewImport(this.route.snapshot.params.aid).subscribe({
+      next: solutions => this.previewSolutions = solutions,
+      error: error => {
+        this.toastService.error('Load Students', 'Failed to load Students from GitHub', error);
+        this.previewSolutions = [];
+      },
+    });
   }
 
   import() {
     const assignmentId = this.route.snapshot.params.aid;
     const usernames = Object.keys(this.checkedUsernames).filter(username => this.checkedUsernames[username]);
-    return this.solutionService.import(assignmentId, undefined, usernames, this.reimport);
+    return this.solutionService.import(assignmentId, undefined, usernames, this.reimport).pipe(
+      map(results => `Successfully imported ${results.length} solutions from GitHub`),
+    );
   }
 }
diff --git a/frontend/src/app/assignment/modules/import/import-modal/import-modal.component.ts b/frontend/src/app/assignment/modules/import/import-modal/import-modal.component.ts
@@ -21,19 +21,13 @@ export class ImportModalComponent {
   import(component: any) {
     this.importing = true;
     component.import().subscribe({
-      next: results => {
+      next: message => {
         this.importing = false;
-        if (typeof results === 'string') {
-          this.toastService.success('Import', 'Successfully ran MOSS');
-        } else if (results && typeof results === 'object' && 'length' in results) {
-          this.toastService.success('Import', `Successfully imported ${results.length} solutions`);
-        } else {
-          this.toastService.success('Import', 'Successfully imported embeddings');
-        }
+        this.toastService.success('Import', message);
       },
       error: error => {
         this.importing = false;
-        this.toastService.error('Import', 'Failed to import solutions', error);
+        this.toastService.error('Import', `Failed to import: ${error.message}`, error);
       },
     });
   }

diff --git a/frontend/src/app/assignment/modules/import/import-moss/import-moss.component.ts b/frontend/src/app/assignment/modules/import/import-moss/import-moss.component.ts
@@ -1,14 +1,15 @@
 import {Component} from '@angular/core';
 import {ActivatedRoute} from "@angular/router";
 import {AssignmentService} from "../../../services/assignment.service";
-import {tap} from "rxjs/operators";
+import {map, tap} from 'rxjs/operators';
+import {ImportTab} from '../import-tab.interface';
 
 @Component({
   selector: 'app-import-moss',
   templateUrl: './import-moss.component.html',
   styleUrls: ['./import-moss.component.scss']
 })
-export class ImportMossComponent {
+export class ImportMossComponent implements ImportTab {
   mossResult?: string;
 
   constructor(
@@ -21,6 +22,7 @@ export class ImportMossComponent {
     const assignmentId = this.route.snapshot.params.aid;
     return this.assignmentService.moss(assignmentId).pipe(
       tap(result => this.mossResult = result),
+      map(result => `Successfully imported to MOSS. Results are available at: ${result}`),
     );
   }
 }
diff --git a/frontend/src/app/assignment/modules/import/import-tab.interface.ts b/frontend/src/app/assignment/modules/import/import-tab.interface.ts
@@ -0,0 +1,5 @@
+import {Observable} from 'rxjs';
+
+export interface ImportTab {
+  import(): Observable<string>;
+}
diff --git a/services/apps/assignments/src/assignment/assignment.controller.ts b/services/apps/assignments/src/assignment/assignment.controller.ts
@@ -112,15 +112,19 @@ export class AssignmentController {
     @Param('id', ObjectIdPipe) id: Types.ObjectId,
     @Body() dto: UpdateAssignmentDto,
   ): Promise<Assignment | null> {
-    const {token, classroom, ...rest} = dto;
+    const {token, ...rest} = dto;
     const update: UpdateQuery<Assignment> = rest;
     if (token) {
       update.token = generateToken();
     }
-    if (classroom) {
-      // need to flatten the classroom object to prevent deleting the GitHub token all the time
-      for (const [key, value] of Object.entries(classroom)) {
-        update[`classroom.${key}`] = value;
+    for (const key of ['classroom', 'openAI', 'moss'] as const) {
+      const obj = rest[key];
+      if (obj) {
+        delete update[key];
+        // need to flatten these objects to prevent deleting the tokens all the time
+        for (const [subKey, value] of Object.entries(obj)) {
+          update[`${key}.${subKey}`] = value;
+        }
       }
     }
     return this.assignmentService.update(id, update);

diff --git a/services/apps/assignments/src/embedding/embedding.service.ts b/services/apps/assignments/src/embedding/embedding.service.ts
@@ -238,12 +238,10 @@ export class EmbeddingService implements OnModuleInit {
       index: 'embeddings',
       query: {
         bool: {
-          must: {
-            term: {
-              assignment,
-              type: 'task',
-            },
-          },
+          filter: [
+            {term: {assignment}},
+            {term: {type: 'task'}},
+          ],
           must_not: {
             terms: {
               task: tasks,
@@ -260,7 +258,7 @@ export class EmbeddingService implements OnModuleInit {
       index: 'embeddings',
       query: {
         bool: {
-          must: {
+          filter: {
             term: {
               assignment,
             },
@@ -276,12 +274,10 @@ export class EmbeddingService implements OnModuleInit {
       index: 'embeddings',
       query: {
         bool: {
-          must: {
-            term: {
-              assignment,
-              solution,
-            },
-          },
+          filter: [
+            {term: {assignment}},
+            {term: {solution}},
+          ],
         },
       },
     });