diff --git a/packages/autotest/src/autotest/AutoTest.ts b/packages/autotest/src/autotest/AutoTest.ts index 03464bd15..e545d90c9 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; @@ -607,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); @@ -620,13 +623,18 @@ 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 + "; 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); @@ -643,6 +651,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 { diff --git a/packages/autotest/src/autotest/ClassPortal.ts b/packages/autotest/src/autotest/ClassPortal.ts index d9c76c942..ef50e1d81 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>; } /** @@ -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); @@ -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 = { @@ -489,12 +490,14 @@ 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 + message: json.message, + fullMessage: json.fullMessage }; } else if (typeof json?.notImplemented === "boolean" && json.notImplemented === true) { resp = null; diff --git a/packages/autotest/src/autotest/DataStore.ts b/packages/autotest/src/autotest/DataStore.ts index f5936b5bc..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 { @@ -154,37 +155,39 @@ 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"); + // 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[]) { - if (typeof r.ref !== "undefined" && (r.ref === "refs/heads/main" || r.ref === "refs/heads/master")) { - return r as CommitTarget; + for (const r of records) { + + if (typeof r.ref !== "undefined" && GitHubUtil.isMain(r.ref) === true) { + 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); } diff --git a/packages/autotest/src/autotest/GradingJob.ts b/packages/autotest/src/autotest/GradingJob.ts index 07f9597d5..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; @@ -149,7 +147,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."; @@ -161,7 +159,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/GitHubAutoTest.ts b/packages/autotest/src/github/GitHubAutoTest.ts index 8fd01c252..12471f779 100644 --- a/packages/autotest/src/github/GitHubAutoTest.ts +++ b/packages/autotest/src/github/GitHubAutoTest.ts @@ -53,31 +53,45 @@ 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 + "; repo: " + info.repoId); if (typeof org !== "undefined" && typeof info.orgId !== "undefined" && org !== info.orgId) { - Log.warn("GitHubAutoTest::handlePushEvent(..) - 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; } - 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); + 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. + // 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 + + "; using previous: " + prevPush.ref + "; instead of: " + info.ref); + info = prevPush; + // already saved so just use that instead (even if the current one is also on main) + } else { + // 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.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 @@ -88,27 +102,26 @@ 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; } 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(`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 { @@ -137,8 +150,8 @@ 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 + + Log.info(PREAMBLE + "scheduling regressionId: " + regressionId); + Log.trace(PREAMBLE + "scheduling regressionId: " + regressionId + "; input: " + JSON.stringify(regressionInput)); this.addToLowQueue(regressionInput); @@ -147,17 +160,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 + "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; } } @@ -249,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 + @@ -442,7 +463,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: " + @@ -511,7 +533,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); @@ -552,7 +574,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; } @@ -560,26 +582,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 { @@ -691,9 +714,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 +741,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; } } } @@ -749,9 +780,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 c4c404436..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 @@ -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 + @@ -309,7 +315,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; } @@ -342,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); @@ -464,4 +470,8 @@ export class GitHubUtil { } return commitURL; } + + public static isMain(ref: string): boolean { + return ref === "refs/heads/main" || ref === "refs/heads/master"; + } } diff --git a/packages/autotest/src/server/AutoTestRouteHandler.ts b/packages/autotest/src/server/AutoTestRouteHandler.ts index 7e4f4a676..d041dc87f 100644 --- a/packages/autotest/src/server/AutoTestRouteHandler.ts +++ b/packages/autotest/src/server/AutoTestRouteHandler.ts @@ -174,18 +174,30 @@ 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 && (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)); + } Log.trace("AutoTestRouteHandler::handleWebhook() - push request: " + JSON.stringify(pushEvent, null, 2)); await at.handlePushEvent(pushEvent); return pushEvent; 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/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-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/Factory.ts b/packages/portal/backend/src/Factory.ts index 928f7cf3b..662bb31c2 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"; @@ -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); @@ -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() - loading CustomCourseController; full path: " + controllerPath); + plug = await require(controllerPath); } catch (err) { const msg = "Factory::getCourseController() - " + plugin + "/src/custom/CustomCourseController.ts must be defined"; Log.error(msg); @@ -100,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/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/AdminController.ts b/packages/portal/backend/src/controllers/AdminController.ts index 20ddb441f..5a2719454 100644 --- a/packages/portal/backend/src/controllers/AdminController.ts +++ b/packages/portal/backend/src/controllers/AdminController.ts @@ -102,10 +102,14 @@ 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 + // 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/CourseController.ts b/packages/portal/backend/src/controllers/CourseController.ts index bb6412a27..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 @@ -77,8 +77,19 @@ export interface ICourseController { requestFeedbackDelay(info: { delivId: string; personId: string; timestamp: number }): Promise<{ accepted: boolean, - message: string + message: string, + fullMessage?: 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; } /** @@ -219,12 +230,23 @@ 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; } + /** + * 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/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/controllers/GitHubActions.ts b/packages/portal/backend/src/controllers/GitHubActions.ts index fef0374d6..11a2db703 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. * @@ -254,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. * @@ -514,6 +532,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 + "/repos/" + this.org + "/" + 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. * @@ -1325,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; } @@ -1362,19 +1447,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(..) - starting; branch uri: " + listUri); const listOptions: RequestInit = { method: "GET", headers: { @@ -1386,25 +1471,51 @@ 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; + } + + /** + * 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(); + + 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 @@ -1417,34 +1528,75 @@ 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); + deleteSucceeded = await this.deleteBranch(repoId, branch); + } + + // 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. + + 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"); + } - 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" - } - }; + Log.info("GitHubAction::deleteBranches(..) - done; success: " + deleteSucceeded + "; took: " + Util.took(start)); + return deleteSucceeded; + } - const deleteResp = await fetch(delUri, delOptions); - Log.trace("GitHubAction::deleteBranches(..) - delete response code: " + deleteResp.status); + /** + * 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(); - 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); - } + 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::deleteBranches(..) - done; success: " + deleteSucceeded + "; took: " + Util.took(start)); - return deleteSucceeded; + 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 + "; took: " + Util.took(start)); + return true; + } } public async renameBranch(repoId: string, oldName: string, newName: string): Promise { diff --git a/packages/portal/backend/src/controllers/GitHubController.ts b/packages/portal/backend/src/controllers/GitHubController.ts index e226c082e..66209ff77 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; } } @@ -429,7 +432,15 @@ 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.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 @@ -454,16 +465,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."); @@ -500,21 +515,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); 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/backend/src/server/BackendServer.ts b/packages/portal/backend/src/server/BackendServer.ts index b6fd61caf..8f5634ecd 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. @@ -93,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"); @@ -111,11 +120,23 @@ 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) { + 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"; diff --git a/packages/portal/backend/src/server/common/AutoTestRoutes.ts b/packages/portal/backend/src/server/common/AutoTestRoutes.ts index 457ca3695..dc0157f3c 100644 --- a/packages/portal/backend/src/server/common/AutoTestRoutes.ts +++ b/packages/portal/backend/src/server/common/AutoTestRoutes.ts @@ -185,12 +185,12 @@ 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)); - 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); } } diff --git a/packages/portal/backend/src/server/common/CSVParser.ts b/packages/portal/backend/src/server/common/CSVParser.ts index 8fc8e94a2..48a5ffbbf 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 { @@ -18,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 @@ -55,13 +56,19 @@ 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: + * 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. * - * If CSID 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 @@ -84,14 +91,51 @@ export class CSVParser { const gc = new GradesController(); const gradePromises: Array> = []; + let errorMessage = ""; + + const allPeople = await pc.getAllPeople(); + for (const row of data) { + // 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 + } else { + throw new Error("CSID/CWL/GITHUB/STUDENTNUMBER must be the first column in the CSV"); + } + + if (Object.keys(row).includes("GRADE") === false) { + throw new Error("GRADE column must be present in the CSV"); + } + + if (typeof row.STUDENTNUMBER !== "undefined") { + const person = allPeople.find((p) => p.studentNumber === row.STUDENTNUMBER); + if (person && typeof person.id === "string") { + 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 + ", "; + } + } + 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); + if (errorMessage === "") { + errorMessage = "Unknown ids: "; + } + errorMessage += row.GITHUB + ", "; } } @@ -99,8 +143,14 @@ 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); + if (errorMessage === "") { + errorMessage = "Unknown ids: "; + } + errorMessage += row.CWL + ", "; } } @@ -114,31 +164,53 @@ export class CSVParser { typeof row.COMMENT === "string" && row.COMMENT.length > 1) { comment = row.COMMENT; + comment = comment.trim(); + } + + let gradeScore = row.GRADE; + + if (typeof gradeScore === "string") { + gradeScore = gradeScore.trim(); + } + + const custom: any = {}; + if (Util.isNumeric(gradeScore) === true) { + gradeScore = parseFloat(gradeScore); + Log.trace("CSVParser::processGrades(..) - grade is a number: " + gradeScore); + } else { + 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: Number(row.GRADE), + score: gradeScore, 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); + const person = await pc.getPerson(personId); if (person !== null) { gradePromises.push(gc.saveGrade(g)); } else { Log.warn("CSVParser::processGrades(..) - record ignored for: " + personId + "; unknown personId"); + if (errorMessage === "") { + errorMessage = "Unknown ids: "; + } + errorMessage += personId + ", "; } } 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(..) - could not parse grade for record: " + JSON.stringify(row)); + // errorMessage += "Bad record: " + JSON.stringify(row) + ", "; } } @@ -149,6 +221,16 @@ export class CSVParser { await dbc.writeAudit(AuditLabel.GRADE_ADMIN, requesterId, {}, {}, {numGrades: grades.length}); Log.info("CSVParser::processGrades(..) - done; # grades: " + grades.length); + if (errorMessage.endsWith(", ")) { + errorMessage = errorMessage.slice(0, -2); + } + + 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 bd6eabc70..b67c8cb01 100644 --- a/packages/portal/backend/test/CSVParserSpec.ts +++ b/packages/portal/backend/test/CSVParserSpec.ts @@ -8,6 +8,7 @@ import {GradesController} from "@backend/controllers/GradesController"; import {CSVParser} from "@backend/server/common/CSVParser"; import "@common/GlobalSpec"; +import {Grade} from "@backend/Types"; describe("CSVParser", function () { @@ -50,6 +51,58 @@ 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(100); + expect(grade.custom.displayScore).to.equal("EXTENDING"); + grade = await gc.getGrade(TestHarness.USER2.id, TestHarness.DELIVID1); + 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(0); + 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/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..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."); } @@ -82,6 +86,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") { diff --git a/packages/portal/backend/test/data/gradesValidBucket.csv b/packages/portal/backend/test/data/gradesValidBucket.csv new file mode 100644 index 000000000..197a0b964 --- /dev/null +++ b/packages/portal/backend/test/data/gradesValidBucket.csv @@ -0,0 +1,4 @@ +CSID,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/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 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, 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/portal/frontend/html/stdio.html b/packages/portal/frontend/html/stdio.html index de5da16b1..c63c2a621 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); @@ -73,15 +75,102 @@ 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.indexOf(match) >= 0) { + // TODO: handle special case for Timeout of (see comment above) + toReturn += line + '\n'; + } + } + } + + // Everything before (and including) + // ProjectGrader::grade(..) - start + // SimpleGrader::doGrading() - start + let preamble = ""; + const PREFIX = "SimpleGrader::doGrading() - start"; + let prefixIndex = stdio.indexOf(PREFIX) + if (prefixIndex >= 0) { + preamble = stdio.substring(0, prefixIndex + PREFIX.length); + preamble += "\n***GRADING START***\n\n"; + } + toReturn = preamble + toReturn; + + // Everything after (and including) + // ProjectGrader::grade(..) - done + // SimpleGrader::doGrading() - done + let postfix = ""; + const POSTFIX = "SimpleGrader::doGrading() - done"; + 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) { + console.error("stdio::simplifyStdio(..) - ERROR: " + err); + toReturn = stdio; + } + return toReturn; + } + + 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::loadStdioBrief(..) - url: " + url); + getHTTP(url, HEADERS, function (stdio) { + if (HOST.indexOf("cs310") >= 0) { + 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 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(..) - 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); }); @@ -93,6 +182,12 @@ // add other keywords we want to highlight keywords.push("AssertionError"); keywords.push("TypeError"); + keywords.push("ENOENT"); + keywords.push("compareJSONArray"); + 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; @@ -137,6 +232,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 +272,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); diff --git a/packages/portal/frontend/html/style.css b/packages/portal/frontend/html/style.css index 968065f0b..ab0365c0f 100644 --- a/packages/portal/frontend/html/style.css +++ b/packages/portal/frontend/html/style.css @@ -105,19 +105,23 @@ Dashboard Page margin-left: auto; margin-right: auto; border-collapse: collapse; - 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 { @@ -127,7 +131,8 @@ Dashboard Page } .selectable { - user-select: text !important; + user-select: auto; + -webkit-user-select: auto; } /** diff --git a/packages/portal/frontend/src/app/util/SortableTable.ts b/packages/portal/frontend/src/app/util/SortableTable.ts index 515ebccd5..a400668a8 100644 --- a/packages/portal/frontend/src/app/util/SortableTable.ts +++ b/packages/portal/frontend/src/app/util/SortableTable.ts @@ -233,6 +233,28 @@ export class SortableTable { const aVal = a[sortIndex].value; const bVal = b[sortIndex].value; + if (typeof aVal === "string" && typeof bVal === "string") { + const BUCKET_ORDER = [ + "", + " ", + "na", + "n/a", + "beginning", + "acquiring", + "developing", + "proficient", + "extending", + "exceeding" + ]; + + 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; + } + } + return Util.compare(aVal, bVal) * mult; }); } @@ -269,35 +291,61 @@ export class SortableTable { // downloadLink.click(); } - private exportTableToCSV() { - const csv = []; + // 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"); + let colsWithMetadata: number[] = []; - for (let i = 0; i < rows.length; i++) { - const row = []; + // tslint:disable-next-line + for (let i = 1; i < rows.length; i++) { // skip the header 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 { - row.push((cols[j] as HTMLTableCellElement).innerText); + 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); + } } } - csv.push(row.join(",")); } - return csv.join("\n"); + // sort metadata columns + colsWithMetadata = colsWithMetadata.sort((a, b) => a - b); + + 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 exportTableLinksToCSV() { + 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++) { @@ -310,15 +358,23 @@ export class SortableTable { let text = (cols[j] as HTMLTableCellElement).innerText; text = text.replace(" ▼", ""); text = text.replace(" ▲", ""); + text = text.trim(); row.push(text); } else { - const col = cols[j] as HTMLElement; + let text = (cols[j] as HTMLTableCellElement).innerText; + text = text.trim(); + row.push(text); + } - // this is super brittle - if (col.children.length > 0 && col.children[0] instanceof HTMLAnchorElement) { - row.push((col.children[0] as HTMLAnchorElement).href); + if (colsWithMetadata.indexOf(j) >= 0) { + if (i === 0) { + // header row + // 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 { - row.push(col.innerText); + // regular row + row.push(this.extractMetadata(cols[j] as HTMLElement)); } } } @@ -328,6 +384,40 @@ export class SortableTable { 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; } @@ -335,8 +425,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"); } /** diff --git a/packages/portal/frontend/src/app/views/AbstractStudentView.ts b/packages/portal/frontend/src/app/views/AbstractStudentView.ts index 99b087638..0c8241daf 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 + + // 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; + } - 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); 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/AdminGradesTab.ts b/packages/portal/frontend/src/app/views/AdminGradesTab.ts index b7316c0d0..4b40b2761 100644 --- a/packages/portal/frontend/src/app/views/AdminGradesTab.ts +++ b/packages/portal/frontend/src/app/views/AdminGradesTab.ts @@ -125,7 +125,13 @@ 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) { + // 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) { scoreText = grade.score.toFixed(2); if (grade.score < 100) { // two decimal places 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-"); 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-"; } 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"); 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"] %>/"; } diff --git a/plugins/default/portal/frontend/html/admin.html b/plugins/default/portal/frontend/html/admin.html index 3b5193b01..cd63e28d9 100644 --- a/plugins/default/portal/frontend/html/admin.html +++ b/plugins/default/portal/frontend/html/admin.html @@ -305,13 +305,12 @@
The CSV must have a header row that contains the following columns: -
CSID,GRADE,COMMENT
+
[CSID|CWL|GITHUB|STUDENTNUMBER],GRADE
- 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. + A COMMENT column is optionally used to provide feedback to the student, if present.
@@ -1155,7 +1154,7 @@