From c2b658adf17f5e40d8ef65daa6afeab9014b9e4d Mon Sep 17 00:00:00 2001 From: reid holmes Date: Tue, 30 Apr 2024 12:22:59 -0700 Subject: [PATCH 01/80] see #109; improving table export --- .../frontend/src/app/util/SortableTable.ts | 58 ++++++++++++++++++- 1 file changed, 57 insertions(+), 1 deletion(-) diff --git a/packages/portal/frontend/src/app/util/SortableTable.ts b/packages/portal/frontend/src/app/util/SortableTable.ts index 515ebccd5..e4cb65a69 100644 --- a/packages/portal/frontend/src/app/util/SortableTable.ts +++ b/packages/portal/frontend/src/app/util/SortableTable.ts @@ -269,9 +269,58 @@ export class SortableTable { // downloadLink.click(); } + // split this into hover and links as separate methods to simplify callers needing to figure out which is which + private findColsWithMetadata(divName: string): number[] { + const root = document.querySelector(this.divName); + const rows = root.querySelectorAll("table tr"); + const colsWithMetadata: number[] = []; + + // tslint:disable-next-line + for (let i = 0; i < rows.length; i++) { + const cols = rows[i].querySelectorAll("td, th"); + + // tslint:disable-next-line + for (let j = 0; j < cols.length; j++) { + const col = cols[j] as HTMLElement; + // document.getElementById('gradesListTable').children[0]...children[0] instanceof HTMLAnchorElement <-- true + // typeof document.getElementById('gradesListTable').children[0]...children[0].title === "string" <-- true + if (col.children.length > 0 && + (col.children[0] instanceof HTMLAnchorElement || typeof (col as any).children[0].title === "string")) { + if (colsWithMetadata.indexOf(j) < 0) { + colsWithMetadata.push(j); + } + } + } + } + + Log.info("SortableTable::findColsWithMetadata() - cols: " + JSON.stringify(colsWithMetadata)); + return colsWithMetadata; + } + + private escapeCSVValue(value: string): string { + let sanitized = value.replace(/"/g, ""); // remove all double quotes + sanitized = value.replace(/'/g, ""); // remove all single quotes + sanitized = sanitized.replace(/ /g, " "); // replace all   with a space + sanitized = sanitized.replace(/,/g, " "); // remove all commas + return sanitized; + } + + private extractMetadata(elem: HTMLElement) { + let out = ""; + if (elem.children[0] instanceof HTMLAnchorElement) { + out = this.escapeCSVValue((elem.children[0] as HTMLAnchorElement).href); + } else if (typeof (elem as any).children[0].title === "string") { + out = this.escapeCSVValue((elem as any).children[0].title); + } + Log.info("SortableTable::extractMetadata() - value: " + out); // remove after working + return out; + } + private exportTableToCSV() { const csv = []; const root = document.querySelector(this.divName); + const colsWithMetadata = this.findColsWithMetadata(this.divName); + const rows = root.querySelectorAll("table tr"); for (let i = 0; i < rows.length; i++) { @@ -284,9 +333,16 @@ export class SortableTable { let text = (cols[j] as HTMLTableCellElement).innerText; text = text.replace(" ▼", ""); text = text.replace(" ▲", ""); + text = text.trim(); row.push(text); } else { - row.push((cols[j] as HTMLTableCellElement).innerText); + let text = (cols[j] as HTMLTableCellElement).innerText; + text = text.trim(); + row.push(text); + } + + if (colsWithMetadata.indexOf(j) >= 0) { + row.push(this.extractMetadata(cols[j] as HTMLElement)); } } csv.push(row.join(",")); From b85930ed55695927bc1c3b058ec80e3843e8d6cd Mon Sep 17 00:00:00 2001 From: reid holmes Date: Tue, 30 Apr 2024 12:51:49 -0700 Subject: [PATCH 02/80] make title optional --- packages/portal/frontend/src/app/util/SortableTable.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/portal/frontend/src/app/util/SortableTable.ts b/packages/portal/frontend/src/app/util/SortableTable.ts index e4cb65a69..6d8ebd0c6 100644 --- a/packages/portal/frontend/src/app/util/SortableTable.ts +++ b/packages/portal/frontend/src/app/util/SortableTable.ts @@ -285,7 +285,7 @@ export class SortableTable { // document.getElementById('gradesListTable').children[0]...children[0] instanceof HTMLAnchorElement <-- true // typeof document.getElementById('gradesListTable').children[0]...children[0].title === "string" <-- true if (col.children.length > 0 && - (col.children[0] instanceof HTMLAnchorElement || typeof (col as any).children[0].title === "string")) { + (col.children[0] instanceof HTMLAnchorElement || typeof (col as any).children[0]?.title === "string")) { if (colsWithMetadata.indexOf(j) < 0) { colsWithMetadata.push(j); } @@ -309,7 +309,7 @@ export class SortableTable { let out = ""; if (elem.children[0] instanceof HTMLAnchorElement) { out = this.escapeCSVValue((elem.children[0] as HTMLAnchorElement).href); - } else if (typeof (elem as any).children[0].title === "string") { + } else if (typeof (elem as any).children[0]?.title === "string") { out = this.escapeCSVValue((elem as any).children[0].title); } Log.info("SortableTable::extractMetadata() - value: " + out); // remove after working From beef7cc54021ec666cbc65d1166a2d1381336a2b Mon Sep 17 00:00:00 2001 From: reid holmes Date: Tue, 30 Apr 2024 12:58:42 -0700 Subject: [PATCH 03/80] better header names for metadata rows --- .../portal/frontend/src/app/util/SortableTable.ts | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/packages/portal/frontend/src/app/util/SortableTable.ts b/packages/portal/frontend/src/app/util/SortableTable.ts index 6d8ebd0c6..57be5b32a 100644 --- a/packages/portal/frontend/src/app/util/SortableTable.ts +++ b/packages/portal/frontend/src/app/util/SortableTable.ts @@ -342,7 +342,18 @@ export class SortableTable { } if (colsWithMetadata.indexOf(j) >= 0) { - row.push(this.extractMetadata(cols[j] as HTMLElement)); + if (i === 0) { + // header row + if (j > 0) { + row.push(row[j - 1] + "_metadata"); // add metadata prior column name + } else { + // just leave blank + row.push(""); + } + } else { + // regular row + row.push(this.extractMetadata(cols[j] as HTMLElement)); + } } } csv.push(row.join(",")); From 917927f638dea14e6b7fe52db664d73f4163e533 Mon Sep 17 00:00:00 2001 From: reid holmes Date: Tue, 30 Apr 2024 13:03:24 -0700 Subject: [PATCH 04/80] fix #109; cleanups and tested in prod --- packages/portal/frontend/src/app/util/SortableTable.ts | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/packages/portal/frontend/src/app/util/SortableTable.ts b/packages/portal/frontend/src/app/util/SortableTable.ts index 57be5b32a..26aba67df 100644 --- a/packages/portal/frontend/src/app/util/SortableTable.ts +++ b/packages/portal/frontend/src/app/util/SortableTable.ts @@ -312,7 +312,7 @@ export class SortableTable { } else if (typeof (elem as any).children[0]?.title === "string") { out = this.escapeCSVValue((elem as any).children[0].title); } - Log.info("SortableTable::extractMetadata() - value: " + out); // remove after working + // Log.info("SortableTable::extractMetadata() - value: " + out); // remove after working return out; } @@ -344,12 +344,7 @@ export class SortableTable { if (colsWithMetadata.indexOf(j) >= 0) { if (i === 0) { // header row - if (j > 0) { - row.push(row[j - 1] + "_metadata"); // add metadata prior column name - } else { - // just leave blank - row.push(""); - } + row.push(row[j] + "_metadata"); // add metadata prior column name } else { // regular row row.push(this.extractMetadata(cols[j] as HTMLElement)); From 030aba30c1c036dc39fd8b74d8dcae1bf2c5c071 Mon Sep 17 00:00:00 2001 From: reid holmes Date: Tue, 30 Apr 2024 13:21:03 -0700 Subject: [PATCH 05/80] wrong cell --- .../frontend/src/app/util/SortableTable.ts | 72 ++++++++++--------- 1 file changed, 37 insertions(+), 35 deletions(-) diff --git a/packages/portal/frontend/src/app/util/SortableTable.ts b/packages/portal/frontend/src/app/util/SortableTable.ts index 26aba67df..5cd3713f8 100644 --- a/packages/portal/frontend/src/app/util/SortableTable.ts +++ b/packages/portal/frontend/src/app/util/SortableTable.ts @@ -344,7 +344,7 @@ export class SortableTable { if (colsWithMetadata.indexOf(j) >= 0) { if (i === 0) { // header row - row.push(row[j] + "_metadata"); // add metadata prior column name + row.push(cols[j] + "_metadata"); // add metadata prior column name } else { // regular row row.push(this.extractMetadata(cols[j] as HTMLElement)); @@ -357,38 +357,39 @@ export class SortableTable { return csv.join("\n"); } - private exportTableLinksToCSV() { - const csv = []; - const root = document.querySelector(this.divName); - const rows = root.querySelectorAll("table tr"); - - for (let i = 0; i < rows.length; i++) { - const row = []; - const cols = rows[i].querySelectorAll("td, th"); - - // tslint:disable-next-line - for (let j = 0; j < cols.length; j++) { - if (i === 0) { - let text = (cols[j] as HTMLTableCellElement).innerText; - text = text.replace(" ▼", ""); - text = text.replace(" ▲", ""); - row.push(text); - } else { - const col = cols[j] as HTMLElement; - - // this is super brittle - if (col.children.length > 0 && col.children[0] instanceof HTMLAnchorElement) { - row.push((col.children[0] as HTMLAnchorElement).href); - } else { - row.push(col.innerText); - } - } - } - csv.push(row.join(",")); - } - - return csv.join("\n"); - } + // no longer used + // private exportTableLinksToCSV() { + // const csv = []; + // const root = document.querySelector(this.divName); + // const rows = root.querySelectorAll("table tr"); + // + // for (let i = 0; i < rows.length; i++) { + // const row = []; + // const cols = rows[i].querySelectorAll("td, th"); + // + // // tslint:disable-next-line + // for (let j = 0; j < cols.length; j++) { + // if (i === 0) { + // let text = (cols[j] as HTMLTableCellElement).innerText; + // text = text.replace(" ▼", ""); + // text = text.replace(" ▲", ""); + // row.push(text); + // } else { + // const col = cols[j] as HTMLElement; + // + // // this is super brittle + // if (col.children.length > 0 && col.children[0] instanceof HTMLAnchorElement) { + // row.push((col.children[0] as HTMLAnchorElement).href); + // } else { + // row.push(col.innerText); + // } + // } + // } + // csv.push(row.join(",")); + // } + // + // return csv.join("\n"); + // } public numRows(): number { return this.rows.length; @@ -397,8 +398,9 @@ export class SortableTable { private attachDownload() { const csv = this.exportTableToCSV(); this.downloadCSV(csv, "classy.csv", "Download Values as CSV "); - const links = this.exportTableLinksToCSV(); - this.downloadCSV(links, "classyLinks.csv", " Download Links as CSV"); + // no longer needed; regular csv now includes these + // const links = this.exportTableLinksToCSV(); + // this.downloadCSV(links, "classyLinks.csv", " Download Links as CSV"); } /** From 91230ca03ad9703759d4de0e464c80ca7734bbf8 Mon Sep 17 00:00:00 2001 From: reid holmes Date: Tue, 30 Apr 2024 13:27:57 -0700 Subject: [PATCH 06/80] offsets --- packages/portal/frontend/src/app/util/SortableTable.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/portal/frontend/src/app/util/SortableTable.ts b/packages/portal/frontend/src/app/util/SortableTable.ts index 5cd3713f8..870c062be 100644 --- a/packages/portal/frontend/src/app/util/SortableTable.ts +++ b/packages/portal/frontend/src/app/util/SortableTable.ts @@ -344,7 +344,9 @@ export class SortableTable { if (colsWithMetadata.indexOf(j) >= 0) { if (i === 0) { // header row - row.push(cols[j] + "_metadata"); // add metadata prior column name + // add metadata prior column name + // strange math because we may have added columns to the left + row.push(row[j + colsWithMetadata.indexOf(j)] + "_metadata"); } else { // regular row row.push(this.extractMetadata(cols[j] as HTMLElement)); From d8224005644f3186c1be9fa3f78363a8a074d996 Mon Sep 17 00:00:00 2001 From: reid holmes Date: Tue, 30 Apr 2024 13:52:52 -0700 Subject: [PATCH 07/80] sort colsWithMetadata --- packages/portal/frontend/src/app/util/SortableTable.ts | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/packages/portal/frontend/src/app/util/SortableTable.ts b/packages/portal/frontend/src/app/util/SortableTable.ts index 870c062be..8c9d8b84e 100644 --- a/packages/portal/frontend/src/app/util/SortableTable.ts +++ b/packages/portal/frontend/src/app/util/SortableTable.ts @@ -273,10 +273,10 @@ export class SortableTable { private findColsWithMetadata(divName: string): number[] { const root = document.querySelector(this.divName); const rows = root.querySelectorAll("table tr"); - const colsWithMetadata: number[] = []; + let colsWithMetadata: number[] = []; // tslint:disable-next-line - for (let i = 0; i < rows.length; i++) { + for (let i = 1; i < rows.length; i++) { // skip the header row const cols = rows[i].querySelectorAll("td, th"); // tslint:disable-next-line @@ -293,6 +293,9 @@ export class SortableTable { } } + // sort numbers ascending + colsWithMetadata = colsWithMetadata.sort((a, b) => a - b); + Log.info("SortableTable::findColsWithMetadata() - cols: " + JSON.stringify(colsWithMetadata)); return colsWithMetadata; } From 34bc7a35cbe0271ba611454ce208413e4c38eb80 Mon Sep 17 00:00:00 2001 From: reid holmes Date: Tue, 30 Apr 2024 12:22:59 -0700 Subject: [PATCH 08/80] see #109; improving table export --- .../frontend/src/app/util/SortableTable.ts | 58 ++++++++++++++++++- 1 file changed, 57 insertions(+), 1 deletion(-) diff --git a/packages/portal/frontend/src/app/util/SortableTable.ts b/packages/portal/frontend/src/app/util/SortableTable.ts index 515ebccd5..e4cb65a69 100644 --- a/packages/portal/frontend/src/app/util/SortableTable.ts +++ b/packages/portal/frontend/src/app/util/SortableTable.ts @@ -269,9 +269,58 @@ export class SortableTable { // downloadLink.click(); } + // split this into hover and links as separate methods to simplify callers needing to figure out which is which + private findColsWithMetadata(divName: string): number[] { + const root = document.querySelector(this.divName); + const rows = root.querySelectorAll("table tr"); + const colsWithMetadata: number[] = []; + + // tslint:disable-next-line + for (let i = 0; i < rows.length; i++) { + const cols = rows[i].querySelectorAll("td, th"); + + // tslint:disable-next-line + for (let j = 0; j < cols.length; j++) { + const col = cols[j] as HTMLElement; + // document.getElementById('gradesListTable').children[0]...children[0] instanceof HTMLAnchorElement <-- true + // typeof document.getElementById('gradesListTable').children[0]...children[0].title === "string" <-- true + if (col.children.length > 0 && + (col.children[0] instanceof HTMLAnchorElement || typeof (col as any).children[0].title === "string")) { + if (colsWithMetadata.indexOf(j) < 0) { + colsWithMetadata.push(j); + } + } + } + } + + Log.info("SortableTable::findColsWithMetadata() - cols: " + JSON.stringify(colsWithMetadata)); + return colsWithMetadata; + } + + private escapeCSVValue(value: string): string { + let sanitized = value.replace(/"/g, ""); // remove all double quotes + sanitized = value.replace(/'/g, ""); // remove all single quotes + sanitized = sanitized.replace(/ /g, " "); // replace all   with a space + sanitized = sanitized.replace(/,/g, " "); // remove all commas + return sanitized; + } + + private extractMetadata(elem: HTMLElement) { + let out = ""; + if (elem.children[0] instanceof HTMLAnchorElement) { + out = this.escapeCSVValue((elem.children[0] as HTMLAnchorElement).href); + } else if (typeof (elem as any).children[0].title === "string") { + out = this.escapeCSVValue((elem as any).children[0].title); + } + Log.info("SortableTable::extractMetadata() - value: " + out); // remove after working + return out; + } + private exportTableToCSV() { const csv = []; const root = document.querySelector(this.divName); + const colsWithMetadata = this.findColsWithMetadata(this.divName); + const rows = root.querySelectorAll("table tr"); for (let i = 0; i < rows.length; i++) { @@ -284,9 +333,16 @@ export class SortableTable { let text = (cols[j] as HTMLTableCellElement).innerText; text = text.replace(" ▼", ""); text = text.replace(" ▲", ""); + text = text.trim(); row.push(text); } else { - row.push((cols[j] as HTMLTableCellElement).innerText); + let text = (cols[j] as HTMLTableCellElement).innerText; + text = text.trim(); + row.push(text); + } + + if (colsWithMetadata.indexOf(j) >= 0) { + row.push(this.extractMetadata(cols[j] as HTMLElement)); } } csv.push(row.join(",")); From 557476c47c98e5b92aabcf01fb5e988cc7db74c7 Mon Sep 17 00:00:00 2001 From: reid holmes Date: Tue, 30 Apr 2024 12:51:49 -0700 Subject: [PATCH 09/80] make title optional --- packages/portal/frontend/src/app/util/SortableTable.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/portal/frontend/src/app/util/SortableTable.ts b/packages/portal/frontend/src/app/util/SortableTable.ts index e4cb65a69..6d8ebd0c6 100644 --- a/packages/portal/frontend/src/app/util/SortableTable.ts +++ b/packages/portal/frontend/src/app/util/SortableTable.ts @@ -285,7 +285,7 @@ export class SortableTable { // document.getElementById('gradesListTable').children[0]...children[0] instanceof HTMLAnchorElement <-- true // typeof document.getElementById('gradesListTable').children[0]...children[0].title === "string" <-- true if (col.children.length > 0 && - (col.children[0] instanceof HTMLAnchorElement || typeof (col as any).children[0].title === "string")) { + (col.children[0] instanceof HTMLAnchorElement || typeof (col as any).children[0]?.title === "string")) { if (colsWithMetadata.indexOf(j) < 0) { colsWithMetadata.push(j); } @@ -309,7 +309,7 @@ export class SortableTable { let out = ""; if (elem.children[0] instanceof HTMLAnchorElement) { out = this.escapeCSVValue((elem.children[0] as HTMLAnchorElement).href); - } else if (typeof (elem as any).children[0].title === "string") { + } else if (typeof (elem as any).children[0]?.title === "string") { out = this.escapeCSVValue((elem as any).children[0].title); } Log.info("SortableTable::extractMetadata() - value: " + out); // remove after working From 9dea4c90938283fc05d3440f4162d22ef394a2fa Mon Sep 17 00:00:00 2001 From: reid holmes Date: Tue, 30 Apr 2024 12:58:42 -0700 Subject: [PATCH 10/80] better header names for metadata rows --- .../portal/frontend/src/app/util/SortableTable.ts | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/packages/portal/frontend/src/app/util/SortableTable.ts b/packages/portal/frontend/src/app/util/SortableTable.ts index 6d8ebd0c6..57be5b32a 100644 --- a/packages/portal/frontend/src/app/util/SortableTable.ts +++ b/packages/portal/frontend/src/app/util/SortableTable.ts @@ -342,7 +342,18 @@ export class SortableTable { } if (colsWithMetadata.indexOf(j) >= 0) { - row.push(this.extractMetadata(cols[j] as HTMLElement)); + if (i === 0) { + // header row + if (j > 0) { + row.push(row[j - 1] + "_metadata"); // add metadata prior column name + } else { + // just leave blank + row.push(""); + } + } else { + // regular row + row.push(this.extractMetadata(cols[j] as HTMLElement)); + } } } csv.push(row.join(",")); From 34b029e54677358e62c8d674fdb6c700126d842e Mon Sep 17 00:00:00 2001 From: reid holmes Date: Tue, 30 Apr 2024 13:03:24 -0700 Subject: [PATCH 11/80] fix #109; cleanups and tested in prod --- packages/portal/frontend/src/app/util/SortableTable.ts | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/packages/portal/frontend/src/app/util/SortableTable.ts b/packages/portal/frontend/src/app/util/SortableTable.ts index 57be5b32a..26aba67df 100644 --- a/packages/portal/frontend/src/app/util/SortableTable.ts +++ b/packages/portal/frontend/src/app/util/SortableTable.ts @@ -312,7 +312,7 @@ export class SortableTable { } else if (typeof (elem as any).children[0]?.title === "string") { out = this.escapeCSVValue((elem as any).children[0].title); } - Log.info("SortableTable::extractMetadata() - value: " + out); // remove after working + // Log.info("SortableTable::extractMetadata() - value: " + out); // remove after working return out; } @@ -344,12 +344,7 @@ export class SortableTable { if (colsWithMetadata.indexOf(j) >= 0) { if (i === 0) { // header row - if (j > 0) { - row.push(row[j - 1] + "_metadata"); // add metadata prior column name - } else { - // just leave blank - row.push(""); - } + row.push(row[j] + "_metadata"); // add metadata prior column name } else { // regular row row.push(this.extractMetadata(cols[j] as HTMLElement)); From de8dc88f3cac3f96557c8d9534ebf140eb8b8788 Mon Sep 17 00:00:00 2001 From: reid holmes Date: Tue, 30 Apr 2024 13:21:03 -0700 Subject: [PATCH 12/80] wrong cell --- .../frontend/src/app/util/SortableTable.ts | 72 ++++++++++--------- 1 file changed, 37 insertions(+), 35 deletions(-) diff --git a/packages/portal/frontend/src/app/util/SortableTable.ts b/packages/portal/frontend/src/app/util/SortableTable.ts index 26aba67df..5cd3713f8 100644 --- a/packages/portal/frontend/src/app/util/SortableTable.ts +++ b/packages/portal/frontend/src/app/util/SortableTable.ts @@ -344,7 +344,7 @@ export class SortableTable { if (colsWithMetadata.indexOf(j) >= 0) { if (i === 0) { // header row - row.push(row[j] + "_metadata"); // add metadata prior column name + row.push(cols[j] + "_metadata"); // add metadata prior column name } else { // regular row row.push(this.extractMetadata(cols[j] as HTMLElement)); @@ -357,38 +357,39 @@ export class SortableTable { return csv.join("\n"); } - private exportTableLinksToCSV() { - const csv = []; - const root = document.querySelector(this.divName); - const rows = root.querySelectorAll("table tr"); - - for (let i = 0; i < rows.length; i++) { - const row = []; - const cols = rows[i].querySelectorAll("td, th"); - - // tslint:disable-next-line - for (let j = 0; j < cols.length; j++) { - if (i === 0) { - let text = (cols[j] as HTMLTableCellElement).innerText; - text = text.replace(" ▼", ""); - text = text.replace(" ▲", ""); - row.push(text); - } else { - const col = cols[j] as HTMLElement; - - // this is super brittle - if (col.children.length > 0 && col.children[0] instanceof HTMLAnchorElement) { - row.push((col.children[0] as HTMLAnchorElement).href); - } else { - row.push(col.innerText); - } - } - } - csv.push(row.join(",")); - } - - return csv.join("\n"); - } + // no longer used + // private exportTableLinksToCSV() { + // const csv = []; + // const root = document.querySelector(this.divName); + // const rows = root.querySelectorAll("table tr"); + // + // for (let i = 0; i < rows.length; i++) { + // const row = []; + // const cols = rows[i].querySelectorAll("td, th"); + // + // // tslint:disable-next-line + // for (let j = 0; j < cols.length; j++) { + // if (i === 0) { + // let text = (cols[j] as HTMLTableCellElement).innerText; + // text = text.replace(" ▼", ""); + // text = text.replace(" ▲", ""); + // row.push(text); + // } else { + // const col = cols[j] as HTMLElement; + // + // // this is super brittle + // if (col.children.length > 0 && col.children[0] instanceof HTMLAnchorElement) { + // row.push((col.children[0] as HTMLAnchorElement).href); + // } else { + // row.push(col.innerText); + // } + // } + // } + // csv.push(row.join(",")); + // } + // + // return csv.join("\n"); + // } public numRows(): number { return this.rows.length; @@ -397,8 +398,9 @@ export class SortableTable { private attachDownload() { const csv = this.exportTableToCSV(); this.downloadCSV(csv, "classy.csv", "Download Values as CSV "); - const links = this.exportTableLinksToCSV(); - this.downloadCSV(links, "classyLinks.csv", " Download Links as CSV"); + // no longer needed; regular csv now includes these + // const links = this.exportTableLinksToCSV(); + // this.downloadCSV(links, "classyLinks.csv", " Download Links as CSV"); } /** From e64831533de4a578f091467a27b3b65d609db1fe Mon Sep 17 00:00:00 2001 From: reid holmes Date: Tue, 30 Apr 2024 13:27:57 -0700 Subject: [PATCH 13/80] offsets --- packages/portal/frontend/src/app/util/SortableTable.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/portal/frontend/src/app/util/SortableTable.ts b/packages/portal/frontend/src/app/util/SortableTable.ts index 5cd3713f8..870c062be 100644 --- a/packages/portal/frontend/src/app/util/SortableTable.ts +++ b/packages/portal/frontend/src/app/util/SortableTable.ts @@ -344,7 +344,9 @@ export class SortableTable { if (colsWithMetadata.indexOf(j) >= 0) { if (i === 0) { // header row - row.push(cols[j] + "_metadata"); // add metadata prior column name + // add metadata prior column name + // strange math because we may have added columns to the left + row.push(row[j + colsWithMetadata.indexOf(j)] + "_metadata"); } else { // regular row row.push(this.extractMetadata(cols[j] as HTMLElement)); From 13df441369b307c47446bdc93955e52c66dd5f8d Mon Sep 17 00:00:00 2001 From: reid holmes Date: Tue, 30 Apr 2024 13:52:52 -0700 Subject: [PATCH 14/80] sort colsWithMetadata --- packages/portal/frontend/src/app/util/SortableTable.ts | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/packages/portal/frontend/src/app/util/SortableTable.ts b/packages/portal/frontend/src/app/util/SortableTable.ts index 870c062be..8c9d8b84e 100644 --- a/packages/portal/frontend/src/app/util/SortableTable.ts +++ b/packages/portal/frontend/src/app/util/SortableTable.ts @@ -273,10 +273,10 @@ export class SortableTable { private findColsWithMetadata(divName: string): number[] { const root = document.querySelector(this.divName); const rows = root.querySelectorAll("table tr"); - const colsWithMetadata: number[] = []; + let colsWithMetadata: number[] = []; // tslint:disable-next-line - for (let i = 0; i < rows.length; i++) { + for (let i = 1; i < rows.length; i++) { // skip the header row const cols = rows[i].querySelectorAll("td, th"); // tslint:disable-next-line @@ -293,6 +293,9 @@ export class SortableTable { } } + // sort numbers ascending + colsWithMetadata = colsWithMetadata.sort((a, b) => a - b); + Log.info("SortableTable::findColsWithMetadata() - cols: " + JSON.stringify(colsWithMetadata)); return colsWithMetadata; } From 036b4449a1ba3cd676e57dd3232e4fbd5554c9f9 Mon Sep 17 00:00:00 2001 From: reid holmes Date: Mon, 17 Jun 2024 08:42:42 -0700 Subject: [PATCH 15/80] update comment --- packages/portal/frontend/src/app/util/SortableTable.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/portal/frontend/src/app/util/SortableTable.ts b/packages/portal/frontend/src/app/util/SortableTable.ts index 8c9d8b84e..0f493faef 100644 --- a/packages/portal/frontend/src/app/util/SortableTable.ts +++ b/packages/portal/frontend/src/app/util/SortableTable.ts @@ -293,7 +293,7 @@ export class SortableTable { } } - // sort numbers ascending + // sort metadata columns colsWithMetadata = colsWithMetadata.sort((a, b) => a - b); Log.info("SortableTable::findColsWithMetadata() - cols: " + JSON.stringify(colsWithMetadata)); From 4713244d4e8cc279cc4ebf89af6599109431b130 Mon Sep 17 00:00:00 2001 From: Reid Holmes Date: Fri, 6 Sep 2024 07:38:29 -0700 Subject: [PATCH 16/80] Improving template provisioning; all unnecessary branches not being removed --- .../backend/src/controllers/GitHubActions.ts | 54 +++++++++++++++---- .../src/controllers/GitHubController.ts | 3 ++ 2 files changed, 47 insertions(+), 10 deletions(-) diff --git a/packages/portal/backend/src/controllers/GitHubActions.ts b/packages/portal/backend/src/controllers/GitHubActions.ts index fef0374d6..a145f2bb1 100644 --- a/packages/portal/backend/src/controllers/GitHubActions.ts +++ b/packages/portal/backend/src/controllers/GitHubActions.ts @@ -1362,19 +1362,19 @@ export class GitHubActions implements IGitHubActions { return branches; } - public async deleteBranches(repoId: string, branchesToKeep: string[]): Promise { + public async listBranches(repoId: string): Promise { const start = Date.now(); const repoExists = await this.repoExists(repoId); // ensure the repo exists if (repoExists === false) { - Log.error("GitHubAction::deleteBranches(..) - failed; repo does not exist"); - return false; + Log.error("GitHubAction::listBranches(..) - failed; repo does not exist"); + return []; } // get branches // GET /repos/{owner}/{repo}/branches const listUri = this.apiPath + "/repos/" + this.org + "/" + repoId + "/branches"; - Log.info("GitHubAction::deleteBranches(..) - list branch uri: " + listUri); + Log.info("GitHubAction::listBranches(..) - list branch uri: " + listUri); const listOptions: RequestInit = { method: "GET", headers: { @@ -1386,25 +1386,45 @@ export class GitHubActions implements IGitHubActions { }; const listResp = await fetch(listUri, listOptions); - Log.trace("GitHubAction::deleteBranches(..) - list response code: " + listResp.status); // 201 success + Log.trace("GitHubAction::listBranches(..) - list response code: " + listResp.status); // 201 success const listRespBody = await listResp.json(); if (listResp.status !== 200) { Log.warn("GitHubAction::deleteBranches(..) - failed to list branches for repo; response: " + JSON.stringify(listRespBody)); + return []; + } + + Log.trace("GitHubAction::listBranches(..) - branch list: " + JSON.stringify(listRespBody)); + + const branches: string[] = []; + for (const githubBranch of listRespBody) { + branches.push(githubBranch.name); + } + Log.info("GitHubAction::listBranches(..) - done; branches found: " + JSON.stringify(branches)); + return branches; + } + + public async deleteBranches(repoId: string, branchesToKeep: string[]): Promise { + const start = Date.now(); + + const repoExists = await this.repoExists(repoId); // ensure the repo exists + if (repoExists === false) { + Log.error("GitHubAction::deleteBranches(..) - failed; repo does not exist"); return false; } - Log.trace("GitHubAction::deleteBranches(..) - branch list: " + JSON.stringify(listRespBody)); + const allBranches = await this.listBranches(repoId); const branchesToKeepThatExist: string[] = []; const branchesToDelete: string[] = []; - for (const githubBranch of listRespBody) { - if (branchesToKeep.indexOf(githubBranch.name) < 0) { - branchesToDelete.push(githubBranch.name); + for (const githubBranch of allBranches) { + if (branchesToKeep.indexOf(githubBranch) < 0) { + branchesToDelete.push(githubBranch); } else { - branchesToKeepThatExist.push(githubBranch.name); + branchesToKeepThatExist.push(githubBranch); } } + Log.info("GitHubAction::deleteBranches(..) - branches to delete: " + JSON.stringify(branchesToDelete)); // make sure there will be at least one branch left on the repo @@ -1443,6 +1463,20 @@ export class GitHubActions implements IGitHubActions { } } + // This is an unsatisfying check. But GitHub Enterprise often returns repo provisioning + // before it is actually complete. This means that all of the branches may not be found + // at the time this method runs the first time. Hopefully after some deletions enough time + // has passed that this will work correctly. An alternative would have been to put a wait + // into the repo provisioning workflow, but the whole reason to change to templates was for + // performance. Hopefully this is good enough. + + const branchesAfter = await this.listBranches(repoId); + if (branchesAfter.length > branchesToKeep.length) { + // do it again + Log.info("GitHubAction::deleteBranches(..) - branches still remain; retry removal"); + await this.deleteBranches(repoId, branchesToKeep); + } + Log.info("GitHubAction::deleteBranches(..) - done; success: " + deleteSucceeded + "; took: " + Util.took(start)); return deleteSucceeded; } diff --git a/packages/portal/backend/src/controllers/GitHubController.ts b/packages/portal/backend/src/controllers/GitHubController.ts index e226c082e..2f906302b 100644 --- a/packages/portal/backend/src/controllers/GitHubController.ts +++ b/packages/portal/backend/src/controllers/GitHubController.ts @@ -429,7 +429,10 @@ export class GitHubController implements IGitHubController { const branchRenameSuccess = await this.gha.renameBranch(repoName, importBranchesToKeep[0], "main"); Log.info("GitHubController::provisionRepository( " + repoName + " ) - branch rename success: " + branchRenameSuccess); } + } else { + Log.info("GitHubController::provisionRepository( " + repoName + " ) - no branch specified; all branches kept"); } + Log.info("GitHubController::provisionRepository( " + repoName + " ) - GitHub repo created"); // we consider the repo to be provisioned once the whole flow is done From b96f9413590aec361a0942f529d25d765b052199 Mon Sep 17 00:00:00 2001 From: Reid Holmes Date: Fri, 6 Sep 2024 07:43:37 -0700 Subject: [PATCH 17/80] lint --- packages/portal/backend/src/controllers/GitHubActions.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/portal/backend/src/controllers/GitHubActions.ts b/packages/portal/backend/src/controllers/GitHubActions.ts index a145f2bb1..10c4f2d9c 100644 --- a/packages/portal/backend/src/controllers/GitHubActions.ts +++ b/packages/portal/backend/src/controllers/GitHubActions.ts @@ -1469,7 +1469,7 @@ export class GitHubActions implements IGitHubActions { // has passed that this will work correctly. An alternative would have been to put a wait // into the repo provisioning workflow, but the whole reason to change to templates was for // performance. Hopefully this is good enough. - + const branchesAfter = await this.listBranches(repoId); if (branchesAfter.length > branchesToKeep.length) { // do it again From 64479b2aef0ab0927415c0b9e59182b03d451ec8 Mon Sep 17 00:00:00 2001 From: Reid Holmes Date: Fri, 6 Sep 2024 10:09:19 -0700 Subject: [PATCH 18/80] more verbose logging of team creation during provisioning --- .../backend/src/controllers/GitHubActions.ts | 9 ++++++--- .../src/controllers/GitHubController.ts | 18 +++++++++--------- 2 files changed, 15 insertions(+), 12 deletions(-) diff --git a/packages/portal/backend/src/controllers/GitHubActions.ts b/packages/portal/backend/src/controllers/GitHubActions.ts index 10c4f2d9c..dfce80ed5 100644 --- a/packages/portal/backend/src/controllers/GitHubActions.ts +++ b/packages/portal/backend/src/controllers/GitHubActions.ts @@ -1374,7 +1374,7 @@ export class GitHubActions implements IGitHubActions { // get branches // GET /repos/{owner}/{repo}/branches const listUri = this.apiPath + "/repos/" + this.org + "/" + repoId + "/branches"; - Log.info("GitHubAction::listBranches(..) - list branch uri: " + listUri); + Log.info("GitHubAction::listBranches(..) - starting; branch uri: " + listUri); const listOptions: RequestInit = { method: "GET", headers: { @@ -1393,13 +1393,13 @@ export class GitHubActions implements IGitHubActions { Log.warn("GitHubAction::deleteBranches(..) - failed to list branches for repo; response: " + JSON.stringify(listRespBody)); return []; } - Log.trace("GitHubAction::listBranches(..) - branch list: " + JSON.stringify(listRespBody)); const branches: string[] = []; for (const githubBranch of listRespBody) { branches.push(githubBranch.name); } + Log.info("GitHubAction::listBranches(..) - done; branches found: " + JSON.stringify(branches)); return branches; } @@ -1439,7 +1439,7 @@ export class GitHubActions implements IGitHubActions { for (const branch of branchesToDelete) { // DELETE /repos/{owner}/{repo}/git/refs/{ref} const delUri = this.apiPath + "/repos/" + this.org + "/" + repoId + "/git/refs/" + "heads/" + branch; - Log.info("GitHubAction::deleteBranches(..) - delete branch uri: " + delUri); + Log.info("GitHubAction::deleteBranches(..) - delete branch; uri: " + delUri); const delOptions: RequestInit = { method: "DELETE", @@ -1470,11 +1470,14 @@ export class GitHubActions implements IGitHubActions { // into the repo provisioning workflow, but the whole reason to change to templates was for // performance. Hopefully this is good enough. + Log.info("GitHubAction::deleteBranches(..) - verifying remaining branches"); const branchesAfter = await this.listBranches(repoId); if (branchesAfter.length > branchesToKeep.length) { // do it again Log.info("GitHubAction::deleteBranches(..) - branches still remain; retry removal"); await this.deleteBranches(repoId, branchesToKeep); + } else { + Log.info("GitHubAction::deleteBranches(..) - extra branches not found"); } Log.info("GitHubAction::deleteBranches(..) - done; success: " + deleteSucceeded + "; took: " + Util.took(start)); diff --git a/packages/portal/backend/src/controllers/GitHubController.ts b/packages/portal/backend/src/controllers/GitHubController.ts index 2f906302b..27c3fcecd 100644 --- a/packages/portal/backend/src/controllers/GitHubController.ts +++ b/packages/portal/backend/src/controllers/GitHubController.ts @@ -457,16 +457,20 @@ export class GitHubController implements IGitHubController { try { let teamValue = null; try { - Log.info("GitHubController::provisionRepository() - create GitHub team"); + Log.info("GitHubController::provisionRepository() - create GitHub team(s): " + JSON.stringify(teams)); for (const team of teams) { + Log.trace("GitHubController::provisionRepository() - team: " + JSON.stringify(team)); const dbT = await dbc.getTeam(team.id); if (dbT === null) { throw new Error("GitHubController::provisionRepository( " + repoName + " ) - " + "team does not exist in datastore (but should): " + team.id); } + Log.trace("GitHubController::provisionRepository() - dbT: " + JSON.stringify(dbT)); - if (team.URL !== null && await tc.getTeamNumber(team.id) !== null) { + const teamNum = await tc.getTeamNumber(team.id); + Log.trace("GitHubController::provisionRepository() - dbT team Number: " + teamNum); + if (team.URL !== null && teamNum !== null) { // already exists Log.warn("GitHubController::provisionRepository( " + repoName + " ) - team already exists: " + teamValue.teamName + "; assuming team members on github are correct."); @@ -503,21 +507,17 @@ export class GitHubController implements IGitHubController { // swallow these errors and keep going } + // add staff team to repo Log.trace("GitHubController::provisionRepository() - add staff team to repo"); - // const staffTeamNumber = await tc.getTeamNumber(TeamController.STAFF_NAME); - // Log.trace("GitHubController::provisionRepository(..) - staffTeamNumber: " + staffTeamNumber); - // const staffAdd = await this.gha.addTeamToRepo(staffTeamNumber, repoName, "admin"); const staffAdd = await this.gha.addTeamToRepo(TeamController.STAFF_NAME, repoName, "admin"); Log.trace("GitHubController::provisionRepository(..) - team name: " + staffAdd.teamName); + // add admin team to repo Log.trace("GitHubController::provisionRepository() - add admin team to repo"); - // const adminTeamNumber = await tc.getTeamNumber(TeamController.ADMIN_NAME); - // Log.trace("GitHubController::provisionRepository(..) - adminTeamNumber: " + adminTeamNumber); - // const adminAdd = await this.gha.addTeamToRepo(adminTeamNumber, repoName, "admin"); const adminAdd = await this.gha.addTeamToRepo(TeamController.ADMIN_NAME, repoName, "admin"); Log.trace("GitHubController::provisionRepository(..) - team name: " + adminAdd.teamName); - // add webhooks + // add webhooks to repo const host = Config.getInstance().getProp(ConfigKey.publichostname); const WEBHOOKADDR = host + "/portal/githubWebhook"; Log.trace("GitHubController::provisionRepository() - add webhook to: " + WEBHOOKADDR); From a1ee7b4b0d970f0cdd27476cad02a80ca8c867eb Mon Sep 17 00:00:00 2001 From: Reid Holmes Date: Fri, 6 Sep 2024 10:33:47 -0700 Subject: [PATCH 19/80] better repo name logging --- .../src/controllers/GitHubController.ts | 73 ++++++++++--------- 1 file changed, 38 insertions(+), 35 deletions(-) diff --git a/packages/portal/backend/src/controllers/GitHubController.ts b/packages/portal/backend/src/controllers/GitHubController.ts index 27c3fcecd..d6fd1f0f3 100644 --- a/packages/portal/backend/src/controllers/GitHubController.ts +++ b/packages/portal/backend/src/controllers/GitHubController.ts @@ -116,41 +116,41 @@ export class GitHubController implements IGitHubController { // const gh = GitHubActions.getInstance(true); - Log.trace("GitHubController::createRepository(..) - see if repo already exists"); + Log.trace("GitHubController::createRepository( " + repoName + " ) - see if repo already exists on GitHub org"); const repoVal = await this.gha.repoExists(repoName); if (repoVal === true) { // unable to create a repository if it already exists! - Log.error("GitHubController::createRepository(..) - Error: Repository already exists;" + - " unable to create a new repository"); - throw new Error("createRepository(..) failed; Repository " + repoName + " already exists."); + Log.error("GitHubController::createRepository( " + repoName + " ) - Error: Repository already exists " + + "on GitHub; unable to create a new repository"); + throw new Error("createRepository(..) failed; Repository " + repoName + " already exists on GitHub."); } try { // create the repository - Log.trace("GitHubController::createRepository() - create GitHub repo"); + Log.trace("GitHubController::createRepository( " + repoName + " ) - create GitHub repo"); const repoCreateVal = await this.gha.createRepo(repoName); - Log.trace("GitHubController::createRepository(..) - success; repo: " + repoCreateVal); + Log.trace("GitHubController::createRepository( " + repoName + " ) - success; repo: " + repoCreateVal); } catch (err) { /* istanbul ignore next: braces needed for ignore */ { - Log.error("GitHubController::createRepository(..) - create repo error: " + err); + Log.error("GitHubController::createRepository( " + repoName + " ) - create repo error: " + err); // repo creation failed; remove if needed (requires createRepo be permissive if already exists) const res = await this.gha.deleteRepo(repoName); - Log.info("GitHubController::createRepository(..) - repo removed: " + res); + Log.info("GitHubController::createRepository( " + repoName + " ) - repo removed: " + res); throw new Error("createRepository(..) failed; Repository " + repoName + " creation failed; ERROR: " + err.message); } } try { // still add staff team with push, just not students - Log.trace("GitHubController::createRepository() - add staff team to repo"); + Log.trace("GitHubController::createRepository( " + repoName + " ) - add staff team to repo"); // const staffTeamNumber = await this.tc.getTeamNumber(TeamController.STAFF_NAME); // Log.trace("GitHubController::createRepository(..) - staffTeamNumber: " + staffTeamNumber); // const staffAdd = await this.gha.addTeamToRepo(staffTeamNumber, repoName, "admin"); const staffAdd = await this.gha.addTeamToRepo(TeamController.STAFF_NAME, repoName, "admin"); Log.trace("GitHubController::createRepository(..) - team name: " + staffAdd.teamName); - Log.trace("GitHubController::createRepository() - add admin team to repo"); + Log.trace("GitHubController::createRepository( " + repoName + " ) - add admin team to repo"); // const adminTeamNumber = await this.tc.getTeamNumber(TeamController.ADMIN_NAME); // Log.trace("GitHubController::createRepository(..) - adminTeamNumber: " + adminTeamNumber); // const adminAdd = await this.gha.addTeamToRepo(adminTeamNumber, repoName, "admin"); @@ -158,15 +158,15 @@ export class GitHubController implements IGitHubController { Log.trace("GitHubController::createRepository(..) - team name: " + adminAdd.teamName); // add webhooks - Log.trace("GitHubController::createRepository() - add webhook"); + Log.trace("GitHubController::createRepository( " + repoName + " ) - add webhook"); const createHook = await this.gha.addWebhook(repoName, WEBHOOKADDR); - Log.trace("GitHubController::createRepository(..) - webook successful: " + createHook); + Log.trace("GitHubController::createRepository(..) - webhook successful: " + createHook); // perform import const c = Config.getInstance(); const targetUrl = c.getProp(ConfigKey.githubHost) + "/" + c.getProp(ConfigKey.org) + "/" + repoName; - Log.trace("GitHubController::createRepository() - importing project (slow)"); + Log.trace("GitHubController::createRepository( " + repoName + " ) - importing project (slow)"); let output; /* istanbul ignore if */ if (typeof path !== "undefined") { @@ -174,14 +174,14 @@ export class GitHubController implements IGitHubController { } else { output = await this.gha.importRepoFS(importUrl, targetUrl); } - Log.trace("GitHubController::createRepository(..) - import complete; success: " + output); + Log.trace("GitHubController::createRepository( " + repoName + " ) - import complete; success: " + output); - Log.trace("GithubController::createRepository(..) - successfully completed for: " + - repoName + "; took: " + Util.took(startTime)); + Log.trace("GithubController::createRepository( " + repoName + " ) - successfully completed; " + + "took: " + Util.took(startTime)); return true; } catch (err) { - Log.error("GithubController::createRepository(..) - ERROR: " + err); + Log.error("GithubController::createRepository( " + repoName + " ) - ERROR: " + err); return false; } } @@ -216,47 +216,49 @@ export class GitHubController implements IGitHubController { // const gh = GitHubActions.getInstance(true); - Log.trace("GitHubController::createRepositoryFromTemplate(..) - see if repo already exists"); + Log.trace("GitHubController::createRepositoryFromTemplate( " + repoName + " ) - see if repo already exists"); const repoVal = await this.gha.repoExists(repoName); if (repoVal === true) { // unable to create a repository if it already exists! - Log.error("GitHubController::createRepositoryFromTemplate(..) - Error: Repository already exists;" + - " unable to create a new repository"); - throw new Error("createRepositoryFromTemplate(..) failed; Repository " + repoName + " already exists."); + Log.error("GitHubController::createRepositoryFromTemplate( " + repoName + " ) - Error: " + + "Repository already exists; unable to create a new repository"); + throw new Error("createRepositoryFromTemplate( " + repoName + " ) failed; " + + "Repository " + repoName + " already exists."); } try { // create the repository - Log.trace("GitHubController::createRepositoryFromTemplate() - create GitHub repo"); + Log.trace("GitHubController::createRepositoryFromTemplate( " + repoName + " ) - create GitHub repo"); const repoCreateVal = await this.gha.createRepo(repoName); - Log.trace("GitHubController::createRepositoryFromTemplate(..) - success; repo: " + repoCreateVal); + Log.trace("GitHubController::createRepositoryFromTemplate( " + repoName + " ) - success; " + + "repo: " + repoCreateVal); } catch (err) { /* istanbul ignore next: braces needed for ignore */ { - Log.error("GitHubController::createRepositoryFromTemplate(..) - create repo error: " + err); + Log.error("GitHubController::createRepositoryFromTemplate( " + repoName + " ) - create repo error: " + err); // repo creation failed; remove if needed (requires createRepo be permissive if already exists) const res = await this.gha.deleteRepo(repoName); - Log.info("GitHubController::createRepositoryFromTemplate(..) - repo removed: " + res); - throw new Error("createRepository(..) failed; Repository " + repoName + " creation failed; ERROR: " + err.message); + Log.info("GitHubController::createRepositoryFromTemplate( " + repoName + " ) - repo removed: " + res); + throw new Error("createRepository( " + repoName + " ) creation failed; ERROR: " + err.message); } } if (branchesToKeep.length > 0) { // TODO: remove any branches we do not need } else { - Log.info("GitHubController::createRepositoryFromTemplate(..) - all branches included"); + Log.info("GitHubController::createRepositoryFromTemplate( " + repoName + " ) - all branches included"); } try { // still add staff team with push, just not students - Log.trace("GitHubController::createRepositoryFromTemplate() - add staff team to repo"); + Log.trace("GitHubController::createRepositoryFromTemplate( " + repoName + " ) - add staff team to repo"); // const staffTeamNumber = await this.tc.getTeamNumber(TeamController.STAFF_NAME); // Log.trace("GitHubController::createRepository(..) - staffTeamNumber: " + staffTeamNumber); // const staffAdd = await this.gha.addTeamToRepo(staffTeamNumber, repoName, "admin"); const staffAdd = await this.gha.addTeamToRepo(TeamController.STAFF_NAME, repoName, "admin"); Log.trace("GitHubController::createRepositoryFromTemplate(..) - team name: " + staffAdd.teamName); - Log.trace("GitHubController::createRepositoryFromTemplate() - add admin team to repo"); + Log.trace("GitHubController::createRepositoryFromTemplate( " + repoName + " ) - add admin team to repo"); // const adminTeamNumber = await this.tc.getTeamNumber(TeamController.ADMIN_NAME); // Log.trace("GitHubController::createRepository(..) - adminTeamNumber: " + adminTeamNumber); // const adminAdd = await this.gha.addTeamToRepo(adminTeamNumber, repoName, "admin"); @@ -264,7 +266,7 @@ export class GitHubController implements IGitHubController { Log.trace("GitHubController::createRepositoryFromTemplate(..) - team name: " + adminAdd.teamName); // add webhooks - Log.trace("GitHubController::createRepositoryFromTemplate() - add webhook"); + Log.trace("GitHubController::createRepositoryFromTemplate( " + repoName + " ) - add webhook"); const createHook = await this.gha.addWebhook(repoName, WEBHOOKADDR); Log.trace("GitHubController::createRepositoryFromTemplate(..) - webook successful: " + createHook); @@ -272,16 +274,17 @@ export class GitHubController implements IGitHubController { const c = Config.getInstance(); const targetUrl = c.getProp(ConfigKey.githubHost) + "/" + c.getProp(ConfigKey.org) + "/" + repoName; - Log.trace("GitHubController::createRepositoryFromTemplate() - importing project (slow)"); + Log.trace("GitHubController::createRepositoryFromTemplate( " + repoName + " ) - importing project (slow)"); const output = await this.gha.importRepoFS(importUrl, targetUrl); - Log.trace("GitHubController::createRepositoryFromTemplate(..) - import complete; success: " + output); + Log.trace("GitHubController::createRepositoryFromTemplate( " + repoName + " ) - import complete; " + + "success: " + output); - Log.trace("GithubController::createRepositoryFromTemplate(..) - successfully completed for: " + - repoName + "; took: " + Util.took(startTime)); + Log.trace("GithubController::createRepositoryFromTemplate( " + repoName + " ) - successfully completed; " + + "took: " + Util.took(startTime)); return true; } catch (err) { - Log.error("GithubController::createRepositoryFromTemplate(..) - ERROR: " + err); + Log.error("GithubController::createRepositoryFromTemplate( " + repoName + " ) - ERROR: " + err); return false; } } From e3c2b733e767d0f2ed113a8b4f8da57e5cc076c8 Mon Sep 17 00:00:00 2001 From: Reid Holmes Date: Mon, 9 Sep 2024 07:13:54 -0700 Subject: [PATCH 20/80] encode branch name in default grader image for 310 --- plugins/default/portal/frontend/html/admin.html | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugins/default/portal/frontend/html/admin.html b/plugins/default/portal/frontend/html/admin.html index 3b5193b01..9fdffe048 100644 --- a/plugins/default/portal/frontend/html/admin.html +++ b/plugins/default/portal/frontend/html/admin.html @@ -1155,7 +1155,7 @@
From e5ddf8570a89c20d8a3081476edef5813d7f4beb Mon Sep 17 00:00:00 2001 From: reid holmes Date: Mon, 9 Sep 2024 11:03:28 -0700 Subject: [PATCH 21/80] add the ability to update template repos (the createRepo endpoint allows modifying many repository features that the templaterepo endpoint does not). --- .../backend/src/controllers/GitHubActions.ts | 76 +++++++++++++++++++ .../src/controllers/GitHubController.ts | 5 ++ .../test/controllers/GitHubActionSpec.ts | 7 +- .../test/controllers/TestGitHubActions.ts | 9 +++ 4 files changed, 96 insertions(+), 1 deletion(-) diff --git a/packages/portal/backend/src/controllers/GitHubActions.ts b/packages/portal/backend/src/controllers/GitHubActions.ts index dfce80ed5..7f6da1903 100644 --- a/packages/portal/backend/src/controllers/GitHubActions.ts +++ b/packages/portal/backend/src/controllers/GitHubActions.ts @@ -43,6 +43,15 @@ export interface IGitHubActions { */ createRepoFromTemplate(repoName: string, templateOwner: string, templateRepo: string): Promise; + /** + * Updates a repo with the settings we want on our repos. This is used for repos created from a template. + * Repos that are created with `createRepo` do not need to use this endpoint (although there is no downside + * to them using it). + * + * @param repoName + */ + updateRepo(repoName: string): Promise; + /** * Deletes a repo from the organization. * @@ -514,6 +523,73 @@ export class GitHubActions implements IGitHubActions { } } + public async updateRepo(repoName: string): Promise { + // These are the settings we want on our repos, but we can't set them on creation when making them with a template + + // name: repoName, + // // In Dev and Test, Github free Org Repos cannot be private. + // private: true, + // has_issues: true, + // has_wiki: false, + // has_downloads: false, + // // squash merging does not use ff causing branch problems in autotest + // allow_squash_merge: false, + // // rebase merging does not use ff causing branch problems in autotest + // allow_rebase_merge: false, + // merge_commit_title: "PR_TITLE", + // merge_commit_message: "PR_BODY", + // auto_init: false + + const start = Date.now(); + try { + Log.info("GitHubAction::updateRepo( " + repoName + " ) - start"); + await GitHubActions.checkDatabase(repoName, null); + + const repoOpts: any = { + name: repoName, + // In Dev and Test, Github free Org Repos cannot be private. + private: true, + has_issues: true, + has_wiki: false, + has_downloads: false, + // squash merging does not use ff causing branch problems in autotest + allow_squash_merge: false, + // rebase merging does not use ff causing branch problems in autotest + allow_rebase_merge: false, + merge_commit_title: "PR_TITLE", + merge_commit_message: "PR_BODY" + }; + + const uri = this.apiPath + "/orgs/" + this.org + "/repos/" + repoName; + const options: RequestInit = { + method: "PATCH", + headers: { + "Authorization": this.gitHubAuthToken, + "User-Agent": this.gitHubUserName, + "Accept": "application/vnd.github+json" + }, + body: JSON.stringify(repoOpts) + }; + + Log.trace("GitHubAction::updateRepo( " + repoName + " ) - making request"); + const response = await fetch(uri, options); + const body = await response.json(); + Log.trace("GitHubAction::updateRepo( " + repoName + " ) - request complete"); + + const url = body.html_url; + const wasSuccess = repoOpts.has_issues === body.has_issues && + repoOpts.has_wiki === body.has_wiki && + repoOpts.has_downloads === body.has_downloads && + repoOpts.allow_squash_merge === body.allow_squash_merge && + repoOpts.allow_rebase_merge === body.allow_rebase_merge; + Log.info("GitHubAction::updateRepo(..) - wasSuccessful: " + wasSuccess + "; URL: " + url + "; took: " + Util.took(start)); + return wasSuccess; + } catch (err) { + Log.error("GitHubAction::updateRepo(..) - ERROR: " + err); + throw new Error("Repository not created; " + err.message); + } + } + /** * Deletes a repo from the organization. * diff --git a/packages/portal/backend/src/controllers/GitHubController.ts b/packages/portal/backend/src/controllers/GitHubController.ts index d6fd1f0f3..66209ff77 100644 --- a/packages/portal/backend/src/controllers/GitHubController.ts +++ b/packages/portal/backend/src/controllers/GitHubController.ts @@ -436,6 +436,11 @@ export class GitHubController implements IGitHubController { Log.info("GitHubController::provisionRepository( " + repoName + " ) - no branch specified; all branches kept"); } + Log.trace("GitHubController::provisionRepository( " + repoName + " ) - updating repo"); + // since we moved to template provisioning, we need to update the repo to make sure the settings are correct + const updateWorked = await this.gha.updateRepo(repoName); + Log.trace("GitHubController::provisionRepository( " + repoName + " ) - repo updated: " + updateWorked); + Log.info("GitHubController::provisionRepository( " + repoName + " ) - GitHub repo created"); // we consider the repo to be provisioned once the whole flow is done diff --git a/packages/portal/backend/test/controllers/GitHubActionSpec.ts b/packages/portal/backend/test/controllers/GitHubActionSpec.ts index 029ccca12..a66235529 100644 --- a/packages/portal/backend/test/controllers/GitHubActionSpec.ts +++ b/packages/portal/backend/test/controllers/GitHubActionSpec.ts @@ -153,7 +153,7 @@ describe("GitHubActions", () => { expect(val).to.equal(name); }).timeout(TIMEOUT); - it("Should be able to create a repo from a template.", async function () { + it("Should be able to create a repo from a template and update it to have the right features.", async function () { const rc = new RepositoryController(); const dc = new DeliverablesController(); const deliv = await dc.getDeliverable(TestHarness.DELIVID0); @@ -162,11 +162,16 @@ describe("GitHubActions", () => { const owner = Config.getInstance().getProp(ConfigKey.org); const repo = TestHarness.REPONAMEREAL_TESTINGSAMPLE; + Log.test("Creating repo with template"); const val = await gh.createRepoFromTemplate(repoName, owner, repo); const name = Config.getInstance().getProp(ConfigKey.githubHost) + "/" + Config.getInstance().getProp(ConfigKey.org) + "/" + repoName; expect(val).to.equal(name); + + Log.test("Updating template repo with proper settings"); + const update = await gh.updateRepo(repoName); + expect(update).to.be.true; }).timeout(TIMEOUT); it("Should be able to rename a branch on a repo.", async function () { diff --git a/packages/portal/backend/test/controllers/TestGitHubActions.ts b/packages/portal/backend/test/controllers/TestGitHubActions.ts index beb275daa..2b17a77df 100644 --- a/packages/portal/backend/test/controllers/TestGitHubActions.ts +++ b/packages/portal/backend/test/controllers/TestGitHubActions.ts @@ -82,6 +82,15 @@ export class TestGitHubActions implements IGitHubActions { return this.repos[repoName]; } + public async updateRepo(repoName: string): Promise { + if (typeof this.repos[repoName] !== "undefined") { + Log.info("TestGitHubActions::updateRepo( " + repoName + " ) - exists"); + return true; + } + Log.info("TestGitHubActions::updateRepo( " + repoName + " ) - does not exist"); + return false; + } + // public async createTeam(teamName: string, permission: string): Promise<{ teamName: string; githubTeamNumber: number; URL: string }> { public async createTeam(teamName: string, permission: string): Promise { // if (typeof this.teams[teamName] === "undefined") { From 1b75da4c9f82358074d0cfc5acec6c0482742fad Mon Sep 17 00:00:00 2001 From: reid holmes Date: Mon, 9 Sep 2024 11:14:12 -0700 Subject: [PATCH 22/80] fix update endpoint --- packages/portal/backend/src/controllers/GitHubActions.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/portal/backend/src/controllers/GitHubActions.ts b/packages/portal/backend/src/controllers/GitHubActions.ts index 7f6da1903..0606bf9ab 100644 --- a/packages/portal/backend/src/controllers/GitHubActions.ts +++ b/packages/portal/backend/src/controllers/GitHubActions.ts @@ -560,7 +560,8 @@ export class GitHubActions implements IGitHubActions { merge_commit_message: "PR_BODY" }; - const uri = this.apiPath + "/orgs/" + this.org + "/repos/" + repoName; + const uri = this.apiPath + "/repos/" + this.org + "/" + repoName; + // const uri = this.apiPath + "/orgs/" + this.org + "/repos/" + repoName; const options: RequestInit = { method: "PATCH", headers: { @@ -575,6 +576,7 @@ export class GitHubActions implements IGitHubActions { const response = await fetch(uri, options); const body = await response.json(); Log.trace("GitHubAction::updateRepo( " + repoName + " ) - request complete"); + Log.info("GitHubAction::updateRepo( " + repoName + " ) - body: " + JSON.stringify(body)); const url = body.html_url; const wasSuccess = repoOpts.has_issues === body.has_issues && From f34237f3133d64e65ed16ade645118890483ca72 Mon Sep 17 00:00:00 2001 From: reid holmes Date: Mon, 9 Sep 2024 11:23:20 -0700 Subject: [PATCH 23/80] update endpoint working, decreasing verbosity --- packages/portal/backend/src/controllers/GitHubActions.ts | 2 -- 1 file changed, 2 deletions(-) diff --git a/packages/portal/backend/src/controllers/GitHubActions.ts b/packages/portal/backend/src/controllers/GitHubActions.ts index 0606bf9ab..956265942 100644 --- a/packages/portal/backend/src/controllers/GitHubActions.ts +++ b/packages/portal/backend/src/controllers/GitHubActions.ts @@ -561,7 +561,6 @@ export class GitHubActions implements IGitHubActions { }; const uri = this.apiPath + "/repos/" + this.org + "/" + repoName; - // const uri = this.apiPath + "/orgs/" + this.org + "/repos/" + repoName; const options: RequestInit = { method: "PATCH", headers: { @@ -576,7 +575,6 @@ export class GitHubActions implements IGitHubActions { const response = await fetch(uri, options); const body = await response.json(); Log.trace("GitHubAction::updateRepo( " + repoName + " ) - request complete"); - Log.info("GitHubAction::updateRepo( " + repoName + " ) - body: " + JSON.stringify(body)); const url = body.html_url; const wasSuccess = repoOpts.has_issues === body.has_issues && From a2c4c57bbe2fafdb40d8fa9927fa4746579448ce Mon Sep 17 00:00:00 2001 From: reid holmes Date: Fri, 13 Sep 2024 16:25:22 -0700 Subject: [PATCH 24/80] see #112 add ability to display bucket grades to students instead of an actual score --- packages/portal/backend/src/Types.ts | 12 +---------- .../src/controllers/CourseController.ts | 20 +++++++++++++++++++ .../src/controllers/GradesController.ts | 15 +++++++++++++- .../src/controllers/ResultsController.ts | 2 +- .../src/app/views/AbstractStudentView.ts | 20 ++++++++++++------- 5 files changed, 49 insertions(+), 20 deletions(-) diff --git a/packages/portal/backend/src/Types.ts b/packages/portal/backend/src/Types.ts index 000634891..316901a73 100644 --- a/packages/portal/backend/src/Types.ts +++ b/packages/portal/backend/src/Types.ts @@ -227,18 +227,8 @@ export interface Grade { urlName: string | null; // name associated with URL (e.g., project name) URL: string | null; // link to commit, if appropriate or repoUrl if not + // bucket grading can use this to store the bucket name custom: any; // {}; not used by the default implementation, but useful for extension (e.g., custom grade values) - /* - custom: { // rather than having custom be .any, this allows courses to make sure they do not clash on their .custom parameters - sdmmStatus?: boolean - - // questions?: any, // AssignmentController // TODO: make into assignment.questions - // assignmentID?: any, // AssignmentController // TODO: make into assignment.id - // studentID?: any, // AssignmentController // TODO: make into assignment.personId - // released?: any, // AssignmentController // TODO: make into assignment.released - assignmentGrade?: AssignmentGrade - }; - */ } export interface Result extends AutoTestResult { // TODO: define this without this extends. This import is no good! diff --git a/packages/portal/backend/src/controllers/CourseController.ts b/packages/portal/backend/src/controllers/CourseController.ts index bb6412a27..65c15cbcb 100644 --- a/packages/portal/backend/src/controllers/CourseController.ts +++ b/packages/portal/backend/src/controllers/CourseController.ts @@ -79,6 +79,16 @@ export interface ICourseController { accepted: boolean, message: string } | null>; + + /** + * Allow a course to specialize how a grade should be presented + * to the students. This is especially useful when a numeric score + * is being replaced by a bucket grade. + * + * The frontend will render gradeTransport.custom.displayScore + * if it is set. + */ + convertGrade(grade: Grade): Promise; } /** @@ -225,6 +235,16 @@ export class CourseController implements ICourseController { return null; } + /** + * By default, nothing is needed here. + * + * @param grade + */ + public async convertGrade(grade: Grade): Promise { + Log.info(`CourseController::convertGrade(${grade}) - Default impl; returning original grade`); + return grade; + } + // NOTE: the default implementation is currently broken; do not use it. /** * This is a method that subtypes can call from computeNames if they do not want to implement it themselves. diff --git a/packages/portal/backend/src/controllers/GradesController.ts b/packages/portal/backend/src/controllers/GradesController.ts index 74baedab1..c7e402461 100644 --- a/packages/portal/backend/src/controllers/GradesController.ts +++ b/packages/portal/backend/src/controllers/GradesController.ts @@ -4,11 +4,15 @@ import Log from "@common/Log"; import {AutoTestGradeTransport, GradeTransport} from "@common/types/PortalTypes"; import {GradePayload} from "@common/types/SDMMTypes"; import Util from "@common/Util"; + import {Grade, PersonKind} from "../Types"; +import {Factory} from "../Factory"; import {DatabaseController} from "./DatabaseController"; import {DeliverablesController} from "./DeliverablesController"; import {PersonController} from "./PersonController"; +import {GitHubActions} from "./GitHubActions"; +import {GitHubController} from "./GitHubController"; export class GradesController { @@ -80,7 +84,16 @@ export class GradesController { if (grade.score !== null && grade.score < 0) { grade.score = null; // null if not set (not -1) } - grades.push(grade); + + // convert grade if needed + const gha = GitHubActions.getInstance(true); + const ghc = new GitHubController(gha); + const cc = await Factory.getCourseController(ghc); + + // allow class instances to specialize how the grades are converted for display to students + const convertedGrade = await cc.convertGrade(grade); + + grades.push(convertedGrade); } else { Log.trace("GradesController::getReleasedGradesForPerson( " + personId + " ) - closed deliv: " + deliv.id); } diff --git a/packages/portal/backend/src/controllers/ResultsController.ts b/packages/portal/backend/src/controllers/ResultsController.ts index 63e8edc49..6f47f3f2d 100644 --- a/packages/portal/backend/src/controllers/ResultsController.ts +++ b/packages/portal/backend/src/controllers/ResultsController.ts @@ -73,7 +73,7 @@ export class ResultsController { public async createResult(record: AutoTestResult): Promise { Log.info("ResultsController::createResult(..) - start; deliv: " + record.delivId + "; repo: " + record.repoId + "; SHA: " + Util.shaHuman(record.commitSHA)); - Log.trace("GradesController::createResult(..) - payload: " + JSON.stringify(record)); + Log.trace("ResultsController::createResult(..) - payload: " + JSON.stringify(record)); const start = Date.now(); const rc = new RepositoryController(); diff --git a/packages/portal/frontend/src/app/views/AbstractStudentView.ts b/packages/portal/frontend/src/app/views/AbstractStudentView.ts index 99b087638..c9dbaac06 100644 --- a/packages/portal/frontend/src/app/views/AbstractStudentView.ts +++ b/packages/portal/frontend/src/app/views/AbstractStudentView.ts @@ -136,16 +136,22 @@ export abstract class AbstractStudentView implements IView { const st = new SortableTable(headers, "#studentGradeTable"); for (const grade of this.grades) { + let scoreToDisplay: number | string = grade.score; // default behaviour + + // allow for custom display scores + if (grade?.custom?.displayScore) { + Log.info("AbstractStudentView::renderGrades() - using custom display score: " + grade.custom.displayScore); + scoreToDisplay = grade.custom.displayScore; + } - let score: number | string = grade.score; let scoreHTML = ""; - if (score === null) { - score = "Not Set"; - scoreHTML = score; // no link if the score is not set + if (scoreToDisplay === null) { + scoreToDisplay = "Not Set"; + scoreHTML = scoreToDisplay; // no link if the scoreToDisplay is not set } else if (grade.URL === null) { - scoreHTML = String(score); // no link if the link is not set + scoreHTML = String(scoreToDisplay); // no link if the link is not set } else { - scoreHTML = "" + score + ""; + scoreHTML = "" + scoreToDisplay + ""; } let comment = grade.comment; if (comment === null) { @@ -153,7 +159,7 @@ export abstract class AbstractStudentView implements IView { } const row: TableCell[] = [ {value: grade.delivId, html: grade.delivId}, - {value: score, html: scoreHTML}, + {value: scoreToDisplay, html: scoreHTML}, {value: comment, html: comment} ]; st.addRow(row); From a1c5b166c501709acc81357cd79d6394c38673ad Mon Sep 17 00:00:00 2001 From: Reid Holmes Date: Sun, 15 Sep 2024 13:53:19 -0700 Subject: [PATCH 25/80] logging --- packages/autotest/src/autotest/AutoTest.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/autotest/src/autotest/AutoTest.ts b/packages/autotest/src/autotest/AutoTest.ts index 03464bd15..b3e8b1569 100644 --- a/packages/autotest/src/autotest/AutoTest.ts +++ b/packages/autotest/src/autotest/AutoTest.ts @@ -350,7 +350,8 @@ export abstract class AutoTest implements IAutoTest { const totalJobsRunning = that.jobs.length; Log.info("AutoTest::tick::tickQueue(..) [JOB] - job start: " + queue.getName() + "; deliv: " + (info.target.delivId + ";").padEnd(8, " ") + " repo: " + (info.target.repoId + ";").padEnd(18, " ") + " SHA: " + - Util.shaHuman(info.target.commitSHA) + "; # running: " + totalJobsRunning + "; # queued: " + totalNumQueued + + Util.shaHuman(info.target.commitSHA) + "; # running: " + totalJobsRunning + + "; # queued: " + (totalNumQueued + "").padStart(4, " ") + " ( e: " + that.expressQueue.length() + ", s: " + that.standardQueue.length() + ", l: " + that.lowQueue.length() + " )"); let gradingJob: GradingJob; From bef24eb0dd5289ae4ec8f58e5b2e11fc4ea5c452 Mon Sep 17 00:00:00 2001 From: Reid Holmes Date: Mon, 16 Sep 2024 16:22:15 -0700 Subject: [PATCH 26/80] better logging so we know what's happening with the custom course controller loading --- packages/portal/backend/src/Factory.ts | 6 ++++-- .../portal/backend/src/server/BackendServer.ts | 15 ++++++++++++++- 2 files changed, 18 insertions(+), 3 deletions(-) diff --git a/packages/portal/backend/src/Factory.ts b/packages/portal/backend/src/Factory.ts index 928f7cf3b..3f44484bf 100644 --- a/packages/portal/backend/src/Factory.ts +++ b/packages/portal/backend/src/Factory.ts @@ -14,7 +14,7 @@ export class Factory { * * Set to true if you want to run these slow tests locally (they will always run on CI): */ - // public static OVERRIDE = true; // NOTE: should be commented out when committing + // public static OVERRIDE = true; // NOTE: should be commented out when committing public static OVERRIDE = false; // NOTE: should NOT be commented out when committing private static readonly TESTNAME = "classytest"; @@ -92,7 +92,9 @@ export class Factory { // If a course wants to specialize the AdminView it should be in the file below. // This is not required. But if it is added, it should never be pushed back to "classy/main" Log.trace("Factory::getCourseController() - name: " + name + " - plug: CustomCourseController"); - plug = await require("../../../../plugins/" + plugin + "/portal/backend/CustomCourseController"); + const controllerPath = "../../../../plugins/" + plugin + "/portal/backend/CustomCourseController"; + Log.info("Factory::getCourseController() - full path: " + controllerPath); + plug = await require(controllerPath); } catch (err) { const msg = "Factory::getCourseController() - " + plugin + "/src/custom/CustomCourseController.ts must be defined"; Log.error(msg); diff --git a/packages/portal/backend/src/server/BackendServer.ts b/packages/portal/backend/src/server/BackendServer.ts index b6fd61caf..fbbea9186 100644 --- a/packages/portal/backend/src/server/BackendServer.ts +++ b/packages/portal/backend/src/server/BackendServer.ts @@ -12,6 +12,8 @@ import AdminRoutes from "./common/AdminRoutes"; import {AuthRoutes} from "./common/AuthRoutes"; import {AutoTestRoutes} from "./common/AutoTestRoutes"; import GeneralRoutes from "./common/GeneralRoutes"; +import {GitHubController} from "@backend/controllers/GitHubController"; +import {GitHubActions} from "@backend/controllers/GitHubActions"; /** * This configures the REST endpoints for the server. @@ -111,8 +113,19 @@ export default class BackendServer { Log.info("BackendServer::start() - Registering common handlers; done"); // Register custom route handler for specific classy instance - Log.info("BackendServer::start() - Registering custom handler"); + Log.info("BackendServer::start() - Registering custom handlers"); + + Log.info("BackendServer::start() - Loading custom course controller"); + // We do not need a Custom Course Controller here, but this is a good place + // to make sure that the CustomCourseController loads up as expected + // alongside the CustomRouteHandler. + Factory.getCourseController(new GitHubController(GitHubActions.getInstance())).then(function (cc) { + Log.info("BackendServer::start() - CustomCourseController loaded"); + }).catch(function (err) { + Log.error("BackendServer::start() - Unable to load CustomCourseController: " + err); + }); + Log.info("BackendServer::start() - Loading custom route handler"); Factory.getCustomRouteHandler().then(function (handler) { handler.registerRoutes(that.rest); Log.info("BackendServer::start() - Registering custom handler; done"); From 43d7cd7cebc81401d6356b2d404fc85e4ab701aa Mon Sep 17 00:00:00 2001 From: Reid Holmes Date: Mon, 16 Sep 2024 16:38:36 -0700 Subject: [PATCH 27/80] enhanced plugin logging --- packages/portal/backend/src/Factory.ts | 12 ++++++------ packages/portal/backend/src/server/BackendServer.ts | 3 ++- 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/packages/portal/backend/src/Factory.ts b/packages/portal/backend/src/Factory.ts index 3f44484bf..662bb31c2 100644 --- a/packages/portal/backend/src/Factory.ts +++ b/packages/portal/backend/src/Factory.ts @@ -38,12 +38,12 @@ export class Factory { let plug: any; Log.info("Factory::getCustomRouteHandler() - instantiating CustomCourseRoutes for: " + name + "; path: " + plugin); plug = await require("../../../../plugins/" + plugin + "/portal/backend/CustomCourseRoutes"); // default for testing - Log.trace("Factory::getCustomRouteHandler() - handler loaded"); + Log.trace("Factory::getCustomRouteHandler() - CustomRouteHandler loaded"); // if this fails an error will be raised and the default view will be provided in the catch below const constructorName = Object.keys(plug)[0]; const handler = new plug[constructorName](); - Log.info("Factory::getCustomRouteHandler() - handler instantiated"); + Log.info("Factory::getCustomRouteHandler() - CustomRouteHandler instantiated"); return handler; } catch (err) { Log.error(err); @@ -77,7 +77,7 @@ export class Factory { ghController = new GitHubController(GitHubActions.getInstance()); } else { // really only for testing - Log.trace("Factory::getCourseController() - using provided controller"); + Log.trace("Factory::getCourseController() - using provided CustomCourseController"); } try { const plugin = Config.getInstance().getProp(ConfigKey.plugin); @@ -93,7 +93,7 @@ export class Factory { // This is not required. But if it is added, it should never be pushed back to "classy/main" Log.trace("Factory::getCourseController() - name: " + name + " - plug: CustomCourseController"); const controllerPath = "../../../../plugins/" + plugin + "/portal/backend/CustomCourseController"; - Log.info("Factory::getCourseController() - full path: " + controllerPath); + Log.info("Factory::getCourseController() - loading CustomCourseController; full path: " + controllerPath); plug = await require(controllerPath); } catch (err) { const msg = "Factory::getCourseController() - " + plugin + "/src/custom/CustomCourseController.ts must be defined"; @@ -102,12 +102,12 @@ export class Factory { plug = null; } - Log.trace("Factory::getCourseController() - handler loaded"); + Log.trace("Factory::getCourseController() - CustomCourseController loaded"); // if this fails an error will be raised and the default view will be provided in the catch below const constructorName = Object.keys(plug)[0]; const handler = new plug[constructorName](ghController); - Log.trace("Factory::getCourseController() - handler instantiated"); + Log.trace("Factory::getCourseController() - CustomCourseController handler instantiated"); return handler; } catch (err) { const msg = "Factory::getCourseController() - src/custom/CustomCourseController.ts must be defined"; diff --git a/packages/portal/backend/src/server/BackendServer.ts b/packages/portal/backend/src/server/BackendServer.ts index fbbea9186..2ea9ddb69 100644 --- a/packages/portal/backend/src/server/BackendServer.ts +++ b/packages/portal/backend/src/server/BackendServer.ts @@ -127,8 +127,9 @@ export default class BackendServer { Log.info("BackendServer::start() - Loading custom route handler"); Factory.getCustomRouteHandler().then(function (handler) { + Log.info("BackendServer::start() - CustomRouteHandler loaded"); handler.registerRoutes(that.rest); - Log.info("BackendServer::start() - Registering custom handler; done"); + Log.info("BackendServer::start() - CustomRouteHandler registered"); // serve up the static frontend resources const frontendHTML = __dirname + "/../../../frontend/html"; From 52101cf4ca1dc04fe0fcd81671ab5882090f7fef Mon Sep 17 00:00:00 2001 From: Reid Holmes Date: Wed, 18 Sep 2024 08:24:12 -0700 Subject: [PATCH 28/80] user-select ignored on safari; trying webkit prefix --- packages/portal/frontend/html/style.css | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/portal/frontend/html/style.css b/packages/portal/frontend/html/style.css index 968065f0b..d9c8e1ded 100644 --- a/packages/portal/frontend/html/style.css +++ b/packages/portal/frontend/html/style.css @@ -106,6 +106,7 @@ Dashboard Page margin-right: auto; border-collapse: collapse; user-select: text !important; + -webkit-user-select: text !important; } .sortableHeader { From fdb245f93628dc4750e45e5cde91419fbe2b2303 Mon Sep 17 00:00:00 2001 From: Reid Holmes Date: Wed, 18 Sep 2024 08:32:22 -0700 Subject: [PATCH 29/80] more select support --- packages/portal/frontend/html/style.css | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/packages/portal/frontend/html/style.css b/packages/portal/frontend/html/style.css index d9c8e1ded..ab0365c0f 100644 --- a/packages/portal/frontend/html/style.css +++ b/packages/portal/frontend/html/style.css @@ -105,20 +105,23 @@ Dashboard Page margin-left: auto; margin-right: auto; border-collapse: collapse; - user-select: text !important; - -webkit-user-select: text !important; + user-select: auto; + -webkit-user-select: auto; } .sortableHeader { - user-select: text !important; + user-select: auto; + -webkit-user-select: auto; } .sortableTableRow { - user-select: text !important; + user-select: auto; + -webkit-user-select: auto; } .sortableTableCell { - user-select: text !important; + user-select: auto; + -webkit-user-select: auto; } .sortableTable th { @@ -128,7 +131,8 @@ Dashboard Page } .selectable { - user-select: text !important; + user-select: auto; + -webkit-user-select: auto; } /** From 8b166e62d3d8cdfea7abb1f8fa6a5956b3b5691a Mon Sep 17 00:00:00 2001 From: reid holmes Date: Thu, 19 Sep 2024 07:53:50 -0700 Subject: [PATCH 30/80] add the ability to bulk delete extraneous branches --- .../backend/src-util/RepositoryUpdater.ts | 184 ++++++++++++++++++ .../backend/src/controllers/GitHubActions.ts | 63 +++++- .../test/controllers/TestGitHubActions.ts | 4 + 3 files changed, 250 insertions(+), 1 deletion(-) create mode 100644 packages/portal/backend/src-util/RepositoryUpdater.ts diff --git a/packages/portal/backend/src-util/RepositoryUpdater.ts b/packages/portal/backend/src-util/RepositoryUpdater.ts new file mode 100644 index 000000000..b8cf311fc --- /dev/null +++ b/packages/portal/backend/src-util/RepositoryUpdater.ts @@ -0,0 +1,184 @@ +import Config from "@common/Config"; +import Log from "@common/Log"; + +import {DatabaseController} from "../src/controllers/DatabaseController"; +import {GitHubActions, IGitHubActions} from "../src/controllers/GitHubActions"; + +import {Repository} from "../src/Types"; +import {RepositoryController} from "@backend/controllers/RepositoryController"; + +/** + * Sometimes you may need to perform some GitHub actions on many repositories. + * This file shows how you can accomplish this. + * + * To run this locally you need to have a .env configured with the production values + * and an ssh tunnel configured to the server you want the database to come from. + * + * 1) Get on the UBC VPN. + * 2) Make sure you do not have a local mongo instance running. + * 3) Ensure your .env corresponds to the production values. + * 4) ssh user@host -L 27017:127.0.0.1:27017 + * 5) Run this script. + * + * Alternatively, this can be run on the production host, which saves you from + * having to configuring a .env. + * + * Regardless of how you are using this, running with DRY_RUN true + * is always recommended, so you can ensure the script is behaving + * as you expect. + * + */ +export class RepositoryUpdater { + + /** + * Only actually performs the action if DRY_RUN is false. + * Otherwise, just show what _would_ happen. + * NOTE: this is ignored for the TEST_USER user. + */ + private DRY_RUN = true; + + /** + * Usernames to ignore DRY_RUN for (aka usually a TA or course repo for testing) + */ + private readonly TEST_REPOSITORIES: string[] = []; // ["project_staff110"]; + + // /** + // * To make this request we are actually transforming a commit URL into an API request URL. + // * Having to hard-code this is not pretty, but it makes the code much simpler. The format + // * you need should be pretty easy to infer from what is present here. + // */ + // private readonly PREFIXOLD = "https://github.students.cs.ubc.ca/orgs/CPSC310-2022W-T2/"; + // private readonly PREFIXNEW = "https://github.students.cs.ubc.ca/api/v3/repos/CPSC310-2022W-T2/"; + + /** + * Specify a restriction on the repos to update. This is the prefix that any relevant repo must match. + */ + private readonly REPOPREFIX = "project_"; + + // private readonly MSG = "@310-bot #d1 #force #silent."; + // private readonly MSG = "@310-bot #d2 #force #silent."; + // private readonly MSG = "@310-bot #c1 #force #silent."; + // private readonly MSG = "@310-bot #c4 #force #silent."; + // private readonly MSG = "@310-bot #c3 #force #silent. C3 results will be posted to the Classy grades view once they are released. " + + // "\n\n Note: if you do not think this is the right commit, please fill out the project late grade request form " + + // "by December 14 @ 0800; we will finalize all project grades that day."; + + private dc: DatabaseController; + + constructor() { + Log.info("RepositoryUpdater:: - start"); + this.dc = DatabaseController.getInstance(); + } + + public async process(): Promise { + Log.info("RepositoryUpdater::process() - start"); + + const c = Config.getInstance(); + const gha = GitHubActions.getInstance(true); + + // Find the commit you want to invoke the bot against. + // e.g., usually you want to run against the commit associated + // with the grade record, as that is the 'max' commit + // but it is conceivable you might want to instead get all + // result rows and run against the latest before the deadline + // or some other approach. + // + // You might use some other approach here; any commit URL + // will work with the code below. + const reposC = new RepositoryController(); + Log.info("RepositoryUpdater::process() - requesting repos"); + const allRepos = await reposC.getAllRepos(); + Log.info("RepositoryUpdater::process() - # repos retrieved: " + allRepos.length); + + const matchedRepos = []; + for (const repo of allRepos as Repository[]) { + if (this.TEST_REPOSITORIES.length > 0) { + // if there are test repos, only consider those= + if (this.TEST_REPOSITORIES.indexOf(repo.id) >= 0) { + matchedRepos.push(repo); + } + } else { + // if there are no test repos, consider all repos that match the prefix + if (repo.id.startsWith(this.REPOPREFIX)) { + matchedRepos.push(repo); + } + } + } + Log.info("RepositoryUpdater::process() - # matched repos: " + matchedRepos.length); + Log.info("RepositoryUpdater::process() - checking that repos exist in GitHub..."); + + const matchedReposThatExist = []; + // Ensure the repo returned by RepositoryController actually exists on GitHub + for (const matchedRepo of matchedRepos) { + const repoExists = await gha.repoExists(matchedRepo.id); + if (repoExists === true) { + matchedReposThatExist.push(matchedRepo); + } + } + + Log.info("RepositoryUpdater::process() - # matched repos that exist: " + matchedReposThatExist.length); + Log.trace("RepositoryUpdater::process() - matched repos: " + JSON.stringify(matchedReposThatExist)); + + for (const matchedExistingRepo of matchedReposThatExist) { + await this.deleteUnwantedBranches(matchedExistingRepo, gha); + } + + Log.info("RepositoryUpdater::process() - done"); + } + + private async deleteUnwantedBranches(repo: Repository, gha: IGitHubActions): Promise { + Log.info("RepositoryUpdater::deleteUnwantedBranches() - start; repo: " + repo.id); + + /** + * The list of branches we want to delete. + */ + const BRANCH_NAMES_TO_DELETE = ["feature/pull_request_template_improvement", "removeFolderTest", "remove-dynamic-tests", "master", "errortype", "23W2EmptyChanges", "empty-repo"]; + + const allBranches = await gha.listRepoBranches(repo.id); + const branchesToRemove = []; + const branchesToKeep = []; + for (const branch of allBranches) { + if (BRANCH_NAMES_TO_DELETE.indexOf(branch) >= 0) { + // Log.info("RepositoryUpdater::deleteUnwantedBranches() - branch to REMOVE: " + branch); + branchesToRemove.push(branch); + } else { + branchesToKeep.push(branch); + } + } + + Log.info("RepositoryUpdater::deleteUnwantedBranches() - repo: " + repo.id + + "; branchesToRemove: " + JSON.stringify(branchesToRemove) + "; branchesToKeep: " + JSON.stringify(branchesToKeep)); + + let allSuccess = true; + for (const branch of branchesToRemove) { + if (this.DRY_RUN === false) { + Log.info("RepositoryUpdater::deleteUnwantedBranches() - DRY_RUN === false; removing branch; repo: " + + repo.id + "; branch: " + branch); + try { + const success = await gha.deleteBranch(repo.id, branch); + if (success === false) { + allSuccess = false; + } + Log.info("RepositoryUpdater::deleteUnwantedBranches() - removed branch; success: " + success); + } catch (err) { + Log.error("RepositoryUpdater::deleteUnwantedBranches() - ERROR: " + err.message); + } + + } else { + Log.info("RepositoryUpdater::deleteUnwantedBranches() - DRY_RUN === true; should have deleted branch - repo: " + + repo.id + "; branch: " + branch); + } + } + Log.info("RepositoryUpdater::deleteUnwantedBranches() - done; repo: " + repo.id + "; allSuccess: " + allSuccess); + return allSuccess; + } +} + +const ru = new RepositoryUpdater(); +ru.process().then(function () { + Log.info("RepositoryUpdater::process() - complete"); + process.exit(); +}).catch(function (err) { + Log.error("RepositoryUpdater::process() - ERROR: " + err.message); + process.exit(-1); +}); diff --git a/packages/portal/backend/src/controllers/GitHubActions.ts b/packages/portal/backend/src/controllers/GitHubActions.ts index 956265942..c32c2b37b 100644 --- a/packages/portal/backend/src/controllers/GitHubActions.ts +++ b/packages/portal/backend/src/controllers/GitHubActions.ts @@ -263,6 +263,15 @@ export interface IGitHubActions { */ deleteBranches(repoId: string, branchesToKeep: string[]): Promise; + /** + * Deletes all branches in a repo except for the ones listed in branchesToKeep. + * + * @param repoId + * @param branchesToDelete The name of the branch to delete + * @returns {Promise} true if the deletion was successful + */ + deleteBranch(repoId: string, branchToDelete: string): Promise; + /** * Renames a branch in a repo. * @@ -1401,7 +1410,7 @@ export class GitHubActions implements IGitHubActions { const start = Date.now(); const repoExists = await this.repoExists(repoId); // ensure the repo exists if (repoExists === false) { - Log.error("GitHubAction::listRepoBranches(..) - failed; repo does not exist"); + Log.error("GitHubAction::listRepoBranches( " + repoId + " ) - failed; repo does not exist"); return null; } @@ -1480,6 +1489,12 @@ export class GitHubActions implements IGitHubActions { return branches; } + /** + * NOTE: This method will delete all branches EXCEPT those in the branchesToKeep list. + * + * @param repoId + * @param branchesToKeep + */ public async deleteBranches(repoId: string, branchesToKeep: string[]): Promise { const start = Date.now(); @@ -1560,6 +1575,52 @@ export class GitHubActions implements IGitHubActions { return deleteSucceeded; } + /** + * NOTE: If a repo has a branch, it will be deleted. + * + * @param repoId + * @param branchToDelete + * @returns {Promise} true if the branch was deleted, false otherwise; throws error if something bad happened. + */ + public async deleteBranch(repoId: string, branchToDelete: string): Promise { + const start = Date.now(); + + // TODO: refactor this so deleteBranches calls this method (currently the delete fcn is fully duplicated) + const repoExists = await this.repoExists(repoId); // ensure the repo exists + if (repoExists === false) { + Log.error("GitHubAction::deleteBranch(..) - failed; repo does not exist"); + return false; + } + + Log.info("GitHubAction::deleteBranch( " + repoId + ", " + branchToDelete + " ) - start"); + + // DELETE /repos/{owner}/{repo}/git/refs/{ref} + const delUri = this.apiPath + "/repos/" + this.org + "/" + repoId + "/git/refs/" + "heads/" + branchToDelete; + Log.info("GitHubAction::deleteBranch(..) - delete branch; uri: " + delUri); + + const delOptions: RequestInit = { + method: "DELETE", + headers: { + "Authorization": this.gitHubAuthToken, + "User-Agent": this.gitHubUserName, + "Accept": "application/vnd.github+json", + "X-GitHub-Api-Version": "2022-11-28" + } + }; + + const deleteResp = await fetch(delUri, delOptions); + Log.trace("GitHubAction::deleteBranch(..) - delete response code: " + deleteResp.status); + + if (deleteResp.status !== 204) { + const delRespBody = await deleteResp.json(); + Log.warn("GitHubAction::deleteBranches(..) - failed to delete branch for repo; response: " + JSON.stringify(delRespBody)); + return false; + } else { + Log.info("GitHubAction::deleteBranches(..) - successfully deleted branch: " + branchToDelete + " from repo: " + repoId); + return true; + } + } + public async renameBranch(repoId: string, oldName: string, newName: string): Promise { Log.info("GitHubAction::renameBranch( " + repoId + ", " + oldName + ", " + newName + " ) - start"); diff --git a/packages/portal/backend/test/controllers/TestGitHubActions.ts b/packages/portal/backend/test/controllers/TestGitHubActions.ts index 2b17a77df..a8d158d0f 100644 --- a/packages/portal/backend/test/controllers/TestGitHubActions.ts +++ b/packages/portal/backend/test/controllers/TestGitHubActions.ts @@ -26,6 +26,10 @@ export class TestGitHubActions implements IGitHubActions { throw new Error("Method not implemented."); } + public deleteBranch(repoId: string, branchToDelete: string): Promise { + throw new Error("Method not implemented."); + } + public renameBranch(repoId: string, oldName: string, newName: string): Promise { throw new Error("Method not implemented."); } From d25f8bf842d7615fb091b717f4d917b36f5543c2 Mon Sep 17 00:00:00 2001 From: Reid Holmes Date: Thu, 19 Sep 2024 13:16:48 -0700 Subject: [PATCH 31/80] remove duplicated code for branch removal --- .../backend/src/controllers/GitHubActions.ts | 32 +++---------------- 1 file changed, 5 insertions(+), 27 deletions(-) diff --git a/packages/portal/backend/src/controllers/GitHubActions.ts b/packages/portal/backend/src/controllers/GitHubActions.ts index c32c2b37b..11a2db703 100644 --- a/packages/portal/backend/src/controllers/GitHubActions.ts +++ b/packages/portal/backend/src/controllers/GitHubActions.ts @@ -1528,30 +1528,7 @@ export class GitHubActions implements IGitHubActions { // delete branches we do not want let deleteSucceeded = true; for (const branch of branchesToDelete) { - // DELETE /repos/{owner}/{repo}/git/refs/{ref} - const delUri = this.apiPath + "/repos/" + this.org + "/" + repoId + "/git/refs/" + "heads/" + branch; - Log.info("GitHubAction::deleteBranches(..) - delete branch; uri: " + delUri); - - const delOptions: RequestInit = { - method: "DELETE", - headers: { - "Authorization": this.gitHubAuthToken, - "User-Agent": this.gitHubUserName, - "Accept": "application/vnd.github+json", - "X-GitHub-Api-Version": "2022-11-28" - } - }; - - const deleteResp = await fetch(delUri, delOptions); - Log.trace("GitHubAction::deleteBranches(..) - delete response code: " + deleteResp.status); - - if (deleteResp.status !== 204) { - const delRespBody = await deleteResp.json(); - Log.warn("GitHubAction::deleteBranches(..) - failed to delete branch for repo; response: " + JSON.stringify(delRespBody)); - deleteSucceeded = false; - } else { - Log.info("GitHubAction::deleteBranches(..) - successfully deleted branch: " + branch + " from repo: " + repoId); - } + deleteSucceeded = await this.deleteBranch(repoId, branch); } // This is an unsatisfying check. But GitHub Enterprise often returns repo provisioning @@ -1585,7 +1562,6 @@ export class GitHubActions implements IGitHubActions { public async deleteBranch(repoId: string, branchToDelete: string): Promise { const start = Date.now(); - // TODO: refactor this so deleteBranches calls this method (currently the delete fcn is fully duplicated) const repoExists = await this.repoExists(repoId); // ensure the repo exists if (repoExists === false) { Log.error("GitHubAction::deleteBranch(..) - failed; repo does not exist"); @@ -1613,10 +1589,12 @@ export class GitHubActions implements IGitHubActions { if (deleteResp.status !== 204) { const delRespBody = await deleteResp.json(); - Log.warn("GitHubAction::deleteBranches(..) - failed to delete branch for repo; response: " + JSON.stringify(delRespBody)); + Log.warn("GitHubAction::deleteBranches(..) - failed to delete branch for repo; response: " + + JSON.stringify(delRespBody)); return false; } else { - Log.info("GitHubAction::deleteBranches(..) - successfully deleted branch: " + branchToDelete + " from repo: " + repoId); + Log.info("GitHubAction::deleteBranches(..) - successfully deleted branch: " + + branchToDelete + " from repo: " + repoId + "; took: " + Util.took(start)); return true; } } From 30e3e437366e523862cd6bd4af39310c6cb21fa8 Mon Sep 17 00:00:00 2001 From: Reid Holmes Date: Mon, 7 Oct 2024 15:02:23 -0700 Subject: [PATCH 32/80] improve timeout string --- packages/autotest/src/autotest/GradingJob.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/autotest/src/autotest/GradingJob.ts b/packages/autotest/src/autotest/GradingJob.ts index 07f9597d5..92824d4bb 100644 --- a/packages/autotest/src/autotest/GradingJob.ts +++ b/packages/autotest/src/autotest/GradingJob.ts @@ -149,7 +149,7 @@ export class GradingJob { out.state = ContainerState.FAIL; out.postbackOnComplete = true; // always send fail feedback } else if (exitCode === -1) { - let msg = "Container did not complete for `" + this.input.target.delivId + "` in the allotted time. "; + let msg = "Container did not complete for **`#" + this.input.target.delivId + "`** in the allotted time. "; msg += "This likely means that _our_ tests exposed a slow or non-terminating path in _your_ implementation. "; msg += "You should augment your tests; a comprehensive local suite will uncover the problem."; From 0b8ae02bbbc2152445f6c0a430b791de1926bb9d Mon Sep 17 00:00:00 2001 From: Reid Holmes Date: Tue, 8 Oct 2024 13:47:05 -0700 Subject: [PATCH 33/80] see #114 allow grades that are not numbers to be uploaded --- .../backend/src/server/common/CSVParser.ts | 26 +++++++++++++++-- packages/portal/backend/test/CSVParserSpec.ts | 29 +++++++++++++++++++ .../backend/test/data/gradesValidBucket.csv | 4 +++ 3 files changed, 57 insertions(+), 2 deletions(-) create mode 100644 packages/portal/backend/test/data/gradesValidBucket.csv diff --git a/packages/portal/backend/src/server/common/CSVParser.ts b/packages/portal/backend/src/server/common/CSVParser.ts index 8fc8e94a2..87b583428 100644 --- a/packages/portal/backend/src/server/common/CSVParser.ts +++ b/packages/portal/backend/src/server/common/CSVParser.ts @@ -114,18 +114,40 @@ export class CSVParser { typeof row.COMMENT === "string" && row.COMMENT.length > 1) { comment = row.COMMENT; + comment = comment.trim(); + } + + const isNumber = function (value: string): boolean { + const num = parseFloat(value); + return Number.isNaN(num) ? false : true; + }; + + let gradeScore = row.GRADE; + + if (typeof gradeScore === "string") { + gradeScore = gradeScore.trim(); + } + + const custom: any = {}; + if (isNumber(gradeScore) === true) { + gradeScore = parseFloat(gradeScore); + Log.trace("CSVParser::processGrades(..) - grade is a number: " + gradeScore); + } else { + custom.displayScore = gradeScore; + gradeScore = -1; // grade is reflected in displayScore instead + Log.trace("CSVParser::processGrades(..) - grade is a string: " + custom.displayScore); } const personId = row.CSID; const g: Grade = { personId: personId, delivId: delivId, - score: Number(row.GRADE), + score: gradeScore, // Number(row.GRADE), comment: comment, timestamp: Date.now(), urlName: "CSV Upload", URL: null, // set to null so GradesController can restore URL if needed - custom: {} + custom: custom }; const person = pc.getPerson(personId); diff --git a/packages/portal/backend/test/CSVParserSpec.ts b/packages/portal/backend/test/CSVParserSpec.ts index bd6eabc70..fc8109b84 100644 --- a/packages/portal/backend/test/CSVParserSpec.ts +++ b/packages/portal/backend/test/CSVParserSpec.ts @@ -50,6 +50,35 @@ describe("CSVParser", function () { expect(grade.score).to.equal(19); }); + it("Should be able to process a valid grade sheet where the grades are strings", async function () { + // check pre + const gc = new GradesController(); + let grade = await gc.getGrade(TestHarness.USER1.id, TestHarness.DELIVID1); + expect(grade.score).to.equal(92); + grade = await gc.getGrade(TestHarness.USER2.id, TestHarness.DELIVID1); + expect(grade.score).to.equal(29); + grade = await gc.getGrade(TestHarness.USER3.id, TestHarness.DELIVID1); + expect(grade.score).to.equal(19); + + // do upload + const path = __dirname + "/data/gradesValidBucket.csv"; + const csv = new CSVParser(); + const rows = await csv.processGrades(TestHarness.ADMIN1.id, TestHarness.DELIVID1, path); + Log.test("# rows processed: " + rows.length); + expect(rows).to.have.lengthOf(3); + + // validate outcome + grade = await gc.getGrade(TestHarness.USER1.id, TestHarness.DELIVID1); + expect(grade.score).to.equal(-1); + expect(grade.custom.displayScore).to.equal("EXTENDING"); + grade = await gc.getGrade(TestHarness.USER2.id, TestHarness.DELIVID1); + expect(grade.score).to.equal(-1); + expect(grade.custom.displayScore).to.equal("PROFICIENT"); + grade = await gc.getGrade(TestHarness.USER3.id, TestHarness.DELIVID1); + expect(grade.score).to.equal(-1); + expect(grade.custom.displayScore).to.equal("N/A"); + }); + it("Should not be able to process grades for an invalid deliverable", async function () { let rows = null; let ex = null; diff --git a/packages/portal/backend/test/data/gradesValidBucket.csv b/packages/portal/backend/test/data/gradesValidBucket.csv new file mode 100644 index 000000000..67af2ecb5 --- /dev/null +++ b/packages/portal/backend/test/data/gradesValidBucket.csv @@ -0,0 +1,4 @@ +CSID,GRADE,COMMENT +user1ID, EXTENDING ,csv comment +user2ID, PROFICIENT,csv comment +user3ID,N/A ,csv comment From 5ee290879623784420a7f8db264fefd5db86fb30 Mon Sep 17 00:00:00 2001 From: Reid Holmes Date: Tue, 8 Oct 2024 14:14:54 -0700 Subject: [PATCH 34/80] see #114 get displayScore to show up in admin view --- packages/common/src/Util.ts | 5 +++++ .../portal/backend/src/server/common/CSVParser.ts | 11 ++++++----- .../portal/frontend/src/app/views/AdminGradesTab.ts | 7 ++++++- 3 files changed, 17 insertions(+), 6 deletions(-) diff --git a/packages/common/src/Util.ts b/packages/common/src/Util.ts index 02f4e2075..aa1e606f1 100644 --- a/packages/common/src/Util.ts +++ b/packages/common/src/Util.ts @@ -299,4 +299,9 @@ export default class Util { // at the end of everything, just defer to localCompare return ("" + a).localeCompare("" + b); } + + public static isNumber(value: string): boolean { + const num = parseFloat(value); + return Number.isNaN(num) ? false : true; + } } diff --git a/packages/portal/backend/src/server/common/CSVParser.ts b/packages/portal/backend/src/server/common/CSVParser.ts index 87b583428..524af2fa6 100644 --- a/packages/portal/backend/src/server/common/CSVParser.ts +++ b/packages/portal/backend/src/server/common/CSVParser.ts @@ -9,6 +9,7 @@ import {DeliverablesController} from "@backend/controllers/DeliverablesControlle import {GradesController} from "@backend/controllers/GradesController"; import {PersonController} from "@backend/controllers/PersonController"; import {AuditLabel, Grade} from "@backend/Types"; +import Util from "@common/Util"; export class CSVParser { @@ -117,10 +118,10 @@ export class CSVParser { comment = comment.trim(); } - const isNumber = function (value: string): boolean { - const num = parseFloat(value); - return Number.isNaN(num) ? false : true; - }; + // const isNumber = function (value: string): boolean { + // const num = parseFloat(value); + // return Number.isNaN(num) ? false : true; + // }; let gradeScore = row.GRADE; @@ -129,7 +130,7 @@ export class CSVParser { } const custom: any = {}; - if (isNumber(gradeScore) === true) { + if (Util.isNumber(gradeScore) === true) { gradeScore = parseFloat(gradeScore); Log.trace("CSVParser::processGrades(..) - grade is a number: " + gradeScore); } else { diff --git a/packages/portal/frontend/src/app/views/AdminGradesTab.ts b/packages/portal/frontend/src/app/views/AdminGradesTab.ts index b7316c0d0..b72378b9a 100644 --- a/packages/portal/frontend/src/app/views/AdminGradesTab.ts +++ b/packages/portal/frontend/src/app/views/AdminGradesTab.ts @@ -125,7 +125,12 @@ export class AdminGradesTab extends AdminPage { const hoverComment = AdminGradesTab.makeHTMLSafe(grade.comment); let scoreText: string = ""; let scorePrepend = ""; - if (grade.score !== null && grade.score >= 0) { + + if (grade?.custom?.displayScore) { + // if there's a custom grade to display, use that instead + // Log.trace("AdminGradesTab::render() - using custom display score: " + grade.custom.displayScore); + scoreText = grade.custom.displayScore; + } else if (grade.score !== null && grade.score >= 0) { scoreText = grade.score.toFixed(2); if (grade.score < 100) { // two decimal places From 11500530872f322e4da95e83e599bd599b27c50d Mon Sep 17 00:00:00 2001 From: Reid Holmes Date: Tue, 8 Oct 2024 14:23:25 -0700 Subject: [PATCH 35/80] docs --- packages/portal/backend/src/server/common/CSVParser.ts | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/packages/portal/backend/src/server/common/CSVParser.ts b/packages/portal/backend/src/server/common/CSVParser.ts index 524af2fa6..ff07118d8 100644 --- a/packages/portal/backend/src/server/common/CSVParser.ts +++ b/packages/portal/backend/src/server/common/CSVParser.ts @@ -56,13 +56,15 @@ export class CSVParser { * * Classy grade upload CSVs require two * columns (each with a case-sensitive header): - * * CSID - * * GRADE + * * CSID + * * GRADE * * One optional column is also considered, if present: * * COMMENT * - * If CSID is absent but CWL or GITHUB is present, we map them to the CSID and proceed as needed. + * If the CSID header is absent but CWL or GITHUB is present, we map them to + * the CSID and proceed as needed. + * * The deliverable the CSV is applied to is specified by the upload page. * * @param requesterId From 5b324c32f5eefdf83a0e5cea30d0fca4500c3681 Mon Sep 17 00:00:00 2001 From: Reid Holmes Date: Tue, 8 Oct 2024 14:35:21 -0700 Subject: [PATCH 36/80] add notion of (optional) DISPLAY column to CSV upload DISPLAY column to --- .../backend/src/server/common/CSVParser.ts | 24 ++++++++++--------- packages/portal/backend/test/CSVParserSpec.ts | 6 ++--- .../backend/test/data/gradesValidBucket.csv | 8 +++---- 3 files changed, 20 insertions(+), 18 deletions(-) diff --git a/packages/portal/backend/src/server/common/CSVParser.ts b/packages/portal/backend/src/server/common/CSVParser.ts index ff07118d8..e4b49688e 100644 --- a/packages/portal/backend/src/server/common/CSVParser.ts +++ b/packages/portal/backend/src/server/common/CSVParser.ts @@ -19,7 +19,7 @@ export class CSVParser { /** * Use CSV-Parse to turn a file path into an array of rows. Since we do not know anything - * about each row, we"re just returning it as an array of any. Clients should check to + * about each row, we are just returning it as an array of any. Clients should check to * make sure the right properties exist on each row (e.g., that all the columns are there). * * @param {string} path @@ -59,8 +59,12 @@ export class CSVParser { * * CSID * * GRADE * - * One optional column is also considered, if present: + * Two optional columns are also considered, if present: * * COMMENT + * * DISPLAY + * * This is the value that will be shown to the students, if it is present. + * * The GRADE will still be visible in the network view though. + * * Set GRADE to -1 if you do not want the students to see it. * * If the CSID header is absent but CWL or GITHUB is present, we map them to * the CSID and proceed as needed. @@ -120,11 +124,6 @@ export class CSVParser { comment = comment.trim(); } - // const isNumber = function (value: string): boolean { - // const num = parseFloat(value); - // return Number.isNaN(num) ? false : true; - // }; - let gradeScore = row.GRADE; if (typeof gradeScore === "string") { @@ -136,16 +135,19 @@ export class CSVParser { gradeScore = parseFloat(gradeScore); Log.trace("CSVParser::processGrades(..) - grade is a number: " + gradeScore); } else { - custom.displayScore = gradeScore; - gradeScore = -1; // grade is reflected in displayScore instead - Log.trace("CSVParser::processGrades(..) - grade is a string: " + custom.displayScore); + gradeScore = Number(gradeScore); // might as well try + } + + if (typeof row.DISPLAY === "string") { + custom.displayScore = row.DISPLAY.trim(); + Log.trace("CSVParser::processGrades(..) - grade includes DISPLAY: " + custom.displayScore); } const personId = row.CSID; const g: Grade = { personId: personId, delivId: delivId, - score: gradeScore, // Number(row.GRADE), + score: gradeScore, comment: comment, timestamp: Date.now(), urlName: "CSV Upload", diff --git a/packages/portal/backend/test/CSVParserSpec.ts b/packages/portal/backend/test/CSVParserSpec.ts index fc8109b84..f613ff37d 100644 --- a/packages/portal/backend/test/CSVParserSpec.ts +++ b/packages/portal/backend/test/CSVParserSpec.ts @@ -69,13 +69,13 @@ describe("CSVParser", function () { // validate outcome grade = await gc.getGrade(TestHarness.USER1.id, TestHarness.DELIVID1); - expect(grade.score).to.equal(-1); + expect(grade.score).to.equal(100); expect(grade.custom.displayScore).to.equal("EXTENDING"); grade = await gc.getGrade(TestHarness.USER2.id, TestHarness.DELIVID1); - expect(grade.score).to.equal(-1); + expect(grade.score).to.equal(80); expect(grade.custom.displayScore).to.equal("PROFICIENT"); grade = await gc.getGrade(TestHarness.USER3.id, TestHarness.DELIVID1); - expect(grade.score).to.equal(-1); + expect(grade.score).to.equal(0); expect(grade.custom.displayScore).to.equal("N/A"); }); diff --git a/packages/portal/backend/test/data/gradesValidBucket.csv b/packages/portal/backend/test/data/gradesValidBucket.csv index 67af2ecb5..197a0b964 100644 --- a/packages/portal/backend/test/data/gradesValidBucket.csv +++ b/packages/portal/backend/test/data/gradesValidBucket.csv @@ -1,4 +1,4 @@ -CSID,GRADE,COMMENT -user1ID, EXTENDING ,csv comment -user2ID, PROFICIENT,csv comment -user3ID,N/A ,csv comment +CSID,GRADE,DISPLAY,COMMENT +user1ID, 100,EXTENDING ,csv comment +user2ID, 80,PROFICIENT,csv comment +user3ID,0,N/A ,csv comment From ff22ffa7ce231d521996455442b97bd976fad432 Mon Sep 17 00:00:00 2001 From: Reid Holmes Date: Tue, 8 Oct 2024 14:53:15 -0700 Subject: [PATCH 37/80] comments --- packages/portal/frontend/src/app/views/AbstractStudentView.ts | 2 +- packages/portal/frontend/src/app/views/AdminGradesTab.ts | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/packages/portal/frontend/src/app/views/AbstractStudentView.ts b/packages/portal/frontend/src/app/views/AbstractStudentView.ts index c9dbaac06..0c8241daf 100644 --- a/packages/portal/frontend/src/app/views/AbstractStudentView.ts +++ b/packages/portal/frontend/src/app/views/AbstractStudentView.ts @@ -138,7 +138,7 @@ export abstract class AbstractStudentView implements IView { for (const grade of this.grades) { let scoreToDisplay: number | string = grade.score; // default behaviour - // allow for custom display scores + // overwrite grade with custom score if it is provided if (grade?.custom?.displayScore) { Log.info("AbstractStudentView::renderGrades() - using custom display score: " + grade.custom.displayScore); scoreToDisplay = grade.custom.displayScore; diff --git a/packages/portal/frontend/src/app/views/AdminGradesTab.ts b/packages/portal/frontend/src/app/views/AdminGradesTab.ts index b72378b9a..4b40b2761 100644 --- a/packages/portal/frontend/src/app/views/AdminGradesTab.ts +++ b/packages/portal/frontend/src/app/views/AdminGradesTab.ts @@ -127,7 +127,8 @@ export class AdminGradesTab extends AdminPage { let scorePrepend = ""; if (grade?.custom?.displayScore) { - // if there's a custom grade to display, use that instead + // check this first so we prefer the custom display score + // if there is a custom grade to display, use that instead // Log.trace("AdminGradesTab::render() - using custom display score: " + grade.custom.displayScore); scoreText = grade.custom.displayScore; } else if (grade.score !== null && grade.score >= 0) { From 866f36661fe0251c9cb8517cae59204cfaa11514 Mon Sep 17 00:00:00 2001 From: Reid Holmes Date: Tue, 8 Oct 2024 19:36:44 -0700 Subject: [PATCH 38/80] see #114 handle github id uploads better in CSVs --- .../backend/src/server/common/CSVParser.ts | 24 ++++++++++++----- packages/portal/backend/test/CSVParserSpec.ts | 26 ++++++++++++++++++- .../test/data/gradesValidBucketCWL.csv | 4 +++ .../test/data/gradesValidBucketGithub.csv | 4 +++ 4 files changed, 51 insertions(+), 7 deletions(-) create mode 100644 packages/portal/backend/test/data/gradesValidBucketCWL.csv create mode 100644 packages/portal/backend/test/data/gradesValidBucketGithub.csv diff --git a/packages/portal/backend/src/server/common/CSVParser.ts b/packages/portal/backend/src/server/common/CSVParser.ts index e4b49688e..cbdeba009 100644 --- a/packages/portal/backend/src/server/common/CSVParser.ts +++ b/packages/portal/backend/src/server/common/CSVParser.ts @@ -91,14 +91,18 @@ export class CSVParser { const gc = new GradesController(); const gradePromises: Array> = []; + let errorMessage = ""; for (const row of data) { if (typeof row.CSID === "undefined" && typeof row.GITHUB !== "undefined") { Log.trace("CSVParser::processGrades(..) - CSID absent but GITHUB present; GITHUB: " + row.GITHUB); const person = await pc.getGitHubPerson(row.GITHUB); if (person !== null) { - row.CSID = person.csId; + row.CSID = person.id; Log.info("CSVParser::processGrades(..) - GITHUB -> CSID: " + row.GITHUB + " -> " + row.CSID); + } else { + Log.warn("CSVParser::processGrades(..) - Unknown GITHUB: " + row.GITHUB); + errorMessage += "Unknown GITHUB: " + row.GITHUB + ", "; } } @@ -106,8 +110,11 @@ export class CSVParser { Log.trace("CSVParser::processGrades(..) - CSID absent but CWL present; CWL: " + row.CWL); const person = await pc.getGitHubPerson(row.CWL); // GITHUB && CWL are the same at UBC if (person !== null) { - row.CSID = person.csId; + row.CSID = person.id; Log.info("CSVParser::processGrades(..) - CWL -> CSID: " + row.CWL + " -> " + row.CSID); + } else { + Log.warn("CSVParser::processGrades(..) - Unknown CWL: " + row.CWL); + errorMessage += "Unknown CWL: " + row.CWL + ", "; } } @@ -155,7 +162,7 @@ export class CSVParser { custom: custom }; - const person = pc.getPerson(personId); + const person = await pc.getPerson(personId); if (person !== null) { gradePromises.push(gc.saveGrade(g)); } else { @@ -163,9 +170,8 @@ export class CSVParser { } } else { - const msg = "Required column missing (required: CSID, GRADE)."; - Log.error("CSVParser::processGrades(..) - column missing from: " + JSON.stringify(row)); - throw new Error(msg); + Log.warn("CSVParser::processGrades(..) - bad record: " + JSON.stringify(row)); + errorMessage += "Bad record: " + JSON.stringify(row) + ", "; } } @@ -176,6 +182,12 @@ export class CSVParser { await dbc.writeAudit(AuditLabel.GRADE_ADMIN, requesterId, {}, {}, {numGrades: grades.length}); Log.info("CSVParser::processGrades(..) - done; # grades: " + grades.length); + if (errorMessage.length > 0) { + const msg = "CSVParser::processGrades(..) - ERROR: " + errorMessage; + Log.error(msg); + throw new Error(msg); + } + return grades; } catch (err) { Log.error("CSVParser::processGrades(..) - ERROR: " + err.message); diff --git a/packages/portal/backend/test/CSVParserSpec.ts b/packages/portal/backend/test/CSVParserSpec.ts index f613ff37d..df2c713c1 100644 --- a/packages/portal/backend/test/CSVParserSpec.ts +++ b/packages/portal/backend/test/CSVParserSpec.ts @@ -8,8 +8,9 @@ import {GradesController} from "@backend/controllers/GradesController"; import {CSVParser} from "@backend/server/common/CSVParser"; import "@common/GlobalSpec"; +import {Grade} from "@backend/Types"; -describe("CSVParser", function () { +describe.only("CSVParser", function () { before(async () => { await TestHarness.suiteBefore("CSVParser"); @@ -79,6 +80,29 @@ describe("CSVParser", function () { expect(grade.custom.displayScore).to.equal("N/A"); }); + it("Should be able to process a valid grade sheet where the grades are strings w/ github header", async function () { + // check pre + const gc = new GradesController(); + let grade: Grade; + // do upload + const path = __dirname + "/data/gradesValidBucketGithub.csv"; + const csv = new CSVParser(); + const rows = await csv.processGrades(TestHarness.ADMIN1.id, TestHarness.DELIVID1, path); + Log.test("# rows processed: " + rows.length); + expect(rows).to.have.lengthOf(3); + + // validate outcome + grade = await gc.getGrade(TestHarness.USER1.id, TestHarness.DELIVID1); + expect(grade.score).to.equal(99); + expect(grade.custom.displayScore).to.equal("EXTENDING1"); + grade = await gc.getGrade(TestHarness.USER2.id, TestHarness.DELIVID1); + expect(grade.score).to.equal(79); + expect(grade.custom.displayScore).to.equal("PROFICIENT1"); + grade = await gc.getGrade(TestHarness.USER3.id, TestHarness.DELIVID1); + expect(grade.score).to.equal(1); + expect(grade.custom.displayScore).to.equal("N/A1"); + }); + it("Should not be able to process grades for an invalid deliverable", async function () { let rows = null; let ex = null; diff --git a/packages/portal/backend/test/data/gradesValidBucketCWL.csv b/packages/portal/backend/test/data/gradesValidBucketCWL.csv new file mode 100644 index 000000000..aecb6c80e --- /dev/null +++ b/packages/portal/backend/test/data/gradesValidBucketCWL.csv @@ -0,0 +1,4 @@ +CWL,GRADE,DISPLAY,COMMENT +user1ID, 100,EXTENDING ,csv comment +user2ID, 80,PROFICIENT,csv comment +user3ID,0,N/A ,csv comment diff --git a/packages/portal/backend/test/data/gradesValidBucketGithub.csv b/packages/portal/backend/test/data/gradesValidBucketGithub.csv new file mode 100644 index 000000000..0f5c79f10 --- /dev/null +++ b/packages/portal/backend/test/data/gradesValidBucketGithub.csv @@ -0,0 +1,4 @@ +GITHUB,GRADE,DISPLAY,COMMENT +user1gh, 99,EXTENDING1 ,csv comment +user2gh, 79,PROFICIENT1,csv comment +user3gh,1,N/A1 ,csv comment From bb8a256bda4f01fd67746d17657162bbde627481 Mon Sep 17 00:00:00 2001 From: Reid Holmes Date: Tue, 8 Oct 2024 19:37:19 -0700 Subject: [PATCH 39/80] remove .only --- packages/portal/backend/test/CSVParserSpec.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/portal/backend/test/CSVParserSpec.ts b/packages/portal/backend/test/CSVParserSpec.ts index df2c713c1..b67c8cb01 100644 --- a/packages/portal/backend/test/CSVParserSpec.ts +++ b/packages/portal/backend/test/CSVParserSpec.ts @@ -10,7 +10,7 @@ import {CSVParser} from "@backend/server/common/CSVParser"; import "@common/GlobalSpec"; import {Grade} from "@backend/Types"; -describe.only("CSVParser", function () { +describe("CSVParser", function () { before(async () => { await TestHarness.suiteBefore("CSVParser"); From 5d1a78d09ff8707682c83d165070ccc81a02e720 Mon Sep 17 00:00:00 2001 From: Reid Holmes Date: Tue, 8 Oct 2024 19:50:07 -0700 Subject: [PATCH 40/80] add sorting for buckets in UI --- .../frontend/src/app/util/SortableTable.ts | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/packages/portal/frontend/src/app/util/SortableTable.ts b/packages/portal/frontend/src/app/util/SortableTable.ts index 0f493faef..29f328423 100644 --- a/packages/portal/frontend/src/app/util/SortableTable.ts +++ b/packages/portal/frontend/src/app/util/SortableTable.ts @@ -233,6 +233,23 @@ export class SortableTable { const aVal = a[sortIndex].value; const bVal = b[sortIndex].value; + if (typeof aVal === "string" && typeof bVal === "string") { + const OPTIONS = [ + "extending", + "proficient", + "developing", + "acquiring", + "beginning", + "n/a"]; + + const aIndex = OPTIONS.indexOf(aVal.toLowerCase()); + const bIndex = OPTIONS.indexOf(bVal.toLowerCase()); + + if (aIndex > -1 && bIndex > -1) { + return Util.compare(aVal, bVal) * mult; + } + } + return Util.compare(aVal, bVal) * mult; }); } From f09fa558b03d0d0ddf437dafbf2fb30d1e26737c Mon Sep 17 00:00:00 2001 From: Reid Holmes Date: Wed, 9 Oct 2024 07:14:08 -0700 Subject: [PATCH 41/80] see #114 better csv upload error handling --- .../backend/src/server/common/CSVParser.ts | 40 +++++++++++++++---- 1 file changed, 33 insertions(+), 7 deletions(-) diff --git a/packages/portal/backend/src/server/common/CSVParser.ts b/packages/portal/backend/src/server/common/CSVParser.ts index cbdeba009..846276c00 100644 --- a/packages/portal/backend/src/server/common/CSVParser.ts +++ b/packages/portal/backend/src/server/common/CSVParser.ts @@ -92,8 +92,18 @@ export class CSVParser { const gc = new GradesController(); const gradePromises: Array> = []; let errorMessage = ""; + let unknownId = ""; for (const row of data) { + // this check could probably be outside the loop, but since it throws + // it only happens once anyways + const firstKey = Object.keys(row)[0]; + if (firstKey === "CSID" || firstKey === "CWL" || firstKey === "GITHUB") { + // good record + } else { + throw new Error("CSID/CWL/GITHUB should be the first column in the CSV"); + } + if (typeof row.CSID === "undefined" && typeof row.GITHUB !== "undefined") { Log.trace("CSVParser::processGrades(..) - CSID absent but GITHUB present; GITHUB: " + row.GITHUB); const person = await pc.getGitHubPerson(row.GITHUB); @@ -102,7 +112,10 @@ export class CSVParser { Log.info("CSVParser::processGrades(..) - GITHUB -> CSID: " + row.GITHUB + " -> " + row.CSID); } else { Log.warn("CSVParser::processGrades(..) - Unknown GITHUB: " + row.GITHUB); - errorMessage += "Unknown GITHUB: " + row.GITHUB + ", "; + if (errorMessage === "") { + "Unknwon GitHub ids: "; + } + errorMessage += row.GITHUB + ", "; } } @@ -114,7 +127,10 @@ export class CSVParser { Log.info("CSVParser::processGrades(..) - CWL -> CSID: " + row.CWL + " -> " + row.CSID); } else { Log.warn("CSVParser::processGrades(..) - Unknown CWL: " + row.CWL); - errorMessage += "Unknown CWL: " + row.CWL + ", "; + if (errorMessage === "") { + "Unknwon CWLs: "; + } + errorMessage += row.CWL + ", "; } } @@ -149,7 +165,6 @@ export class CSVParser { custom.displayScore = row.DISPLAY.trim(); Log.trace("CSVParser::processGrades(..) - grade includes DISPLAY: " + custom.displayScore); } - const personId = row.CSID; const g: Grade = { personId: personId, @@ -167,11 +182,15 @@ export class CSVParser { gradePromises.push(gc.saveGrade(g)); } else { Log.warn("CSVParser::processGrades(..) - record ignored for: " + personId + "; unknown personId"); + if (unknownId === "") { + unknownId = "Unknown personIds: "; + } + unknownId += personId + ", "; } } else { - Log.warn("CSVParser::processGrades(..) - bad record: " + JSON.stringify(row)); - errorMessage += "Bad record: " + JSON.stringify(row) + ", "; + Log.warn("CSVParser::processGrades(..) - could not parse grade for record: " + JSON.stringify(row)); + // errorMessage += "Bad record: " + JSON.stringify(row) + ", "; } } @@ -182,8 +201,15 @@ export class CSVParser { await dbc.writeAudit(AuditLabel.GRADE_ADMIN, requesterId, {}, {}, {numGrades: grades.length}); Log.info("CSVParser::processGrades(..) - done; # grades: " + grades.length); - if (errorMessage.length > 0) { - const msg = "CSVParser::processGrades(..) - ERROR: " + errorMessage; + if (errorMessage.endsWith(", ")) { + errorMessage.slice(0, -2); + } + if (unknownId.endsWith(", ")) { + unknownId.slice(0, -2); + } + + if (errorMessage.length > 0 || unknownId.length > 0) { + const msg = "CSVParser::processGrades(..) - ERROR: " + errorMessage + "; ID: " + unknownId; Log.error(msg); throw new Error(msg); } From 16d19e066b933b5d7426f3eecf934ee28ffdeb6b Mon Sep 17 00:00:00 2001 From: Reid Holmes Date: Wed, 9 Oct 2024 07:28:05 -0700 Subject: [PATCH 42/80] see #114 csv parsing (and some table sorting) --- packages/common/src/Util.ts | 5 ----- .../backend/src/server/common/CSVParser.ts | 20 ++++++++----------- .../frontend/src/app/util/SortableTable.ts | 7 +++++-- 3 files changed, 13 insertions(+), 19 deletions(-) diff --git a/packages/common/src/Util.ts b/packages/common/src/Util.ts index aa1e606f1..02f4e2075 100644 --- a/packages/common/src/Util.ts +++ b/packages/common/src/Util.ts @@ -299,9 +299,4 @@ export default class Util { // at the end of everything, just defer to localCompare return ("" + a).localeCompare("" + b); } - - public static isNumber(value: string): boolean { - const num = parseFloat(value); - return Number.isNaN(num) ? false : true; - } } diff --git a/packages/portal/backend/src/server/common/CSVParser.ts b/packages/portal/backend/src/server/common/CSVParser.ts index 846276c00..976c2f557 100644 --- a/packages/portal/backend/src/server/common/CSVParser.ts +++ b/packages/portal/backend/src/server/common/CSVParser.ts @@ -92,7 +92,6 @@ export class CSVParser { const gc = new GradesController(); const gradePromises: Array> = []; let errorMessage = ""; - let unknownId = ""; for (const row of data) { // this check could probably be outside the loop, but since it throws @@ -113,7 +112,7 @@ export class CSVParser { } else { Log.warn("CSVParser::processGrades(..) - Unknown GITHUB: " + row.GITHUB); if (errorMessage === "") { - "Unknwon GitHub ids: "; + errorMessage = "Unknown ids: "; } errorMessage += row.GITHUB + ", "; } @@ -128,7 +127,7 @@ export class CSVParser { } else { Log.warn("CSVParser::processGrades(..) - Unknown CWL: " + row.CWL); if (errorMessage === "") { - "Unknwon CWLs: "; + errorMessage = "Unknown ids: "; } errorMessage += row.CWL + ", "; } @@ -154,7 +153,7 @@ export class CSVParser { } const custom: any = {}; - if (Util.isNumber(gradeScore) === true) { + if (Util.isNumeric(gradeScore) === true) { gradeScore = parseFloat(gradeScore); Log.trace("CSVParser::processGrades(..) - grade is a number: " + gradeScore); } else { @@ -182,10 +181,10 @@ export class CSVParser { gradePromises.push(gc.saveGrade(g)); } else { Log.warn("CSVParser::processGrades(..) - record ignored for: " + personId + "; unknown personId"); - if (unknownId === "") { - unknownId = "Unknown personIds: "; + if (errorMessage === "") { + errorMessage = "Unknown ids: "; } - unknownId += personId + ", "; + errorMessage += personId + ", "; } } else { @@ -204,12 +203,9 @@ export class CSVParser { if (errorMessage.endsWith(", ")) { errorMessage.slice(0, -2); } - if (unknownId.endsWith(", ")) { - unknownId.slice(0, -2); - } - if (errorMessage.length > 0 || unknownId.length > 0) { - const msg = "CSVParser::processGrades(..) - ERROR: " + errorMessage + "; ID: " + unknownId; + if (errorMessage.length > 0) { + const msg = "CSVParser::processGrades(..) - ERROR: " + errorMessage; Log.error(msg); throw new Error(msg); } diff --git a/packages/portal/frontend/src/app/util/SortableTable.ts b/packages/portal/frontend/src/app/util/SortableTable.ts index 29f328423..ed199afbc 100644 --- a/packages/portal/frontend/src/app/util/SortableTable.ts +++ b/packages/portal/frontend/src/app/util/SortableTable.ts @@ -235,18 +235,21 @@ export class SortableTable { if (typeof aVal === "string" && typeof bVal === "string") { const OPTIONS = [ + "exceeding", // remove later "extending", "proficient", "developing", "acquiring", "beginning", - "n/a"]; + "n/a", + "na" // remove later + ]; const aIndex = OPTIONS.indexOf(aVal.toLowerCase()); const bIndex = OPTIONS.indexOf(bVal.toLowerCase()); if (aIndex > -1 && bIndex > -1) { - return Util.compare(aVal, bVal) * mult; + return Util.compare(aIndex, bIndex) * mult; } } From d6327df22caeb5ea1798950826f2fadfdb526ab1 Mon Sep 17 00:00:00 2001 From: Reid Holmes Date: Wed, 9 Oct 2024 07:37:28 -0700 Subject: [PATCH 43/80] more table sorting --- .../frontend/src/app/util/SortableTable.ts | 22 ++++++++++--------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/packages/portal/frontend/src/app/util/SortableTable.ts b/packages/portal/frontend/src/app/util/SortableTable.ts index ed199afbc..a400668a8 100644 --- a/packages/portal/frontend/src/app/util/SortableTable.ts +++ b/packages/portal/frontend/src/app/util/SortableTable.ts @@ -234,19 +234,21 @@ export class SortableTable { const bVal = b[sortIndex].value; if (typeof aVal === "string" && typeof bVal === "string") { - const OPTIONS = [ - "exceeding", // remove later - "extending", - "proficient", - "developing", - "acquiring", - "beginning", + const BUCKET_ORDER = [ + "", + " ", + "na", "n/a", - "na" // remove later + "beginning", + "acquiring", + "developing", + "proficient", + "extending", + "exceeding" ]; - const aIndex = OPTIONS.indexOf(aVal.toLowerCase()); - const bIndex = OPTIONS.indexOf(bVal.toLowerCase()); + const aIndex = BUCKET_ORDER.indexOf(aVal.toLowerCase()); + const bIndex = BUCKET_ORDER.indexOf(bVal.toLowerCase()); if (aIndex > -1 && bIndex > -1) { return Util.compare(aIndex, bIndex) * mult; From 9ac65380d687c1928a4506704eb30cd7166d6fa9 Mon Sep 17 00:00:00 2001 From: Reid Holmes Date: Wed, 9 Oct 2024 07:45:48 -0700 Subject: [PATCH 44/80] see #114; forgot an assignement after slice --- packages/portal/backend/src/server/common/CSVParser.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/portal/backend/src/server/common/CSVParser.ts b/packages/portal/backend/src/server/common/CSVParser.ts index 976c2f557..77c42afc9 100644 --- a/packages/portal/backend/src/server/common/CSVParser.ts +++ b/packages/portal/backend/src/server/common/CSVParser.ts @@ -201,7 +201,7 @@ export class CSVParser { Log.info("CSVParser::processGrades(..) - done; # grades: " + grades.length); if (errorMessage.endsWith(", ")) { - errorMessage.slice(0, -2); + errorMessage = errorMessage.slice(0, -2); } if (errorMessage.length > 0) { From 7025aac100716a1d0302b60798be2558b7c1c3b4 Mon Sep 17 00:00:00 2001 From: Reid Holmes Date: Fri, 11 Oct 2024 07:43:43 -0700 Subject: [PATCH 45/80] improve admin teams tab usability by having more flexibility for default deliv --- .../frontend/src/app/views/AdminTeamsTab.ts | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/packages/portal/frontend/src/app/views/AdminTeamsTab.ts b/packages/portal/frontend/src/app/views/AdminTeamsTab.ts index ba2d40442..9ced66ec0 100644 --- a/packages/portal/frontend/src/app/views/AdminTeamsTab.ts +++ b/packages/portal/frontend/src/app/views/AdminTeamsTab.ts @@ -1,6 +1,12 @@ import Log from "@common/Log"; -import {CourseTransport, RepositoryTransport, StudentTransport, TeamTransport, TeamTransportPayload} from "@common/types/PortalTypes"; +import { + CourseTransport, + RepositoryTransport, + StudentTransport, + TeamTransport, + TeamTransportPayload +} from "@common/types/PortalTypes"; import {SortableTable, TableCell, TableHeader} from "../util/SortableTable"; import {UI} from "../util/UI"; @@ -49,8 +55,19 @@ export class AdminTeamsTab extends AdminPage { if (typeof opts.delivId === "undefined") { const defaultDelivProvisions = provisionDelivs .some((deliv) => deliv.id === this.course.defaultDeliverableId); + + const projectProvisions = provisionDelivs + .some((deliv) => deliv.id === "project"); + + // try to choose a sensible default when the tab opens + // 1) if the current default deliverable provisions + // 2) if the project provisions + // 3) nothing + if (defaultDelivProvisions) { opts.delivId = this.course.defaultDeliverableId; + } else if (projectProvisions) { + opts.delivId = "project"; } else { opts.delivId = "-None-"; } From 754254d248e50ec55fa1bd88800581fd498fdbd5 Mon Sep 17 00:00:00 2001 From: Reid Holmes Date: Fri, 11 Oct 2024 08:09:34 -0700 Subject: [PATCH 46/80] improved usability for dashboard and result tabs --- packages/portal/frontend/src/app/views/AdminDashboardTab.ts | 6 +++++- packages/portal/frontend/src/app/views/AdminResultsTab.ts | 6 +++++- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/packages/portal/frontend/src/app/views/AdminDashboardTab.ts b/packages/portal/frontend/src/app/views/AdminDashboardTab.ts index 6470250bd..41a04bb91 100644 --- a/packages/portal/frontend/src/app/views/AdminDashboardTab.ts +++ b/packages/portal/frontend/src/app/views/AdminDashboardTab.ts @@ -98,7 +98,11 @@ export class AdminDashboardTab extends AdminPage { let delivNames: string[] = []; for (const deliv of delivs) { - delivNames.push(deliv.id); + if (deliv.shouldAutoTest === true) { + // dash results are only available for deliverables that + // use autotest, so skipp adding to dropdown otherwise + delivNames.push(deliv.id); + } } delivNames = delivNames.sort(); delivNames.unshift("-Any-"); diff --git a/packages/portal/frontend/src/app/views/AdminResultsTab.ts b/packages/portal/frontend/src/app/views/AdminResultsTab.ts index b9bb2118e..310589fac 100644 --- a/packages/portal/frontend/src/app/views/AdminResultsTab.ts +++ b/packages/portal/frontend/src/app/views/AdminResultsTab.ts @@ -222,7 +222,11 @@ export class AdminResultsTab extends AdminPage { let delivNames: string[] = []; for (const deliv of delivs) { - delivNames.push(deliv.id); + // only add a deliv if it uses AutoTest + // or it will never have results to render + if (deliv.shouldAutoTest === true) { + delivNames.push(deliv.id); + } } delivNames = delivNames.sort(); delivNames.unshift("-Any-"); From f7458de5967974e10008e7cc22e064a6da9b290f Mon Sep 17 00:00:00 2001 From: Reid Holmes Date: Fri, 11 Oct 2024 08:19:41 -0700 Subject: [PATCH 47/80] fix e2e test broken by csv improvements --- packages/portal/backend/test/server/AdminRoutesSpec.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/portal/backend/test/server/AdminRoutesSpec.ts b/packages/portal/backend/test/server/AdminRoutesSpec.ts index 883469c7b..c86d9ab2f 100644 --- a/packages/portal/backend/test/server/AdminRoutesSpec.ts +++ b/packages/portal/backend/test/server/AdminRoutesSpec.ts @@ -742,7 +742,7 @@ describe("Admin Routes", function () { expect(response.status).to.equal(400); expect(body.failure).to.not.be.undefined; expect(body.failure.message).to.be.an("string"); // test column missing - expect(body.failure.message).to.contain("column missing"); + expect(body.failure.message).to.contain("Grades upload unsuccessful"); response = await request(app).post(url).attach("gradelist", __dirname + "/../data/gradesEmpty.csv").set({ user: userName, From 1829e0a675baca5c2fc77be3c41bf4fb165c71c5 Mon Sep 17 00:00:00 2001 From: Reid Holmes Date: Sun, 13 Oct 2024 09:26:56 -0700 Subject: [PATCH 48/80] See #115, allow more verbose feedback for requestFeedbackDelay (aka add .fullMessage param) --- packages/autotest/src/autotest/ClassPortal.ts | 10 ++++++---- packages/autotest/src/github/GitHubAutoTest.ts | 16 ++++++++++++---- .../backend/src/controllers/CourseController.ts | 6 ++++-- 3 files changed, 22 insertions(+), 10 deletions(-) diff --git a/packages/autotest/src/autotest/ClassPortal.ts b/packages/autotest/src/autotest/ClassPortal.ts index d9c76c942..5240fda96 100644 --- a/packages/autotest/src/autotest/ClassPortal.ts +++ b/packages/autotest/src/autotest/ClassPortal.ts @@ -128,7 +128,7 @@ export interface IClassPortal { * @param timestamp */ requestFeedbackDelay(delivId: string, personId: string, timestamp: number): - Promise<{ accepted: boolean, message: string } | null>; + Promise<{ accepted: boolean, message: string, fullMessage?: string } | null>; } /** @@ -453,7 +453,8 @@ export class ClassPortal implements IClassPortal { public async requestFeedbackDelay(delivId: string, personId: string, timestamp: number): Promise<{ accepted: boolean; - message: string + message: string; + fullMessage?: string; } | null> { const url = `${this.host}:${this.port}/portal/at/feedbackDelay`; @@ -462,7 +463,7 @@ export class ClassPortal implements IClassPortal { Log.info(`ClassPortal::requestFeedbackDelay(..) - start; person: ${personId}; delivId: ${delivId}`); // default to null - let resp: { accepted: boolean, message: string } | null = null; + let resp: { accepted: boolean, message: string, fullMessage: string } | null = null; try { const opts: RequestInit = { @@ -494,7 +495,8 @@ export class ClassPortal implements IClassPortal { if (typeof json?.accepted === "boolean" && typeof json?.message === "string") { resp = { accepted: json.accepted, - message: json.message + message: json.message, + fullMessage: json.fullMessage }; } else if (typeof json?.notImplemented === "boolean" && json.notImplemented === true) { resp = null; diff --git a/packages/autotest/src/github/GitHubAutoTest.ts b/packages/autotest/src/github/GitHubAutoTest.ts index 8fd01c252..cd5af915a 100644 --- a/packages/autotest/src/github/GitHubAutoTest.ts +++ b/packages/autotest/src/github/GitHubAutoTest.ts @@ -691,9 +691,15 @@ export class GitHubAutoTest extends AutoTest implements IGitHubTestManager { Log.info("GitHubAutoTest::requestFeedbackDelay( " + userName + " ) - custom done; can request feedback"); return null; } else { - Log.info("GitHubAutoTest::requestFeedbackDelay( " + userName + " ) - custom done; can NOT request feedback: " + - feedbackDelay.message); - return feedbackDelay.message; + Log.info("GitHubAutoTest::requestFeedbackDelay( " + userName + " ) - custom done; can NOT request feedback; message: " + + feedbackDelay.message + "; fullMessage: " + feedbackDelay?.fullMessage); + let msg = ""; + if (typeof feedbackDelay.fullMessage !== "undefined") { + msg = feedbackDelay.fullMessage; + } else { + msg = "You must wait " + feedbackDelay + " before requesting feedback."; + } + return msg; } } else { Log.info( @@ -712,7 +718,9 @@ export class GitHubAutoTest extends AutoTest implements IGitHubTestManager { const msg = Util.tookHuman(reqTimestamp, nextTimeslot); Log.info("GitHubAutoTest::requestFeedbackDelay( " + userName + " ) - default done; NOT enough time passed, delay: " + msg); - return msg; + + const delayMsg = "You must wait " + msg + " before requesting feedback."; + return delayMsg; } } } diff --git a/packages/portal/backend/src/controllers/CourseController.ts b/packages/portal/backend/src/controllers/CourseController.ts index 65c15cbcb..3e04148b9 100644 --- a/packages/portal/backend/src/controllers/CourseController.ts +++ b/packages/portal/backend/src/controllers/CourseController.ts @@ -77,7 +77,8 @@ export interface ICourseController { requestFeedbackDelay(info: { delivId: string; personId: string; timestamp: number }): Promise<{ accepted: boolean, - message: string + message: string, + fullMessage?: string } | null>; /** @@ -229,7 +230,8 @@ export class CourseController implements ICourseController { public async requestFeedbackDelay(info: { delivId: string; personId: string; timestamp: number }): Promise<{ accepted: boolean, - message: string + message: string, + fullMessage?: string } | null> { Log.warn(`CourseController::requestFeedbackDelay(${info}) - Default impl; returning null`); return null; From bff6327cdb45d961347dbba3b2c120bf62ecccab Mon Sep 17 00:00:00 2001 From: Reid Holmes Date: Sun, 13 Oct 2024 10:09:02 -0700 Subject: [PATCH 49/80] see #115 --- packages/autotest/src/github/GitHubAutoTest.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/autotest/src/github/GitHubAutoTest.ts b/packages/autotest/src/github/GitHubAutoTest.ts index cd5af915a..be3661026 100644 --- a/packages/autotest/src/github/GitHubAutoTest.ts +++ b/packages/autotest/src/github/GitHubAutoTest.ts @@ -442,7 +442,8 @@ export class GitHubAutoTest extends AutoTest implements IGitHubTestManager { Log.info("GitHubAutoTest::handleCommentStudent(..) - too early for: " + target.personId + "; must wait: " + feedbackDelay + "; SHA: " + Util.shaHuman(target.commitURL)); // NOPE, not yet (this is the most common case; feedback requested without time constraints) - const msg = "You must wait " + feedbackDelay + " before requesting feedback."; + // const msg = "You must wait " + feedbackDelay + " before requesting feedback."; + const msg = feedbackDelay; // msg now fully formatted in requestFeedbackDelay await this.postToGitHub(target, {url: target.postbackURL, message: msg}); } else if (previousRequest !== null) { Log.info("GitHubAutoTest::handleCommentStudent(..) - feedback previously given for: " + From ca14038a5c6abeb01f45317074048e88509fa3d2 Mon Sep 17 00:00:00 2001 From: Reid Holmes Date: Tue, 15 Oct 2024 19:37:53 -0700 Subject: [PATCH 50/80] better fullmessage tracking (#115) --- packages/autotest/src/autotest/ClassPortal.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/packages/autotest/src/autotest/ClassPortal.ts b/packages/autotest/src/autotest/ClassPortal.ts index 5240fda96..a08bb0b64 100644 --- a/packages/autotest/src/autotest/ClassPortal.ts +++ b/packages/autotest/src/autotest/ClassPortal.ts @@ -490,9 +490,10 @@ export class ClassPortal implements IClassPortal { Log.trace("ClassPortal::requestFeedbackDelay(..) - types; json: " + typeof json + "; json.accepted: " + typeof json?.accepted + "; json.message: " + typeof json?.message + - "; json.notImplemented: " + typeof json?.notImplemented); + "; json.fullMessage: " + typeof json?.fullMessage); - if (typeof json?.accepted === "boolean" && typeof json?.message === "string") { + if (typeof json?.accepted === "boolean" && + (typeof json?.message === "string" || typeof json?.fullMessage === "string")) { resp = { accepted: json.accepted, message: json.message, From f1fa9f2ecda940fba6266e3ec824a6275d1b9982 Mon Sep 17 00:00:00 2001 From: Reid Holmes Date: Thu, 17 Oct 2024 12:17:43 -0700 Subject: [PATCH 51/80] expose settabvisibility (and let it make tabs visible, not just invisible) --- packages/portal/frontend/src/app/views/AdminView.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/portal/frontend/src/app/views/AdminView.ts b/packages/portal/frontend/src/app/views/AdminView.ts index 2ab82bfb3..56f8356c4 100644 --- a/packages/portal/frontend/src/app/views/AdminView.ts +++ b/packages/portal/frontend/src/app/views/AdminView.ts @@ -108,11 +108,13 @@ export class AdminView implements IView { } } - private setTabVisibility(name: string, visible: boolean) { + protected setTabVisibility(name: string, visible: boolean) { const e = document.getElementById(name); if (e !== null) { if (visible === false) { e.style.display = "none"; + } else { + e.style.display = ""; } } else { Log.warn("AdminView::setTabVisibility( " + name + ", " + visible + " ) - tab not found"); From b045f4f16b36091bda4814c4362c132a0fc47e34 Mon Sep 17 00:00:00 2001 From: Reid Holmes Date: Fri, 18 Oct 2024 10:35:16 -0700 Subject: [PATCH 52/80] see #116 provide mechanism for results to contribute custom fields back to grades. CustomCourseController::convertGrades can then use this to generate course-specific grade renderings, if neeed. Controller::conver --- packages/autotest/src/autotest/AutoTest.ts | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/packages/autotest/src/autotest/AutoTest.ts b/packages/autotest/src/autotest/AutoTest.ts index b3e8b1569..4ea9f6947 100644 --- a/packages/autotest/src/autotest/AutoTest.ts +++ b/packages/autotest/src/autotest/AutoTest.ts @@ -644,6 +644,14 @@ export abstract class AutoTest implements IAutoTest { timestamp: input.target.timestamp, custom: {} }; + // provide a way for the grade controller to contribute data directly + // to the grade record + if (record?.output?.custom) { + gradePayload.custom.output = record.output.custom; + } + if (record?.output?.report?.custom) { + gradePayload.custom.result = record.output.report.custom; + } } catch (err) { Log.error("AutoTest::handleTick(..) - ERROR in execution for SHA: " + input.target.commitSHA + "; ERROR: " + err); } finally { From 2ed439f47844547374290130d2b00606455a5bc6 Mon Sep 17 00:00:00 2001 From: reid holmes Date: Fri, 18 Oct 2024 13:37:02 -0700 Subject: [PATCH 53/80] minor tweak --- packages/autotest/src/autotest/AutoTest.ts | 2 +- packages/autotest/src/autotest/GradingJob.ts | 2 +- packages/autotest/src/github/GitHubUtil.ts | 2 +- packages/portal/backend/src/controllers/CourseController.ts | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/autotest/src/autotest/AutoTest.ts b/packages/autotest/src/autotest/AutoTest.ts index 4ea9f6947..3fd7574ea 100644 --- a/packages/autotest/src/autotest/AutoTest.ts +++ b/packages/autotest/src/autotest/AutoTest.ts @@ -627,7 +627,7 @@ export abstract class AutoTest implements IAutoTest { "; repo: " + input.target.repoId + "; SHA: " + Util.shaHuman(input.target.commitSHA)); let score = -1; - if (record.output.report !== null && typeof record.output.report.scoreOverall !== "undefined") { + if (record?.output?.report?.scoreOverall) { score = record.output.report.scoreOverall; } const githubHost = Config.getInstance().getProp(ConfigKey.githubHost); diff --git a/packages/autotest/src/autotest/GradingJob.ts b/packages/autotest/src/autotest/GradingJob.ts index 92824d4bb..ed962654e 100644 --- a/packages/autotest/src/autotest/GradingJob.ts +++ b/packages/autotest/src/autotest/GradingJob.ts @@ -161,7 +161,7 @@ export class GradingJob { } else if (reportRead === false) { Log.warn("GradingJob::run() - No grading report for repo: " + this.input.target.repoId + "; delivId: " + this.input.target.delivId + "; SHA: " + Util.shaHuman(this.input.target.commitSHA)); - out.report.feedback = "Failed to read grade report. Make a new commit and try again."; + out.report.feedback = "Failed to read grade report for **`#" + this.input.target.delivId + "`**. Make a new commit and try again."; out.report.result = ContainerState.NO_REPORT; out.state = ContainerState.NO_REPORT; } else { diff --git a/packages/autotest/src/github/GitHubUtil.ts b/packages/autotest/src/github/GitHubUtil.ts index c4c404436..c07fe8c92 100644 --- a/packages/autotest/src/github/GitHubUtil.ts +++ b/packages/autotest/src/github/GitHubUtil.ts @@ -309,7 +309,7 @@ export class GitHubUtil { const timestamp = Date.now(); // it does not matter when the work was done, what matters is when it was submitted const backendConfig = await portal.getConfiguration(); if (backendConfig === null) { - Log.warn("GitHubUtil::processComment() - no default deliverable for course"); + Log.warn("GitHubUtil::processPush() - no default deliverable for course"); return null; } diff --git a/packages/portal/backend/src/controllers/CourseController.ts b/packages/portal/backend/src/controllers/CourseController.ts index 3e04148b9..197c24baa 100644 --- a/packages/portal/backend/src/controllers/CourseController.ts +++ b/packages/portal/backend/src/controllers/CourseController.ts @@ -36,7 +36,7 @@ export interface ICourseController { * saved? The Deliverable is included in case due dates want to be considered. The * Grade timestamp is the timestamp of the GitHub push event, not the commit event, * as this is the only time we can guarantee was not tampered with on the client side. - * This will be called once-per-teammember if there are multiple people on the repo + * This will be called once-per-teammate if there are multiple people on the repo * receiving the grade. * * @param {Deliverable} deliv From bc90766a692b66be036cee865419673a7b279648 Mon Sep 17 00:00:00 2001 From: Reid Holmes Date: Wed, 23 Oct 2024 07:55:57 -0700 Subject: [PATCH 54/80] better instrumentation in stdio.html --- packages/portal/frontend/html/stdio.html | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/packages/portal/frontend/html/stdio.html b/packages/portal/frontend/html/stdio.html index de5da16b1..5b55c9574 100644 --- a/packages/portal/frontend/html/stdio.html +++ b/packages/portal/frontend/html/stdio.html @@ -18,7 +18,7 @@
Container Output Viewer
- Admin View + Admin View
@@ -40,6 +40,8 @@ var HEADERS = {}; function getHTTP(url, headers, onsuccess, onerror) { + console.log("stdio.html::onLoad - url: " + url); + var request = new XMLHttpRequest(); request.open('GET', url, true); @@ -75,6 +77,7 @@ function loadStdio(delivId, repoId, sha, stdioType) { const url = `${HOST}/portal/resource/${sha}-${delivId}/${stdioType}/stdio.txt`; + console.log("stdio.html::loadStdio(..) - url: " + url); getHTTP(url, HEADERS, function (stdio) { editor.setValue(stdio); editor.clearSelection(); @@ -137,6 +140,8 @@ function loadNotPassingTestNames(delivId, repoId, sha) { const url = `${HOST}/portal/admin/dashboard/${delivId}/${repoId}`; + + console.log("stdio.html::loadNotPassingTestNames(..) - url: " + url); getHTTP(url, HEADERS, function (data) { json = JSON.parse(data); var commit = json['success'] @@ -175,15 +180,18 @@ // switchViewBtn: only enable the admin/staff stdio switch btn when the user is an admin var switchViewBtn = document.getElementById('switchViewBtn'); + function handleSwitchStdio() { switchViewBtn.addEventListener('click', (event) => { var btnLabel = event.target.innerText; editor.setValue('Loading...'); if (btnLabel.startsWith('Admin')) { + console.log("stdio.html::handleSwitchStdio(..) - switch to staff view"); event.target.innerText = 'Staff View'; loadStdio(delivId, repoId, sha, 'admin'); loadNotPassingTestNames(delivId, repoId, sha); } else { + console.log("stdio.html::handleSwitchStdio(..) - switch to admin view"); event.target.innerText = 'Admin View'; loadStdio(delivId, repoId, sha, 'staff'); loadNotPassingTestNames(delivId, repoId, sha); From 0b1243e77b8dfe41b805376f7c60a87a8d4e3d8d Mon Sep 17 00:00:00 2001 From: Reid Holmes Date: Wed, 23 Oct 2024 08:10:06 -0700 Subject: [PATCH 55/80] add simplify view for stdio --- packages/portal/frontend/html/stdio.html | 66 +++++++++++++++++++++++- 1 file changed, 65 insertions(+), 1 deletion(-) diff --git a/packages/portal/frontend/html/stdio.html b/packages/portal/frontend/html/stdio.html index 5b55c9574..1cf41b05c 100644 --- a/packages/portal/frontend/html/stdio.html +++ b/packages/portal/frontend/html/stdio.html @@ -75,6 +75,70 @@ return vars; } + function simplifyStdio(stdio) { + let toReturn = ""; + + try { + // Every line that includes AssertionError: + // Every line that includes compareJSONArray + // Every line that includes Error: Timeout of (but truncate anything after (... since this returns our path names) + // Every line that includes ENOENT + let LINES_INCLUDE = [ + "AssertionError", + "compareJSONArray", + "Error: Timeout of", + "ENOENT" + ]; + + let lines = stdio.split('\n'); + for (let line of lines) { + for (let match of LINES_INCLUDE) { + if (line.includes(match)) { + // TODO: handle special case for Timeout of (see comment above) + toReturn += line + '\n'; + } + } + } + + // Everything before (and including) ProjectGrader::grade(..) - start + let preamble = ""; + if (stdio.contains("ProjectGrader::grade(..) - start")) { + let index = stdio.indexOf("ProjectGrader::grade(..) - start"); + preamble = stdio.substring(0, index); + } + toReturn = preamble + toReturn; + + // Everything after (and including) ProjectGrader::grade(..) - done + let postfix = ""; + if (stdio.contains("ProjectGrader::grade(..) - done")) { + let index = stdio.indexOf("ProjectGrader::grade(..) - done"); + postfix = stdio.substring(index); + } + toReturn = toReturn + postfix; + } catch (err) { + console.log("Error in simplifyStdio: " + err); + toReturn = stdio; + } + return toReturn; + } + + function loadAdminStdio(delivId, repoId, sha) { + const url = `${HOST}/portal/resource/${sha}-${delivId}/staff/stdio.txt`; + // admin stdio.txt does not exist, instead render a custom view here + console.log("stdio.html::loadAdminStdio(..) - url: " + url); + getHTTP(url, HEADERS, function (stdio) { + stdio = simplifyStdio(stdio); + editor.setValue(stdio); + editor.clearSelection(); + editor.focus(); // manually set the focus so that CMD/CTRL+F will search within the stdio + }, function (error) { + if (typeof error !== 'string') { + error = 'The request failed, possibly due to unauthorization.'; + } + alert(error); + }); + } + function loadStdio(delivId, repoId, sha, stdioType) { const url = `${HOST}/portal/resource/${sha}-${delivId}/${stdioType}/stdio.txt`; console.log("stdio.html::loadStdio(..) - url: " + url); @@ -188,7 +252,7 @@ if (btnLabel.startsWith('Admin')) { console.log("stdio.html::handleSwitchStdio(..) - switch to staff view"); event.target.innerText = 'Staff View'; - loadStdio(delivId, repoId, sha, 'admin'); + loadAdminStdio(delivId, repoId, sha); loadNotPassingTestNames(delivId, repoId, sha); } else { console.log("stdio.html::handleSwitchStdio(..) - switch to admin view"); From 837cb639c8d52dfb50e768398e79f74bcf3c93d8 Mon Sep 17 00:00:00 2001 From: Reid Holmes Date: Wed, 23 Oct 2024 08:18:17 -0700 Subject: [PATCH 56/80] use indexOf consistently --- packages/portal/frontend/html/stdio.html | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/packages/portal/frontend/html/stdio.html b/packages/portal/frontend/html/stdio.html index 1cf41b05c..9e700434d 100644 --- a/packages/portal/frontend/html/stdio.html +++ b/packages/portal/frontend/html/stdio.html @@ -93,7 +93,7 @@ let lines = stdio.split('\n'); for (let line of lines) { for (let match of LINES_INCLUDE) { - if (line.includes(match)) { + if (line.indexOf(match) >= 0) { // TODO: handle special case for Timeout of (see comment above) toReturn += line + '\n'; } @@ -102,21 +102,21 @@ // Everything before (and including) ProjectGrader::grade(..) - start let preamble = ""; - if (stdio.contains("ProjectGrader::grade(..) - start")) { - let index = stdio.indexOf("ProjectGrader::grade(..) - start"); - preamble = stdio.substring(0, index); + let prefixIndex = stdio.indexOf("ProjectGrader::grade(..) - start") + if (prefixIndex >= 0) { + preamble = stdio.substring(0, prefixIndex); } toReturn = preamble + toReturn; // Everything after (and including) ProjectGrader::grade(..) - done let postfix = ""; - if (stdio.contains("ProjectGrader::grade(..) - done")) { - let index = stdio.indexOf("ProjectGrader::grade(..) - done"); - postfix = stdio.substring(index); + let postfixIndex = stdio.indexOf("ProjectGrader::grade(..) - done") + if ( postfixIndex >= 0) { + postfix = stdio.substring(postfixIndex, stdio.length); } toReturn = toReturn + postfix; } catch (err) { - console.log("Error in simplifyStdio: " + err); + console.error("stdio::simplifyStdio(..) - ERROR: " + err); toReturn = stdio; } return toReturn; From d1619de32126e212365a9d001017ccffbf6717fc Mon Sep 17 00:00:00 2001 From: Reid Holmes Date: Wed, 23 Oct 2024 08:27:51 -0700 Subject: [PATCH 57/80] change prefix/postfix to more inclusive keys --- packages/portal/frontend/html/stdio.html | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/packages/portal/frontend/html/stdio.html b/packages/portal/frontend/html/stdio.html index 9e700434d..61c3e1cd2 100644 --- a/packages/portal/frontend/html/stdio.html +++ b/packages/portal/frontend/html/stdio.html @@ -100,15 +100,19 @@ } } - // Everything before (and including) ProjectGrader::grade(..) - start + // Everything before (and including) + // ProjectGrader::grade(..) - start + // SimpleGrader::doGrading() - start let preamble = ""; - let prefixIndex = stdio.indexOf("ProjectGrader::grade(..) - start") + let prefixIndex = stdio.indexOf("SimpleGrader::doGrading() - start") if (prefixIndex >= 0) { preamble = stdio.substring(0, prefixIndex); } toReturn = preamble + toReturn; - // Everything after (and including) ProjectGrader::grade(..) - done + // Everything after (and including) + // ProjectGrader::grade(..) - done + // SimpleGrader::doGrading() - done let postfix = ""; let postfixIndex = stdio.indexOf("ProjectGrader::grade(..) - done") if ( postfixIndex >= 0) { From 33fe74d2218dcc477476f2c7070606539bb29933 Mon Sep 17 00:00:00 2001 From: Reid Holmes Date: Wed, 23 Oct 2024 08:35:27 -0700 Subject: [PATCH 58/80] update matching --- packages/portal/frontend/html/stdio.html | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/packages/portal/frontend/html/stdio.html b/packages/portal/frontend/html/stdio.html index 61c3e1cd2..a5a05655f 100644 --- a/packages/portal/frontend/html/stdio.html +++ b/packages/portal/frontend/html/stdio.html @@ -104,9 +104,10 @@ // ProjectGrader::grade(..) - start // SimpleGrader::doGrading() - start let preamble = ""; - let prefixIndex = stdio.indexOf("SimpleGrader::doGrading() - start") + const PREFIX = "SimpleGrader::doGrading() - start"; + let prefixIndex = stdio.indexOf(PREFIX) if (prefixIndex >= 0) { - preamble = stdio.substring(0, prefixIndex); + preamble = stdio.substring(0, prefixIndex + PREFIX.length); } toReturn = preamble + toReturn; @@ -114,8 +115,9 @@ // ProjectGrader::grade(..) - done // SimpleGrader::doGrading() - done let postfix = ""; - let postfixIndex = stdio.indexOf("ProjectGrader::grade(..) - done") - if ( postfixIndex >= 0) { + const POSTFIX = "SimpleGrader::doGrading() - done"; + let postfixIndex = stdio.indexOf(POSTFIX) + if (postfixIndex >= 0) { postfix = stdio.substring(postfixIndex, stdio.length); } toReturn = toReturn + postfix; From 95024879ad1056b913770bd4e7d85b6cce2dac6b Mon Sep 17 00:00:00 2001 From: Reid Holmes Date: Wed, 23 Oct 2024 08:45:18 -0700 Subject: [PATCH 59/80] better separation for start/end events --- packages/portal/frontend/html/stdio.html | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/portal/frontend/html/stdio.html b/packages/portal/frontend/html/stdio.html index a5a05655f..4d5316175 100644 --- a/packages/portal/frontend/html/stdio.html +++ b/packages/portal/frontend/html/stdio.html @@ -108,6 +108,7 @@ let prefixIndex = stdio.indexOf(PREFIX) if (prefixIndex >= 0) { preamble = stdio.substring(0, prefixIndex + PREFIX.length); + preamble += "\n***GRADING START***\n\n"; } toReturn = preamble + toReturn; @@ -119,6 +120,7 @@ let postfixIndex = stdio.indexOf(POSTFIX) if (postfixIndex >= 0) { postfix = stdio.substring(postfixIndex, stdio.length); + postfix = "\n\n***GRADING DONE***\n" + postfix; } toReturn = toReturn + postfix; } catch (err) { From c07e081efb63be0476d7cbc2232a7f9ca9a564d1 Mon Sep 17 00:00:00 2001 From: Reid Holmes Date: Wed, 23 Oct 2024 08:51:20 -0700 Subject: [PATCH 60/80] add 310 guard, just to be safe --- packages/portal/frontend/html/stdio.html | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/packages/portal/frontend/html/stdio.html b/packages/portal/frontend/html/stdio.html index 4d5316175..9a11df14d 100644 --- a/packages/portal/frontend/html/stdio.html +++ b/packages/portal/frontend/html/stdio.html @@ -135,7 +135,10 @@ // admin stdio.txt does not exist, instead render a custom view here console.log("stdio.html::loadAdminStdio(..) - url: " + url); getHTTP(url, HEADERS, function (stdio) { - stdio = simplifyStdio(stdio); + if (HOST.indexOf("cs310") >= 0) { + console.log("stdio.html::loadAdminStdio(..) - simplify stdio for 310"); + stdio = simplifyStdio(stdio); + } editor.setValue(stdio); editor.clearSelection(); editor.focus(); // manually set the focus so that CMD/CTRL+F will search within the stdio From da08d3bf5baf4749514a3cf2c999616c1de1ac67 Mon Sep 17 00:00:00 2001 From: Reid Holmes Date: Wed, 23 Oct 2024 10:12:11 -0700 Subject: [PATCH 61/80] brief by default --- packages/portal/frontend/html/stdio.html | 25 +++++++++++++++++------- 1 file changed, 18 insertions(+), 7 deletions(-) diff --git a/packages/portal/frontend/html/stdio.html b/packages/portal/frontend/html/stdio.html index 9a11df14d..f8091fc13 100644 --- a/packages/portal/frontend/html/stdio.html +++ b/packages/portal/frontend/html/stdio.html @@ -130,36 +130,47 @@ return toReturn; } - function loadAdminStdio(delivId, repoId, sha) { + function loadStdioBrief(delivId, repoId, sha) { const url = `${HOST}/portal/resource/${sha}-${delivId}/staff/stdio.txt`; // admin stdio.txt does not exist, instead render a custom view here - console.log("stdio.html::loadAdminStdio(..) - url: " + url); + console.log("stdio.html::loadStdioBrief(..) - url: " + url); getHTTP(url, HEADERS, function (stdio) { if (HOST.indexOf("cs310") >= 0) { - console.log("stdio.html::loadAdminStdio(..) - simplify stdio for 310"); + console.log("stdio.html::loadStdioBrief(..) - simplify stdio for 310"); stdio = simplifyStdio(stdio); + } else { + console.log("stdio.html::loadStdioBrief(..) - brief and full are the same for this course"); } editor.setValue(stdio); editor.clearSelection(); editor.focus(); // manually set the focus so that CMD/CTRL+F will search within the stdio }, function (error) { if (typeof error !== 'string') { - error = 'The request failed, possibly due to unauthorization.'; + error = 'The request failed, possibly due to de-authorization.'; } alert(error); }); } function loadStdio(delivId, repoId, sha, stdioType) { - const url = `${HOST}/portal/resource/${sha}-${delivId}/${stdioType}/stdio.txt`; - console.log("stdio.html::loadStdio(..) - url: " + url); + console.log("stdio.html::loadStdio(..) - stdioType: " + stdioType); + if (stdioType === "admin") { + loadStdioFull(delivId, repoId, sha); + } else { + loadStdioBrief(delivId, repoId, sha); + } + } + + function loadStdioFull(delivId, repoId, sha) { + const url = `${HOST}/portal/resource/${sha}-${delivId}/staff/stdio.txt`; + console.log("stdio.html::loadStdioFull(..) - url: " + url); getHTTP(url, HEADERS, function (stdio) { editor.setValue(stdio); editor.clearSelection(); editor.focus(); // manually set the focus so that CMD/CTRL+F will search within the stdio }, function (error) { if (typeof error !== 'string') { - error = 'The request failed, possibly due to unauthorization.'; + error = 'The request failed, possibly due to de-authorization.'; } alert(error); }); From e6c0c3ea78ca518ff399ccef6dbf766b4cadff49 Mon Sep 17 00:00:00 2001 From: Reid Holmes Date: Wed, 23 Oct 2024 10:18:56 -0700 Subject: [PATCH 62/80] update logging --- packages/portal/frontend/html/stdio.html | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/portal/frontend/html/stdio.html b/packages/portal/frontend/html/stdio.html index f8091fc13..c52ee8882 100644 --- a/packages/portal/frontend/html/stdio.html +++ b/packages/portal/frontend/html/stdio.html @@ -274,7 +274,7 @@ if (btnLabel.startsWith('Admin')) { console.log("stdio.html::handleSwitchStdio(..) - switch to staff view"); event.target.innerText = 'Staff View'; - loadAdminStdio(delivId, repoId, sha); + loadStdio(delivId, repoId, sha, 'admin'); loadNotPassingTestNames(delivId, repoId, sha); } else { console.log("stdio.html::handleSwitchStdio(..) - switch to admin view"); From a61fa5fa75a12cd11bad324d60d4f7f275719c71 Mon Sep 17 00:00:00 2001 From: Reid Holmes Date: Wed, 23 Oct 2024 10:26:21 -0700 Subject: [PATCH 63/80] more stdio keywords for highlighting --- packages/portal/frontend/html/stdio.html | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/packages/portal/frontend/html/stdio.html b/packages/portal/frontend/html/stdio.html index c52ee8882..54400249c 100644 --- a/packages/portal/frontend/html/stdio.html +++ b/packages/portal/frontend/html/stdio.html @@ -182,6 +182,11 @@ // add other keywords we want to highlight keywords.push("AssertionError"); keywords.push("TypeError"); + keywords.push("ENOENT"); + keywords.push("compareJSONArrays"); + keywords.push("Timeout"); + keywords.push("expected"); + var oop = require("ace/lib/oop"); var TextMode = require("ace/mode/text").Mode; var Tokenizer = require("ace/tokenizer").Tokenizer; From 5f5b3283c6ce8b4e69a2626a3829b9c3e1c2adc0 Mon Sep 17 00:00:00 2001 From: Reid Holmes Date: Wed, 23 Oct 2024 10:32:05 -0700 Subject: [PATCH 64/80] add one last keyword --- packages/portal/frontend/html/stdio.html | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/portal/frontend/html/stdio.html b/packages/portal/frontend/html/stdio.html index 54400249c..c63c2a621 100644 --- a/packages/portal/frontend/html/stdio.html +++ b/packages/portal/frontend/html/stdio.html @@ -183,6 +183,7 @@ keywords.push("AssertionError"); keywords.push("TypeError"); keywords.push("ENOENT"); + keywords.push("compareJSONArray"); keywords.push("compareJSONArrays"); keywords.push("Timeout"); keywords.push("expected"); From 479d1d942498916f48db0bd1ceb605e9c696ce20 Mon Sep 17 00:00:00 2001 From: reid holmes Date: Mon, 28 Oct 2024 17:14:13 -0700 Subject: [PATCH 65/80] better logging in getPushRecord --- packages/autotest/src/autotest/DataStore.ts | 23 +++++++++++---------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/packages/autotest/src/autotest/DataStore.ts b/packages/autotest/src/autotest/DataStore.ts index f5936b5bc..4c10a8725 100644 --- a/packages/autotest/src/autotest/DataStore.ts +++ b/packages/autotest/src/autotest/DataStore.ts @@ -154,37 +154,38 @@ export class MongoDataStore implements IDataStore { /** * Gets the push event record for a given commitURL. If more than one exist, - * return the one for main/master. If there isn't a main/master push, return the + * return the one for main/master. If there is not a main/master push, return the * most recent one. */ public async getPushRecord(commitURL: string): Promise { Log.trace("MongoDataStore::getPushRecord(..) - start"); try { const start = Date.now(); - let res = await this.getRecords(this.PUSHCOLL, {commitURL: commitURL}); - if (res === null) { + let records = await this.getRecords(this.PUSHCOLL, {commitURL: commitURL}) as CommitTarget[]; + if (records === null) { Log.trace("MongoDataStore::getPushRecord(..) - record not found for: " + commitURL); } else { - Log.trace("MongoDataStore::getPushRecord(..) - found; took: " + Util.took(start)); - if (res.length === 1) { + if (records.length === 1) { + Log.trace("MongoDataStore::getPushRecord(..) - one found; took: " + Util.took(start)); // the usual case - return res[0] as CommitTarget; + return records[0]; } else { // return main/master if exists - for (const r of res as CommitTarget[]) { + for (const r of records) { if (typeof r.ref !== "undefined" && (r.ref === "refs/heads/main" || r.ref === "refs/heads/master")) { - return r as CommitTarget; + Log.trace("MongoDataStore::getPushRecord(..) - multiple found, returning main; took: " + Util.took(start)); + return r; } } // sort, should make the oldest record first - res = res.sort(function (c1: CommitTarget, c2: CommitTarget) { + records = records.sort(function (c1: CommitTarget, c2: CommitTarget) { return c1.timestamp - c2.timestamp; }); - return res[0] as CommitTarget; + Log.trace("MongoDataStore::getPushRecord(..) - multiple found, returning oldest; took: " + Util.took(start)); + return records[0] as CommitTarget; } } - } catch (err) { Log.error("MongoDataStore::getPushRecord(..) - ERROR: " + err); } From 252e3615d871fb975d068416a02cdb9fc3b372f7 Mon Sep 17 00:00:00 2001 From: reid holmes Date: Tue, 29 Oct 2024 14:36:55 -0700 Subject: [PATCH 66/80] see project-resources#557; trying to debug cases where multiple pushes are being received for the same sha. If one of them is on main, we really want to always value those most highly. --- packages/autotest/src/autotest/DataStore.ts | 2 +- .../autotest/src/github/GitHubAutoTest.ts | 37 ++++++++++++++----- packages/autotest/src/github/GitHubUtil.ts | 10 ++++- .../src/server/AutoTestRouteHandler.ts | 1 - .../src/controllers/AdminController.ts | 8 +++- .../src/controllers/DatabaseController.ts | 13 ++++++- .../src/server/common/AutoTestRoutes.ts | 2 +- 7 files changed, 55 insertions(+), 18 deletions(-) diff --git a/packages/autotest/src/autotest/DataStore.ts b/packages/autotest/src/autotest/DataStore.ts index 4c10a8725..b8c790f2d 100644 --- a/packages/autotest/src/autotest/DataStore.ts +++ b/packages/autotest/src/autotest/DataStore.ts @@ -158,7 +158,7 @@ export class MongoDataStore implements IDataStore { * most recent one. */ public async getPushRecord(commitURL: string): Promise { - Log.trace("MongoDataStore::getPushRecord(..) - start"); + // Log.trace("MongoDataStore::getPushRecord(..) - start"); try { const start = Date.now(); let records = await this.getRecords(this.PUSHCOLL, {commitURL: commitURL}) as CommitTarget[]; diff --git a/packages/autotest/src/github/GitHubAutoTest.ts b/packages/autotest/src/github/GitHubAutoTest.ts index be3661026..22b97e318 100644 --- a/packages/autotest/src/github/GitHubAutoTest.ts +++ b/packages/autotest/src/github/GitHubAutoTest.ts @@ -70,9 +70,25 @@ export class GitHubAutoTest extends AutoTest implements IGitHubTestManager { Log.info("GitHubAutoTest::handlePushEvent(..) - " + "repo: " + info.repoId + "; person: " + info.personId + "; SHA: " + Util.shaHuman(info.commitSHA) + "; branch: " + info.ref); - const start = Date.now(); - await this.savePushInfo(info); + // if there are already pushes for this SHA, and any of them are on main/master, use those instead + const prevPush = await this.dataStore.getPushRecord(info.commitURL); + if (prevPush !== null && (prevPush.ref === "refs/heads/main" || prevPush.ref === "refs/heads/master")) { + Log.info("GitHubAutoTest::handlePushEvent(..) - " + + "repo: " + info.repoId + "; person: " + info.personId + + "; SHA: " + Util.shaHuman(info.commitSHA) + "; branch: " + info.ref + + "; using main/master push instead of: " + prevPush.ref); + info = prevPush; + // already saved so just use that instead + } else { + Log.info("GitHubAutoTest::handlePushEvent(..) - " + + "repo: " + info.repoId + "; person: " + info.personId + + "; SHA: " + Util.shaHuman(info.commitSHA) + "; branch: " + info.ref + + "; no main/master push found"); + await this.savePushInfo(info); + } + + const start = Date.now(); if (typeof delivId === "undefined" || delivId === null) { Log.trace("GitHubAutoTest::handlePushEvent(..) - deliv not specified; requesting"); delivId = await this.getDefaultDelivId(); // current default deliverable @@ -512,7 +528,7 @@ export class GitHubAutoTest extends AutoTest implements IGitHubTestManager { } Log.info("GitHubAutoTest::handleCommentEvent( " + info.personId + " ) - start; SHA: " + Util.shaHuman(info.commitSHA)); - Log.trace("GitHubAutoTest::handleCommentEvent(..) - start; comment: " + JSON.stringify(info)); + Log.trace("GitHubAutoTest::handleCommentEvent( " + info.personId + " ) - start; comment: " + JSON.stringify(info)); // sanity check; this keeps the rest of the code much simpler const preconditionsMet = await this.checkCommentPreconditions(info); @@ -553,7 +569,7 @@ export class GitHubAutoTest extends AutoTest implements IGitHubTestManager { // This is helpful for testing student workflows with the queue if (isStaff !== null && (isStaff.isStaff === true || isStaff.isAdmin === true)) { if (typeof info.flags !== "undefined" && info.flags.indexOf("#student") >= 0) { - Log.info("GitHubAutoTest::handleCommentEvent(..) - running admin request as student"); + Log.info("GitHubAutoTest::handleCommentEvent( " + info.personId + " ) - running admin request as student"); isStaff.isStaff = false; isStaff.isAdmin = false; } @@ -561,26 +577,27 @@ export class GitHubAutoTest extends AutoTest implements IGitHubTestManager { if (isStaff !== null && (isStaff.isStaff === true || isStaff.isAdmin === true)) { // staff request - Log.info("GitHubAutoTest::handleCommentEvent(..) - handleAdmin; for: " + - info.personId + "; deliv: " + info.delivId + "; SHA: " + Util.shaHuman(info.commitSHA)); + Log.info( + "GitHubAutoTest::handleCommentEvent( " + info.personId + " ) - handleAdmin; deliv: " + info.delivId + + "; SHA: " + Util.shaHuman(info.commitSHA)); info.adminRequest = true; // set admin request so queues can handle this appropriately if (typeof info.flags !== "undefined" && info.flags.indexOf("#force") >= 0) { - Log.info("GitHubAutoTest::handleCommentEvent(..) - handleAdmin; processing with #force"); + Log.info("GitHubAutoTest::handleCommentEvent( " + info.personId + " ) - handleAdmin; processing with #force"); await this.processComment(info, null); // do not pass the previous result so a new one will be generated } else { await this.processComment(info, res); } } else { // student request - Log.info("GitHubAutoTest::handleCommentEvent(..) - handleStudent; for: " + - info.personId + "; deliv: " + info.delivId + "; SHA: " + Util.shaHuman(info.commitSHA)); + Log.info("GitHubAutoTest::handleCommentEvent( " + info.personId + " ) - handleStudent; deliv: " + info.delivId + + "; SHA: " + Util.shaHuman(info.commitSHA)); info.adminRequest = false; await this.handleCommentStudent(info, res); } // make sure the queues have ticked after the comment has been processed this.tick(); - Log.trace("GitHubAutoTest::handleCommentEvent(..) - done; took: " + Util.took(start)); + Log.trace("GitHubAutoTest::handleCommentEvent( " + info.personId + " ) - done; took: " + Util.took(start)); } protected async processExecution(data: AutoTestResult): Promise { diff --git a/packages/autotest/src/github/GitHubUtil.ts b/packages/autotest/src/github/GitHubUtil.ts index c07fe8c92..07120274e 100644 --- a/packages/autotest/src/github/GitHubUtil.ts +++ b/packages/autotest/src/github/GitHubUtil.ts @@ -292,12 +292,18 @@ export class GitHubUtil { if (typeof payload.commits !== "undefined" && payload.commits.length > 0) { commitSHA = payload.commits[payload.commits.length - 1].id; commitURL = payload.commits[payload.commits.length - 1].url; - Log.trace("GitHubUtil::processPush(..) - regular push; repo: " + repo + "; # commits: " + payload.commits.length); + let shas = ""; + for (const sha of payload.commits) { + shas += Util.shaHuman(sha.id) + " "; + } + shas = shas.trim(); + Log.trace("GitHubUtil::processPush(..) - regular push; repo: " + repo + "; # commits: " + payload.commits.length + + ", all commits: [" + shas + "]"); } else { // use this one when creating a new branch (may not have any commits) commitSHA = payload.head_commit.id; commitURL = payload.head_commit.url; - Log.trace("GitHubUtil::processPush(..) - branch added; repo: " + repo); + Log.trace("GitHubUtil::processPush(..) - branch added; repo: " + repo + "; sha: " + Util.shaHuman(commitSHA)); } Log.info("GitHubUtil::processPush(..) - start; repo: " + repo + diff --git a/packages/autotest/src/server/AutoTestRouteHandler.ts b/packages/autotest/src/server/AutoTestRouteHandler.ts index 7e4f4a676..f7d3c41ca 100644 --- a/packages/autotest/src/server/AutoTestRouteHandler.ts +++ b/packages/autotest/src/server/AutoTestRouteHandler.ts @@ -185,7 +185,6 @@ export default class AutoTestRouteHandler { case "issue_comment": const prEvent = await GitHubUtil.processIssueComment(body); return prEvent; - // no return for now, just fall through to error default: Log.error("AutoTestRouteHandler::handleWebhook() - Unhandled GitHub event: " + event); throw new Error("Unhandled GitHub hook event: " + event); diff --git a/packages/portal/backend/src/controllers/AdminController.ts b/packages/portal/backend/src/controllers/AdminController.ts index 20ddb441f..2a44a1b5d 100644 --- a/packages/portal/backend/src/controllers/AdminController.ts +++ b/packages/portal/backend/src/controllers/AdminController.ts @@ -104,8 +104,12 @@ export class AdminController { Log.trace("AdminController::processNewAutoTestGrade(..) - handling grade for " + personId + "; existingGrade: " + existingGradeScore + "; newGrade: " + newGrade.score); const shouldSave = await cc.handleNewAutoTestGrade(deliv, newGrade, existingGrade); - Log.trace("AdminController::processNewAutoTestGrade(..) - handled grade for " + personId + - "; shouldSave: " + shouldSave); // NOTE: for hangup debugging + // Log.trace("AdminController::processNewAutoTestGrade(..) - handled grade for " + personId + + // "; shouldSave: " + shouldSave); // NOTE: for hangup debugging + + Log.trace( + "AdminController::processNewAutoTestGrade(..) - grade: " + JSON.stringify( + newGrade) + "; repoId: " + grade.repoId + "; shouldSave: " + shouldSave); if (shouldSave === true) { Log.info("AdminController::processNewAutoTestGrade(..) - saving grade for deliv: " diff --git a/packages/portal/backend/src/controllers/DatabaseController.ts b/packages/portal/backend/src/controllers/DatabaseController.ts index 4d06d44a0..9b4f21130 100644 --- a/packages/portal/backend/src/controllers/DatabaseController.ts +++ b/packages/portal/backend/src/controllers/DatabaseController.ts @@ -1003,7 +1003,18 @@ export class DatabaseController { * @returns {Promise} */ public async getResultFromURL(commitURL: string, delivId: string): Promise { - return await this.readSingleRecord(this.RESULTCOLL, {commitURL: commitURL, delivId: delivId}) as Result; + // return await this.readSingleRecord(this.RESULTCOLL, {commitURL: commitURL, delivId: delivId}) as Result; + const records = await this.readRecords(this.RESULTCOLL, QueryKind.FAST, false, + {commitURL: commitURL, delivId: delivId}) as Result[]; + + if (records.length > 1) { + // This should not happen, but we want to know for sure + Log.warn("DatabaseController::getResultFromURL( " + commitURL + ", " + delivId + " ) - multiple results returned"); + } + if (records.length > 0) { + return records[0]; + } + return null; } } diff --git a/packages/portal/backend/src/server/common/AutoTestRoutes.ts b/packages/portal/backend/src/server/common/AutoTestRoutes.ts index 457ca3695..9c83e7096 100644 --- a/packages/portal/backend/src/server/common/AutoTestRoutes.ts +++ b/packages/portal/backend/src/server/common/AutoTestRoutes.ts @@ -185,7 +185,7 @@ export class AutoTestRoutes implements IREST { if (validGradeRecord !== null) { throw new Error("Invalid Grade Record: " + validGradeRecord); } else { - Log.info("AutoTestRoutes::atGrade(..) - deliv: " + grade.delivId + + Log.info("AutoTestRoutes::performPostGrade(..) - deliv: " + grade.delivId + "; repo: " + grade.repoId + "; grade: " + grade.score); // Log.trace("AutoTestRoutes::atGrade(..) - repoId: " + grade.repoId + // "; delivId: " + grade.delivId + "; body: " + JSON.stringify(grade)); From b11ca245c266896f17a32a636cde05265662a6d5 Mon Sep 17 00:00:00 2001 From: reid holmes Date: Tue, 29 Oct 2024 15:21:42 -0700 Subject: [PATCH 67/80] cleanup push event modelling --- packages/autotest/src/autotest/DataStore.ts | 4 +- .../autotest/src/github/GitHubAutoTest.ts | 57 +++++++++---------- packages/autotest/src/github/GitHubUtil.ts | 4 ++ 3 files changed, 34 insertions(+), 31 deletions(-) diff --git a/packages/autotest/src/autotest/DataStore.ts b/packages/autotest/src/autotest/DataStore.ts index b8c790f2d..d4938e46f 100644 --- a/packages/autotest/src/autotest/DataStore.ts +++ b/packages/autotest/src/autotest/DataStore.ts @@ -6,6 +6,7 @@ import {AutoTestResult, IFeedbackGiven} from "@common/types/AutoTestTypes"; import {CommitTarget} from "@common/types/ContainerTypes"; import Util from "@common/Util"; +import {GitHubUtil} from "@autotest/github/GitHubUtil"; export interface IDataStore { @@ -172,7 +173,8 @@ export class MongoDataStore implements IDataStore { } else { // return main/master if exists for (const r of records) { - if (typeof r.ref !== "undefined" && (r.ref === "refs/heads/main" || r.ref === "refs/heads/master")) { + + if (typeof r.ref !== "undefined" && GitHubUtil.isMain(r.ref) === true) { Log.trace("MongoDataStore::getPushRecord(..) - multiple found, returning main; took: " + Util.took(start)); return r; } diff --git a/packages/autotest/src/github/GitHubAutoTest.ts b/packages/autotest/src/github/GitHubAutoTest.ts index 22b97e318..15201d101 100644 --- a/packages/autotest/src/github/GitHubAutoTest.ts +++ b/packages/autotest/src/github/GitHubAutoTest.ts @@ -53,47 +53,46 @@ export class GitHubAutoTest extends AutoTest implements IGitHubTestManager { * @param {string} delivId */ public async handlePushEvent(info: CommitTarget, delivId?: string): Promise { + try { if (typeof info === "undefined" || info === null) { Log.info("GitHubAutoTest::handlePushEvent(..) - skipped; info not provided"); return false; } + const PREAMBLE = "GitHubAutoTest::handlePushEvent( " + Util.shaHuman(info.commitSHA) + ") - "; + const org = Config.getInstance().getProp(ConfigKey.org); - Log.trace("GitHubAutoTest::handlePushEvent(..) - start; org: " + org + "; push org: " + info.orgId); + Log.trace(PREAMBLE + "start; org: " + org + + "; push org: " + info.orgId); if (typeof org !== "undefined" && typeof info.orgId !== "undefined" && org !== info.orgId) { - Log.warn("GitHubAutoTest::handlePushEvent(..) - ignored, org: " + info.orgId + + Log.warn(PREAMBLE + "ignored, org: " + info.orgId + " does not match current course: " + org); return false; } - Log.info("GitHubAutoTest::handlePushEvent(..) - " + - "repo: " + info.repoId + "; person: " + info.personId + - "; SHA: " + Util.shaHuman(info.commitSHA) + "; branch: " + info.ref); + Log.info(PREAMBLE + "repo: " + info.repoId + "; person: " + info.personId + "; branch: " + info.ref); // if there are already pushes for this SHA, and any of them are on main/master, use those instead + // the implication here is that a SHA on a branch will not overwrite a result for a SHA on main/master + // but a SHA on main/master can overwrite a SHA on a branch const prevPush = await this.dataStore.getPushRecord(info.commitURL); - if (prevPush !== null && (prevPush.ref === "refs/heads/main" || prevPush.ref === "refs/heads/master")) { - Log.info("GitHubAutoTest::handlePushEvent(..) - " + - "repo: " + info.repoId + "; person: " + info.personId + - "; SHA: " + Util.shaHuman(info.commitSHA) + "; branch: " + info.ref + - "; using main/master push instead of: " + prevPush.ref); + if (prevPush !== null && GitHubUtil.isMain(prevPush.ref) === true) { + Log.info(PREAMBLE + "repo: " + info.repoId + "; person: " + info.personId + + "; using previous: " + prevPush.ref + "; instead of: " + info.ref); info = prevPush; - // already saved so just use that instead + // already saved so just use that instead (even if the current one is also on main) } else { - Log.info("GitHubAutoTest::handlePushEvent(..) - " + - "repo: " + info.repoId + "; person: " + info.personId + - "; SHA: " + Util.shaHuman(info.commitSHA) + "; branch: " + info.ref + - "; no main/master push found"); + // Log.trace(PREAMBLE + "repo: " + info.repoId + "; person: " + info.personId + + // "; branch: " + info.ref + "; no previous main/master push found"); await this.savePushInfo(info); } const start = Date.now(); if (typeof delivId === "undefined" || delivId === null) { - Log.trace("GitHubAutoTest::handlePushEvent(..) - deliv not specified; requesting"); + Log.trace(PREAMBLE + "deliv not specified; requesting"); delivId = await this.getDefaultDelivId(); // current default deliverable - Log.info("GitHubAutoTest::handlePushEvent(..) - retrieved deliv: " + - delivId + "; type: " + typeof delivId); + Log.info(PREAMBLE + "retrieved deliv: " + delivId + "; type: " + typeof delivId); if (delivId === "null") { // delivId should be null if null, not "null"; force this flag if this is the case @@ -104,12 +103,12 @@ export class GitHubAutoTest extends AutoTest implements IGitHubTestManager { if (delivId !== null) { const containerConfig = await this.classPortal.getContainerDetails(delivId); if (containerConfig === null) { - Log.info("GitHubAutoTest::handlePushEvent(..) - not scheduled; no default deliverable"); + Log.info(PREAMBLE + "not scheduled; no default deliverable"); return false; } if (containerConfig.closeTimestamp < info.timestamp && containerConfig.lateAutoTest === false) { - Log.info("GitHubAutoTest::handlePushEvent(..) - not scheduled; deliv is closed to grading"); + Log.info(PREAMBLE + "not scheduled; deliv is closed to grading"); return false; } @@ -119,12 +118,12 @@ export class GitHubAutoTest extends AutoTest implements IGitHubTestManager { const sha = Util.shaHuman(info.commitSHA); if (info.botMentioned === true) { - Log.info(`GitHubAutoTest::handlePushEvent( ${sha} ) - bot mentioned; adding to exp queue`); + Log.info(PREAMBLE + "bot mentioned; adding to exp queue"); // requested jobs will always go on express this.addToExpressQueue(input); } else { if (shouldPromotePush === true) { - Log.info(`GitHubAutoTest::handlePushEvent( ${sha} ) - shouldPromote; Force adding to std queue`); + Log.info(PREAMBLE + "shouldPromote; Force adding to std queue"); // promoted jobs should always go on standard this.addToStandardQueue(input, true); } else { @@ -153,9 +152,9 @@ export class GitHubAutoTest extends AutoTest implements IGitHubTestManager { }; regressionInput.target.shouldPromote = true; // plugin is expecting it, make sure it is added - Log.info("GitHubAutoTest::handlePushEvent(..) - scheduling regressionId: " + regressionId); - Log.trace("GitHubAutoTest::handlePushEvent(..) - scheduling regressionId: " + regressionId + - "; input: " + JSON.stringify(regressionInput)); + Log.info(PREAMBLE + "scheduling regressionId: " + regressionId); + Log.trace( + PREAMBLE + "scheduling regressionId: " + regressionId + "; input: " + JSON.stringify(regressionInput)); this.addToLowQueue(regressionInput); } @@ -163,17 +162,15 @@ export class GitHubAutoTest extends AutoTest implements IGitHubTestManager { } this.tick(); - Log.info("GitHubAutoTest::handlePushEvent(..) - done; " + - "deliv: " + info.delivId + "; repo: " + info.repoId + "; SHA: " + - Util.shaHuman(info.commitSHA) + "; took: " + Util.took(start)); + Log.info(PREAMBLE + "done; " + "deliv: " + info.delivId + "; repo: " + info.repoId + "; took: " + Util.took(start)); return true; } else { // no active deliverable, ignore this push event (do not push an error either) - Log.warn("GitHubAutoTest::handlePushEvent(..) - commit: " + info.commitSHA + " - No active deliverable; push ignored."); + Log.warn(PREAMBLE + "commit: " + info.commitSHA + " - No active deliverable; push ignored."); return false; } } catch (err) { - Log.error("GitHubAutoTest::handlePushEvent(..) - ERROR: " + err.message); + Log.error("GitHubAutoTest::handlePushEvent( " + Util.shaHuman(info.commitSHA) + " ) - ERROR: " + err.message); throw err; } } diff --git a/packages/autotest/src/github/GitHubUtil.ts b/packages/autotest/src/github/GitHubUtil.ts index 07120274e..20ed3d7c6 100644 --- a/packages/autotest/src/github/GitHubUtil.ts +++ b/packages/autotest/src/github/GitHubUtil.ts @@ -470,4 +470,8 @@ export class GitHubUtil { } return commitURL; } + + public static isMain(ref: string): boolean { + return ref === "refs/heads/main" || ref === "refs/heads/master"; + } } From 6e10422d04d2c22508153b14e434c71f68cd52ba Mon Sep 17 00:00:00 2001 From: reid holmes Date: Tue, 29 Oct 2024 15:38:00 -0700 Subject: [PATCH 68/80] add some debugging for empty payloads for comment/push events --- packages/autotest/src/github/GitHubAutoTest.ts | 11 ++++++----- packages/autotest/src/server/AutoTestRouteHandler.ts | 8 ++++++++ 2 files changed, 14 insertions(+), 5 deletions(-) diff --git a/packages/autotest/src/github/GitHubAutoTest.ts b/packages/autotest/src/github/GitHubAutoTest.ts index 15201d101..03b6815ad 100644 --- a/packages/autotest/src/github/GitHubAutoTest.ts +++ b/packages/autotest/src/github/GitHubAutoTest.ts @@ -60,7 +60,7 @@ export class GitHubAutoTest extends AutoTest implements IGitHubTestManager { return false; } - const PREAMBLE = "GitHubAutoTest::handlePushEvent( " + Util.shaHuman(info.commitSHA) + ") - "; + const PREAMBLE = "GitHubAutoTest::handlePushEvent( " + Util.shaHuman(info.commitSHA) + " ) - "; const org = Config.getInstance().getProp(ConfigKey.org); Log.trace(PREAMBLE + "start; org: " + org + @@ -73,9 +73,10 @@ export class GitHubAutoTest extends AutoTest implements IGitHubTestManager { Log.info(PREAMBLE + "repo: " + info.repoId + "; person: " + info.personId + "; branch: " + info.ref); - // if there are already pushes for this SHA, and any of them are on main/master, use those instead + // If there are already pushes for this SHA, and any of them are on main/master, use those instead // the implication here is that a SHA on a branch will not overwrite a result for a SHA on main/master - // but a SHA on main/master can overwrite a SHA on a branch + // but a SHA on main/master can overwrite a SHA on a branch. + // This sometimes happens when a commit is on a named branch and main, although I don't know why/how this comes to be. const prevPush = await this.dataStore.getPushRecord(info.commitURL); if (prevPush !== null && GitHubUtil.isMain(prevPush.ref) === true) { Log.info(PREAMBLE + "repo: " + info.repoId + "; person: " + info.personId + @@ -90,9 +91,9 @@ export class GitHubAutoTest extends AutoTest implements IGitHubTestManager { const start = Date.now(); if (typeof delivId === "undefined" || delivId === null) { - Log.trace(PREAMBLE + "deliv not specified; requesting"); + // Log.trace(PREAMBLE + "deliv not specified; requesting"); delivId = await this.getDefaultDelivId(); // current default deliverable - Log.info(PREAMBLE + "retrieved deliv: " + delivId + "; type: " + typeof delivId); + Log.trace(PREAMBLE + "retrieved deliv: " + delivId + "; type: " + typeof delivId); if (delivId === "null") { // delivId should be null if null, not "null"; force this flag if this is the case diff --git a/packages/autotest/src/server/AutoTestRouteHandler.ts b/packages/autotest/src/server/AutoTestRouteHandler.ts index f7d3c41ca..e094d2b33 100644 --- a/packages/autotest/src/server/AutoTestRouteHandler.ts +++ b/packages/autotest/src/server/AutoTestRouteHandler.ts @@ -174,11 +174,19 @@ export default class AutoTestRouteHandler { switch (event) { case "commit_comment": const commentEvent = await GitHubUtil.processComment(body); + if (commentEvent === null) { + Log.warn( + "AutoTestRouteHandler::handleWebhook() - comment event is null; figure out why; payload: " + JSON.stringify(body)); + } Log.trace("AutoTestRouteHandler::handleWebhook() - comment request: " + JSON.stringify(commentEvent, null, 2)); await at.handleCommentEvent(commentEvent); return commentEvent; case "push": const pushEvent = await GitHubUtil.processPush(body, new ClassPortal()); + if (pushEvent === null) { + Log.warn( + "AutoTestRouteHandler::handleWebhook() - push event is null; figure out why; payload: " + JSON.stringify(body)); + } Log.trace("AutoTestRouteHandler::handleWebhook() - push request: " + JSON.stringify(pushEvent, null, 2)); await at.handlePushEvent(pushEvent); return pushEvent; From 6e88f3bcba46dae0213e90c0a4b83b76743f99ba Mon Sep 17 00:00:00 2001 From: reid holmes Date: Tue, 29 Oct 2024 15:49:12 -0700 Subject: [PATCH 69/80] more push instrumentation --- packages/autotest/src/github/GitHubAutoTest.ts | 13 +++++-------- .../autotest/src/server/AutoTestRouteHandler.ts | 3 ++- 2 files changed, 7 insertions(+), 9 deletions(-) diff --git a/packages/autotest/src/github/GitHubAutoTest.ts b/packages/autotest/src/github/GitHubAutoTest.ts index 03b6815ad..2b1ebb53d 100644 --- a/packages/autotest/src/github/GitHubAutoTest.ts +++ b/packages/autotest/src/github/GitHubAutoTest.ts @@ -63,11 +63,9 @@ export class GitHubAutoTest extends AutoTest implements IGitHubTestManager { const PREAMBLE = "GitHubAutoTest::handlePushEvent( " + Util.shaHuman(info.commitSHA) + " ) - "; const org = Config.getInstance().getProp(ConfigKey.org); - Log.trace(PREAMBLE + "start; org: " + org + - "; push org: " + info.orgId); + Log.trace(PREAMBLE + "start; org: " + org + "; repo: " + info.repoId); if (typeof org !== "undefined" && typeof info.orgId !== "undefined" && org !== info.orgId) { - Log.warn(PREAMBLE + "ignored, org: " + info.orgId + - " does not match current course: " + org); + Log.warn(PREAMBLE + "ignored, org: " + info.orgId + " does not match current course: " + org); return false; } @@ -116,7 +114,6 @@ export class GitHubAutoTest extends AutoTest implements IGitHubTestManager { const input: ContainerInput = {target: info, containerConfig}; const shouldPromotePush = await this.classPortal.shouldPromotePush(info); input.target.shouldPromote = shouldPromotePush; - const sha = Util.shaHuman(info.commitSHA); if (info.botMentioned === true) { Log.info(PREAMBLE + "bot mentioned; adding to exp queue"); @@ -154,8 +151,8 @@ export class GitHubAutoTest extends AutoTest implements IGitHubTestManager { regressionInput.target.shouldPromote = true; // plugin is expecting it, make sure it is added Log.info(PREAMBLE + "scheduling regressionId: " + regressionId); - Log.trace( - PREAMBLE + "scheduling regressionId: " + regressionId + "; input: " + JSON.stringify(regressionInput)); + Log.trace(PREAMBLE + "scheduling regressionId: " + regressionId + + "; input: " + JSON.stringify(regressionInput)); this.addToLowQueue(regressionInput); } @@ -167,7 +164,7 @@ export class GitHubAutoTest extends AutoTest implements IGitHubTestManager { return true; } else { // no active deliverable, ignore this push event (do not push an error either) - Log.warn(PREAMBLE + "commit: " + info.commitSHA + " - No active deliverable; push ignored."); + Log.warn(PREAMBLE + "no active deliverable; push ignored."); return false; } } catch (err) { diff --git a/packages/autotest/src/server/AutoTestRouteHandler.ts b/packages/autotest/src/server/AutoTestRouteHandler.ts index e094d2b33..cb54b8a25 100644 --- a/packages/autotest/src/server/AutoTestRouteHandler.ts +++ b/packages/autotest/src/server/AutoTestRouteHandler.ts @@ -183,7 +183,8 @@ export default class AutoTestRouteHandler { return commentEvent; case "push": const pushEvent = await GitHubUtil.processPush(body, new ClassPortal()); - if (pushEvent === null) { + if (pushEvent === null && (body as any)?.deleted !== true) { + // figure out reasons we end up here that are not deleted branches Log.warn( "AutoTestRouteHandler::handleWebhook() - push event is null; figure out why; payload: " + JSON.stringify(body)); } From b34f2bf7a9ee3012473aafbc6df1144d3ad377ac Mon Sep 17 00:00:00 2001 From: reid holmes Date: Wed, 30 Oct 2024 09:01:04 -0700 Subject: [PATCH 70/80] more push logging --- packages/autotest/src/github/GitHubAutoTest.ts | 5 ++--- packages/autotest/src/github/GitHubUtil.ts | 6 +++--- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/packages/autotest/src/github/GitHubAutoTest.ts b/packages/autotest/src/github/GitHubAutoTest.ts index 2b1ebb53d..5c96fb086 100644 --- a/packages/autotest/src/github/GitHubAutoTest.ts +++ b/packages/autotest/src/github/GitHubAutoTest.ts @@ -770,9 +770,8 @@ export class GitHubAutoTest extends AutoTest implements IGitHubTestManager { */ private async savePushInfo(info: CommitTarget) { try { - Log.trace("GitHubAutoTest::savePushInfo(..) - deliv: " + info.delivId + - "repo: " + info.repoId + - "; SHA: " + Util.shaHuman(info.commitSHA)); + Log.trace("GitHubAutoTest::savePushInfo( " + Util.shaHuman(info.commitSHA) + " ) - deliv: " + info.delivId + + "; repo: " + info.repoId + "; ref: " + info.ref); await this.dataStore.savePush(info); } catch (err) { Log.error("GitHubAutoTest::savePushInfo(..) - ERROR: " + err); diff --git a/packages/autotest/src/github/GitHubUtil.ts b/packages/autotest/src/github/GitHubUtil.ts index 20ed3d7c6..1f2b52b04 100644 --- a/packages/autotest/src/github/GitHubUtil.ts +++ b/packages/autotest/src/github/GitHubUtil.ts @@ -348,9 +348,9 @@ export class GitHubUtil { ref }; - Log.info("GitHubUtil::processPush(..) - done; repo: " + repo + "; person: " + - pusher?.personId + "; SHA: " + Util.shaHuman(commitSHA) + "; took: " + Util.took(start)); - Log.trace("GitHubUtil::processPush(..) - done; pushEvent:", pushEvent); + Log.info("GitHubUtil::processPush(..) - done; person: " + pusher?.personId + "; repo: " + repo + + "; SHA: " + Util.shaHuman(commitSHA) + "; ref: " + ref + "; took: " + Util.took(start)); + // Log.trace("GitHubUtil::processPush(..) - done; pushEvent:", pushEvent); return pushEvent; } catch (err) { Log.error("GitHubUtil::processPush(..) - ERROR parsing: " + err); From 1114465aa65e906039c4bbc3c8f612eb1a8c400a Mon Sep 17 00:00:00 2001 From: reid holmes Date: Fri, 1 Nov 2024 10:11:50 -0700 Subject: [PATCH 71/80] see #117; disable caching --- packages/portal/backend/html/index.html | 9 ++++++--- packages/portal/backend/src/server/BackendServer.ts | 7 +++++++ packages/portal/frontend/html/index.html | 5 +++++ packages/proxy/nginx.rconf.default | 6 ++++++ 4 files changed, 24 insertions(+), 3 deletions(-) diff --git a/packages/portal/backend/html/index.html b/packages/portal/backend/html/index.html index 9549cb875..949fd466a 100644 --- a/packages/portal/backend/html/index.html +++ b/packages/portal/backend/html/index.html @@ -2,9 +2,12 @@ - SDMM Backend ERROR + + + + Classy ERROR -

The requested resource does not exist

+

The requested resource does not exist.

- \ No newline at end of file + diff --git a/packages/portal/backend/src/server/BackendServer.ts b/packages/portal/backend/src/server/BackendServer.ts index 2ea9ddb69..8f5634ecd 100644 --- a/packages/portal/backend/src/server/BackendServer.ts +++ b/packages/portal/backend/src/server/BackendServer.ts @@ -95,6 +95,13 @@ export default class BackendServer { return next(); }); + // prevent caching, overrides cache headers in html files + that.rest.use(function(req, res, next) { + res.header("Last-Modified", new Date()); + res.header("Cache-Control", "no-cache, no-store, must-revalidate, max-age=0"); + return next(); + }); + // Register handlers common between all classy instances Log.info("BackendServer::start() - Registering common handlers"); diff --git a/packages/portal/frontend/html/index.html b/packages/portal/frontend/html/index.html index 5f72f1f46..3885c4c0e 100644 --- a/packages/portal/frontend/html/index.html +++ b/packages/portal/frontend/html/index.html @@ -2,6 +2,11 @@ + + + + + Classy diff --git a/packages/proxy/nginx.rconf.default b/packages/proxy/nginx.rconf.default index e55466268..f71e1bb63 100644 --- a/packages/proxy/nginx.rconf.default +++ b/packages/proxy/nginx.rconf.default @@ -40,6 +40,12 @@ http { # pass requests to the portal service (which is automatically defined in the hosts file by docker) location / { + # kill cache (https://stackoverflow.com/a/45285696) + add_header Last-Modified $date_gmt; + add_header Cache-Control 'no-store, no-cache'; + if_modified_since off; + expires off; + etag off; proxy_pass "https://portal:<%= ENV["BACKEND_PORT"] %>/"; } From 5db9f244838b50c1ccca9362497a9d018f8c2ed3 Mon Sep 17 00:00:00 2001 From: Reid Holmes Date: Sun, 3 Nov 2024 15:00:27 -0800 Subject: [PATCH 72/80] logging work --- packages/autotest/src/autotest/ClassPortal.ts | 4 ++-- packages/autotest/src/server/AutoTestRouteHandler.ts | 8 ++++++-- .../portal/backend/src/controllers/AdminController.ts | 2 +- .../portal/backend/src/server/common/AutoTestRoutes.ts | 4 ++-- 4 files changed, 11 insertions(+), 7 deletions(-) diff --git a/packages/autotest/src/autotest/ClassPortal.ts b/packages/autotest/src/autotest/ClassPortal.ts index a08bb0b64..85301d925 100644 --- a/packages/autotest/src/autotest/ClassPortal.ts +++ b/packages/autotest/src/autotest/ClassPortal.ts @@ -286,8 +286,8 @@ export class ClassPortal implements IClassPortal { body: JSON.stringify(grade) }; - Log.trace("ClassPortal::sendGrade(..) - sending to: " + url + "; deliv: " + grade.delivId + - "; repo: " + grade.repoId + "; url: " + grade.URL); + Log.trace("ClassPortal::sendGrade(..) - deliv: " + grade.delivId + + "; repo: " + grade.repoId + +"; grade: " + grade.score + "; url: " + grade.URL); Log.trace("ClassPortal::sendGrade(..) - payload: " + JSON.stringify(grade)); const res = await fetch(url, opts); diff --git a/packages/autotest/src/server/AutoTestRouteHandler.ts b/packages/autotest/src/server/AutoTestRouteHandler.ts index cb54b8a25..d041dc87f 100644 --- a/packages/autotest/src/server/AutoTestRouteHandler.ts +++ b/packages/autotest/src/server/AutoTestRouteHandler.ts @@ -183,8 +183,12 @@ export default class AutoTestRouteHandler { return commentEvent; case "push": const pushEvent = await GitHubUtil.processPush(body, new ClassPortal()); - if (pushEvent === null && (body as any)?.deleted !== true) { - // figure out reasons we end up here that are not deleted branches + + if (pushEvent === null && (body as any)?.deleted === true) { + // branch was deleted + Log.info("AutoTestRouteHandler::handleWebhook() - branch was deleted; no action required"); + } else if (pushEvent === null) { + // figure out other reasons we end up here Log.warn( "AutoTestRouteHandler::handleWebhook() - push event is null; figure out why; payload: " + JSON.stringify(body)); } diff --git a/packages/portal/backend/src/controllers/AdminController.ts b/packages/portal/backend/src/controllers/AdminController.ts index 2a44a1b5d..5a2719454 100644 --- a/packages/portal/backend/src/controllers/AdminController.ts +++ b/packages/portal/backend/src/controllers/AdminController.ts @@ -102,7 +102,7 @@ export class AdminController { const existingGrade = await this.gc.getGrade(personId, grade.delivId); const existingGradeScore = (existingGrade?.score) ? existingGrade.score : "N/A"; Log.trace("AdminController::processNewAutoTestGrade(..) - handling grade for " + personId + - "; existingGrade: " + existingGradeScore + "; newGrade: " + newGrade.score); + "; repo: " + grade.repoId + "; existingGrade: " + existingGradeScore + "; newGrade: " + newGrade.score); const shouldSave = await cc.handleNewAutoTestGrade(deliv, newGrade, existingGrade); // Log.trace("AdminController::processNewAutoTestGrade(..) - handled grade for " + personId + // "; shouldSave: " + shouldSave); // NOTE: for hangup debugging diff --git a/packages/portal/backend/src/server/common/AutoTestRoutes.ts b/packages/portal/backend/src/server/common/AutoTestRoutes.ts index 9c83e7096..dc0157f3c 100644 --- a/packages/portal/backend/src/server/common/AutoTestRoutes.ts +++ b/packages/portal/backend/src/server/common/AutoTestRoutes.ts @@ -189,8 +189,8 @@ export class AutoTestRoutes implements IREST { "; repo: " + grade.repoId + "; grade: " + grade.score); // Log.trace("AutoTestRoutes::atGrade(..) - repoId: " + grade.repoId + // "; delivId: " + grade.delivId + "; body: " + JSON.stringify(grade)); - const cc = new AdminController(new GitHubController(GitHubActions.getInstance())); - return await cc.processNewAutoTestGrade(grade); + const ac = new AdminController(new GitHubController(GitHubActions.getInstance())); + return await ac.processNewAutoTestGrade(grade); } } From e1a8730c9a6f2073f63c64107a8d59b3641b9f0f Mon Sep 17 00:00:00 2001 From: Reid Holmes Date: Mon, 4 Nov 2024 09:22:55 -0800 Subject: [PATCH 73/80] add warning when container not specified for deliverable (console-facing, not student-facing) --- packages/autotest/src/autotest/AutoTest.ts | 7 +++++++ packages/autotest/src/autotest/ClassPortal.ts | 2 +- 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/packages/autotest/src/autotest/AutoTest.ts b/packages/autotest/src/autotest/AutoTest.ts index 3fd7574ea..e545d90c9 100644 --- a/packages/autotest/src/autotest/AutoTest.ts +++ b/packages/autotest/src/autotest/AutoTest.ts @@ -608,6 +608,8 @@ export abstract class AutoTest implements IAutoTest { Log.info("AutoTest::handleTick(..) - stale job; old container: " + input.containerConfig.dockerImage + "; new container: " + containerConfig.dockerImage); input.containerConfig = containerConfig; + } else if (containerConfig === null) { + Log.warn("AutoTest::handleTick(..) - no container found for delivId: " + input.target.delivId); } } catch (err) { Log.warn("AutoTest::handleTick(..) - problem updating container config: " + err.message); @@ -621,6 +623,11 @@ export abstract class AutoTest implements IAutoTest { try { await job.prepare(); + + Log.info("AutoTest::handleTick(..) - prepared; deliv: " + input.target.delivId + + "; repo: " + input.target.repoId + "; SHA: " + Util.shaHuman( + input.target.commitSHA)); + record = await job.run(this.docker); Log.info("AutoTest::handleTick(..) - executed; deliv: " + input.target.delivId + diff --git a/packages/autotest/src/autotest/ClassPortal.ts b/packages/autotest/src/autotest/ClassPortal.ts index 85301d925..ef50e1d81 100644 --- a/packages/autotest/src/autotest/ClassPortal.ts +++ b/packages/autotest/src/autotest/ClassPortal.ts @@ -287,7 +287,7 @@ export class ClassPortal implements IClassPortal { }; Log.trace("ClassPortal::sendGrade(..) - deliv: " + grade.delivId + - "; repo: " + grade.repoId + +"; grade: " + grade.score + "; url: " + grade.URL); + "; repo: " + grade.repoId + "; grade: " + grade.score + "; url: " + grade.URL); Log.trace("ClassPortal::sendGrade(..) - payload: " + JSON.stringify(grade)); const res = await fetch(url, opts); From 1ac2aa49fd091d220cd1abafe7e3c6435f725ec4 Mon Sep 17 00:00:00 2001 From: Reid Holmes Date: Tue, 5 Nov 2024 20:33:41 -0800 Subject: [PATCH 74/80] logging --- packages/autotest/src/autotest/GradingJob.ts | 4 +--- packages/autotest/src/github/GitHubUtil.ts | 8 ++++---- 2 files changed, 5 insertions(+), 7 deletions(-) diff --git a/packages/autotest/src/autotest/GradingJob.ts b/packages/autotest/src/autotest/GradingJob.ts index ed962654e..f5ded5ab8 100644 --- a/packages/autotest/src/autotest/GradingJob.ts +++ b/packages/autotest/src/autotest/GradingJob.ts @@ -116,14 +116,12 @@ export class GradingJob { Log.trace("GradingJob::run() - updated: " + this.id); const maxExecTime = this.input.containerConfig.maxExecTime; - Log.trace("GradingJob::run() - after container: " + this.id); - const stdio = fs.createWriteStream(this.path + "/staff/stdio.txt"); const stream = await container.attach({stream: true, stdout: true, stderr: true}); container.modem.demuxStream(stream, stdio, stdio); const exitCode = await GradingJob.runContainer(container, maxExecTime); - Log.trace("GradingJob::run() - after run: " + this.id + "; exit code: " + exitCode); + Log.trace("GradingJob::run() - container complete: " + this.id + "; exit code: " + exitCode); // NOTE: at this point, out just contains default values const out = this.record.output; diff --git a/packages/autotest/src/github/GitHubUtil.ts b/packages/autotest/src/github/GitHubUtil.ts index 1f2b52b04..02119c66f 100644 --- a/packages/autotest/src/github/GitHubUtil.ts +++ b/packages/autotest/src/github/GitHubUtil.ts @@ -170,13 +170,13 @@ export class GitHubUtil { Log.warn("GitHubUtil::processComment(..) - failed to parse org: " + err); } - Log.trace("GitHubUtil::processComment(..) - 1"); + // Log.trace("GitHubUtil::processComment(..) - 1"); const cp = new ClassPortal(); const config = await cp.getConfiguration(); const delivId = GitHubUtil.parseDeliverableFromComment(message, config.deliverableIds); - Log.trace("GitHubUtil::processComment(..) - 2"); + // Log.trace("GitHubUtil::processComment(..) - 2"); const flags: string[] = GitHubUtil.parseCommandsFromComment(message); @@ -185,7 +185,7 @@ export class GitHubUtil { const repoId = payload.repository.name; - Log.trace("GitHubUtil::processComment(..) - 3"); + // Log.trace("GitHubUtil::processComment(..) - 3"); // const timestamp = new Date(payload.comment.updated_at).getTime(); // updated so they cannot add requests to a past comment const timestamp = Date.now(); // set timestamp to the time the commit was made @@ -199,7 +199,7 @@ export class GitHubUtil { if (authLevel.isStaff === true || authLevel.isAdmin === true) { adminRequest = true; } - Log.trace("GitHubUtil::processComment(..) - 4"); + // Log.trace("GitHubUtil::processComment(..) - 4"); const kind = "standard"; const shouldPromote = false; // will be set later if needed From a84c67c231fe148dc7c198b6a8d646a8dd1c7496 Mon Sep 17 00:00:00 2001 From: reid holmes Date: Tue, 17 Dec 2024 13:14:19 -0800 Subject: [PATCH 75/80] see #118; enable grade CSVs to be uploaded with StudentNumber as the primary key. --- .../backend/src/server/common/CSVParser.ts | 19 +++++++++++++++++-- .../default/portal/frontend/html/admin.html | 10 ++++------ 2 files changed, 21 insertions(+), 8 deletions(-) diff --git a/packages/portal/backend/src/server/common/CSVParser.ts b/packages/portal/backend/src/server/common/CSVParser.ts index 77c42afc9..72d6488c6 100644 --- a/packages/portal/backend/src/server/common/CSVParser.ts +++ b/packages/portal/backend/src/server/common/CSVParser.ts @@ -92,15 +92,30 @@ export class CSVParser { const gc = new GradesController(); const gradePromises: Array> = []; let errorMessage = ""; + + const allPeople = await pc.getAllPeople(); + for (const row of data) { // this check could probably be outside the loop, but since it throws // it only happens once anyways const firstKey = Object.keys(row)[0]; - if (firstKey === "CSID" || firstKey === "CWL" || firstKey === "GITHUB") { + if (firstKey === "CSID" || firstKey === "CWL" || firstKey === "GITHUB" || firstKey === "StudentNumber") { // good record } else { - throw new Error("CSID/CWL/GITHUB should be the first column in the CSV"); + throw new Error("CSID/CWL/GITHUB/StudentNumber must be the first column in the CSV"); + } + + if (typeof row.StudentNumber !== "undefined") { + // student number is given, make sure person has the right id + + // find the person with the student number + const person = allPeople.find((p) => p.studentNumber === row.StudentNumber); + if (person !== null) { + row.CSID = person.id; + } else { + Log.warn("CSVParser::processGrades(..) - Unknown StudentNumber: " + row.StudentNumber); + } } if (typeof row.CSID === "undefined" && typeof row.GITHUB !== "undefined") { diff --git a/plugins/default/portal/frontend/html/admin.html b/plugins/default/portal/frontend/html/admin.html index 9fdffe048..b0393ce6d 100644 --- a/plugins/default/portal/frontend/html/admin.html +++ b/plugins/default/portal/frontend/html/admin.html @@ -305,13 +305,11 @@
The CSV must have a header row that contains the following columns: - 
CSID,GRADE,COMMENT
+ 
[CSID|CWL|GITHUB|StudentNumber],GRADE,COMMENT
- If the CSID column is not present but a CWL or GITHUB column is present, Classy tries to map - from the CWL/GITHUB - column back to a CSID. This will overwrite any grade that exist for that the students listed in - the CSV. This cannot - be undone. + If the CSID column is not present but a CWL, GITHUB, or StudentNumber column is present, + Classy tries to map from the CWL/GITHUB/StudentNumber column back to a CSID. This will + overwrite any grade that exist for that the students listed in the CSV. This cannot be undone.
From e9916ea5ca9c2c8cd8a809d533727b7ac8fe64ec Mon Sep 17 00:00:00 2001 From: reid holmes Date: Tue, 17 Dec 2024 13:37:26 -0800 Subject: [PATCH 76/80] see #118 make caps consistent, also provide useful error message if GRADE col is missing --- .../portal/backend/src/server/common/CSVParser.ts | 14 +++++++++----- packages/portal/backend/test/CSVParserSpec.ts | 2 +- plugins/default/portal/frontend/html/admin.html | 7 ++++--- 3 files changed, 14 insertions(+), 9 deletions(-) diff --git a/packages/portal/backend/src/server/common/CSVParser.ts b/packages/portal/backend/src/server/common/CSVParser.ts index 72d6488c6..b4a882680 100644 --- a/packages/portal/backend/src/server/common/CSVParser.ts +++ b/packages/portal/backend/src/server/common/CSVParser.ts @@ -100,21 +100,25 @@ export class CSVParser { // this check could probably be outside the loop, but since it throws // it only happens once anyways const firstKey = Object.keys(row)[0]; - if (firstKey === "CSID" || firstKey === "CWL" || firstKey === "GITHUB" || firstKey === "StudentNumber") { + if (firstKey === "CSID" || firstKey === "CWL" || firstKey === "GITHUB" || firstKey === "STUDENTNUMBER") { // good record } else { - throw new Error("CSID/CWL/GITHUB/StudentNumber must be the first column in the CSV"); + throw new Error("CSID/CWL/GITHUB/STUDENTNUMBER must be the first column in the CSV"); } - if (typeof row.StudentNumber !== "undefined") { + if (Object.keys(row)[0].includes("GRADE") === false) { + throw new Error("GRADE column must be present in the CSV"); + } + + if (typeof row.STUDENTNUMBER !== "undefined") { // student number is given, make sure person has the right id // find the person with the student number - const person = allPeople.find((p) => p.studentNumber === row.StudentNumber); + const person = allPeople.find((p) => p.studentNumber === row.STUDENTNUMBER); if (person !== null) { row.CSID = person.id; } else { - Log.warn("CSVParser::processGrades(..) - Unknown StudentNumber: " + row.StudentNumber); + Log.warn("CSVParser::processGrades(..) - Unknown STUDENTNUMBER: " + row.STUDENTNUMBER); } } diff --git a/packages/portal/backend/test/CSVParserSpec.ts b/packages/portal/backend/test/CSVParserSpec.ts index b67c8cb01..df2c713c1 100644 --- a/packages/portal/backend/test/CSVParserSpec.ts +++ b/packages/portal/backend/test/CSVParserSpec.ts @@ -10,7 +10,7 @@ import {CSVParser} from "@backend/server/common/CSVParser"; import "@common/GlobalSpec"; import {Grade} from "@backend/Types"; -describe("CSVParser", function () { +describe.only("CSVParser", function () { before(async () => { await TestHarness.suiteBefore("CSVParser"); diff --git a/plugins/default/portal/frontend/html/admin.html b/plugins/default/portal/frontend/html/admin.html index b0393ce6d..cd63e28d9 100644 --- a/plugins/default/portal/frontend/html/admin.html +++ b/plugins/default/portal/frontend/html/admin.html @@ -305,11 +305,12 @@
The CSV must have a header row that contains the following columns: - 
[CSID|CWL|GITHUB|StudentNumber],GRADE,COMMENT
+ 
[CSID|CWL|GITHUB|STUDENTNUMBER],GRADE
- If the CSID column is not present but a CWL, GITHUB, or StudentNumber column is present, - Classy tries to map from the CWL/GITHUB/StudentNumber column back to a CSID. This will + If the CSID column is not present but a CWL, GITHUB, or STUDENTNUMBER column is present, + Classy tries to map from the CWL/GITHUB/STUDENTNUMBER column back to a CSID. This will overwrite any grade that exist for that the students listed in the CSV. This cannot be undone. + A COMMENT column is optionally used to provide feedback to the student, if present.
From a2a98c6f8b636cf0ddbda60d8ce08d35971731b9 Mon Sep 17 00:00:00 2001 From: reid holmes Date: Tue, 17 Dec 2024 13:38:02 -0800 Subject: [PATCH 77/80] remove errant .only --- packages/portal/backend/test/CSVParserSpec.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/portal/backend/test/CSVParserSpec.ts b/packages/portal/backend/test/CSVParserSpec.ts index df2c713c1..b67c8cb01 100644 --- a/packages/portal/backend/test/CSVParserSpec.ts +++ b/packages/portal/backend/test/CSVParserSpec.ts @@ -10,7 +10,7 @@ import {CSVParser} from "@backend/server/common/CSVParser"; import "@common/GlobalSpec"; import {Grade} from "@backend/Types"; -describe.only("CSVParser", function () { +describe("CSVParser", function () { before(async () => { await TestHarness.suiteBefore("CSVParser"); From 9936ac098ba770490fc40ce67652ecef66291304 Mon Sep 17 00:00:00 2001 From: reid holmes Date: Tue, 17 Dec 2024 13:45:08 -0800 Subject: [PATCH 78/80] See #118 more robustly track errors for student numbers that are unknown --- .../portal/backend/src/server/common/CSVParser.ts | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/packages/portal/backend/src/server/common/CSVParser.ts b/packages/portal/backend/src/server/common/CSVParser.ts index b4a882680..14872ea17 100644 --- a/packages/portal/backend/src/server/common/CSVParser.ts +++ b/packages/portal/backend/src/server/common/CSVParser.ts @@ -97,8 +97,8 @@ export class CSVParser { for (const row of data) { - // this check could probably be outside the loop, but since it throws - // it only happens once anyways + // these column checks should be outside the loop, but since they throw + // they only happens once const firstKey = Object.keys(row)[0]; if (firstKey === "CSID" || firstKey === "CWL" || firstKey === "GITHUB" || firstKey === "STUDENTNUMBER") { // good record @@ -106,19 +106,21 @@ export class CSVParser { throw new Error("CSID/CWL/GITHUB/STUDENTNUMBER must be the first column in the CSV"); } - if (Object.keys(row)[0].includes("GRADE") === false) { + if (Object.keys(row).includes("GRADE") === false) { throw new Error("GRADE column must be present in the CSV"); } if (typeof row.STUDENTNUMBER !== "undefined") { - // student number is given, make sure person has the right id - - // find the person with the student number const person = allPeople.find((p) => p.studentNumber === row.STUDENTNUMBER); if (person !== null) { row.CSID = person.id; + Log.trace("CSVParser::processGrades(..) - STUDENTNUMBER -> CSID: " + row.STUDENTNUMBER + " -> " + row.CSID); } else { Log.warn("CSVParser::processGrades(..) - Unknown STUDENTNUMBER: " + row.STUDENTNUMBER); + if (errorMessage === "") { + errorMessage = "Unknown Student Numbers: "; + } + errorMessage += row.STUDENTNUMBER + ", "; } } From 8ad104074e765bedf5ba6e2d7efd509850c87949 Mon Sep 17 00:00:00 2001 From: reid holmes Date: Tue, 17 Dec 2024 13:55:03 -0800 Subject: [PATCH 79/80] see #118 work with truthyness of find better --- packages/portal/backend/src/server/common/CSVParser.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/portal/backend/src/server/common/CSVParser.ts b/packages/portal/backend/src/server/common/CSVParser.ts index 14872ea17..48a5ffbbf 100644 --- a/packages/portal/backend/src/server/common/CSVParser.ts +++ b/packages/portal/backend/src/server/common/CSVParser.ts @@ -112,7 +112,7 @@ export class CSVParser { if (typeof row.STUDENTNUMBER !== "undefined") { const person = allPeople.find((p) => p.studentNumber === row.STUDENTNUMBER); - if (person !== null) { + if (person && typeof person.id === "string") { row.CSID = person.id; Log.trace("CSVParser::processGrades(..) - STUDENTNUMBER -> CSID: " + row.STUDENTNUMBER + " -> " + row.CSID); } else { From 1d59247a45815fdecb02174e96d31a43f6be649c Mon Sep 17 00:00:00 2001 From: reid holmes Date: Wed, 18 Dec 2024 13:42:50 -0800 Subject: [PATCH 80/80] dissallow #dev use by non-admins --- packages/autotest/src/github/GitHubAutoTest.ts | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/packages/autotest/src/github/GitHubAutoTest.ts b/packages/autotest/src/github/GitHubAutoTest.ts index 5c96fb086..12471f779 100644 --- a/packages/autotest/src/github/GitHubAutoTest.ts +++ b/packages/autotest/src/github/GitHubAutoTest.ts @@ -260,6 +260,16 @@ export class GitHubAutoTest extends AutoTest implements IGitHubTestManager { return false; } + // reject #dev requests by requesters who are not admins or staff + if (info.flags.indexOf("#dev") >= 0) { + Log.info("GitHubAutoTest::checkCommentPreconditions( " + info.personId + + " ) - ignored, student use of #dev"); + const msg = "Only admins can use the #dev flag."; + delete info.flags; + await this.postToGitHub(info, {url: info.postbackURL, message: msg}); + return false; + } + // reject #silent requests by requesters that are not admins or staff if (info.flags.indexOf("#silent") >= 0) { Log.warn("GitHubAutoTest::checkCommentPreconditions( " + info.personId +