From 233659a32ece9f6f4f4f730a203b612315330054 Mon Sep 17 00:00:00 2001 From: "Reid Holmes (MBA)" Date: Thu, 20 Dec 2018 13:22:52 -0800 Subject: [PATCH] see #180 coursecontroller can now stand on its own, 310controller is just coursecontroller, pass lateautotest to backend. --- .../src/autotest/mocks/MockClassPortal.ts | 3 +- .../autotest/src/github/GitHubAutoTest.ts | 6 +- packages/common/types/PortalTypes.ts | 5 + .../src/controllers/CourseController.ts | 79 ++++-- .../src/controllers/cs310/CS310Controller.ts | 262 +++++++++--------- .../src/server/common/AutoTestRoutes.ts | 21 +- .../src/app/views/AdminDeliverablesTab.ts | 3 +- 7 files changed, 209 insertions(+), 170 deletions(-) diff --git a/packages/autotest/src/autotest/mocks/MockClassPortal.ts b/packages/autotest/src/autotest/mocks/MockClassPortal.ts index dbae0b4f8..d0fc6140c 100644 --- a/packages/autotest/src/autotest/mocks/MockClassPortal.ts +++ b/packages/autotest/src/autotest/mocks/MockClassPortal.ts @@ -55,7 +55,8 @@ export class MockClassPortal implements IClassPortal { regressionDelivIds: [], custom: {}, openTimestamp: new Date(2018, 1, 1).getTime(), - closeTimestamp: new Date(2018, 6, 1).getTime() + closeTimestamp: new Date(2018, 6, 1).getTime(), + lateAutoTest: false }; } Log.error('MockClassPortal::getContainerDetails() - MockClassPortal should not be used with: ' + name); diff --git a/packages/autotest/src/github/GitHubAutoTest.ts b/packages/autotest/src/github/GitHubAutoTest.ts index 1cc2b6478..32ecae3fa 100644 --- a/packages/autotest/src/github/GitHubAutoTest.ts +++ b/packages/autotest/src/github/GitHubAutoTest.ts @@ -1,3 +1,4 @@ +import * as Docker from "dockerode"; import Config, {ConfigKey} from "../../../common/Config"; import Log from "../../../common/Log"; import {AutoTestResult, IFeedbackGiven} from "../../../common/types/AutoTestTypes"; @@ -11,8 +12,6 @@ import { } from "../../../common/types/PortalTypes"; import Util from "../../../common/Util"; import {AutoTest} from "../autotest/AutoTest"; - -import * as Docker from "dockerode"; import {IClassPortal} from "../autotest/ClassPortal"; import {IDataStore} from "../autotest/DataStore"; import {GitHubService, IGitHubMessage} from "./GitHubService"; @@ -169,7 +168,8 @@ export class GitHubAutoTest extends AutoTest implements IGitHubTestManager { return false; } - if (deliv.closeTimestamp < info.timestamp) { + // late is ok if lateAutoTest is true + if (deliv.closeTimestamp < info.timestamp || deliv.lateAutoTest === false) { Log.warn("GitHubAutoTest::checkCommentPreconditions(..) - ignored, deliverable closed to AutoTest."); // closed const msg = "This deliverable is closed to grading."; diff --git a/packages/common/types/PortalTypes.ts b/packages/common/types/PortalTypes.ts index 9211220a5..81736bd90 100644 --- a/packages/common/types/PortalTypes.ts +++ b/packages/common/types/PortalTypes.ts @@ -200,6 +200,11 @@ export interface AutoTestConfigTransport { openTimestamp: number; closeTimestamp: number; + + /** + * Whether AutoTest can be invoked after the closeTimestamp has passed + */ + lateAutoTest: boolean; } export interface AutoTestAuthPayload { diff --git a/packages/portal/backend/src/controllers/CourseController.ts b/packages/portal/backend/src/controllers/CourseController.ts index 285370362..d222965c5 100644 --- a/packages/portal/backend/src/controllers/CourseController.ts +++ b/packages/portal/backend/src/controllers/CourseController.ts @@ -1,26 +1,9 @@ -import * as rp from "request-promise-native"; - -import Config, {ConfigKey} from "../../../../common/Config"; import Log from "../../../../common/Log"; -import { - AutoTestDashboardTransport, - AutoTestGradeTransport, - AutoTestResultSummaryTransport, - CourseTransport, - DeliverableTransport, - GradeTransport, - ProvisionTransport, - RepositoryTransport, - StudentTransport, - TeamTransport -} from '../../../../common/types/PortalTypes'; -import Util from "../../../../common/Util"; -import {AuditLabel, Course, Deliverable, Grade, Person, PersonKind, Repository, Result, Team} from "../Types"; +import {ProvisionTransport} from '../../../../common/types/PortalTypes'; +import {Deliverable, Grade, Person} from "../Types"; import {DatabaseController} from "./DatabaseController"; -import {DeliverablesController} from "./DeliverablesController"; -import {GitHubActions} from "./GitHubActions"; -import {GitHubController, IGitHubController} from "./GitHubController"; +import {IGitHubController} from "./GitHubController"; import {GradesController} from "./GradesController"; import {PersonController} from "./PersonController"; import {RepositoryController} from "./RepositoryController"; @@ -76,7 +59,10 @@ export interface ICourseController { computeNames(deliv: Deliverable, people: Person[]): Promise<{teamName: string | null, repoName: string | null}>; } -export abstract class CourseController implements ICourseController { +/** + * This is a default course controller for courses that do not want to do anything unusual. + */ +export class CourseController implements ICourseController { protected dbc = DatabaseController.getInstance(); protected pc = new PersonController(); @@ -117,7 +103,11 @@ export abstract class CourseController implements ICourseController { public handleNewAutoTestGrade(deliv: Deliverable, newGrade: Grade, existingGrade: Grade): Promise { Log.info("CourseController::handleNewAutoTestGrade( " + deliv.id + ", " + newGrade.personId + ", " + newGrade.score + ", ... ) - start"); - if ((existingGrade === null || newGrade.score >= existingGrade.score) && newGrade.timestamp <= deliv.closeTimestamp) { + + const gradeIsLarger = (existingGrade === null || newGrade.score >= existingGrade.score); + const gradeBeforeDeadline = newGrade.timestamp <= deliv.closeTimestamp; + + if (gradeIsLarger === true && gradeBeforeDeadline === true) { Log.trace("CourseController::handleNewAutoTestGrade( " + deliv.id + ", " + newGrade.personId + ", " + newGrade.score + ", ... ) - returning true"); return Promise.resolve(true); @@ -128,7 +118,50 @@ export abstract class CourseController implements ICourseController { } } - public abstract computeNames(deliv: Deliverable, people: Person[]): Promise<{teamName: string | null, repoName: string | null}>; + public async computeNames(deliv: Deliverable, people: Person[]): Promise<{teamName: string | null, repoName: string | null}> { + Log.info('CourseController::computeNames( ' + deliv.id + ', ... ) - start'); + + if (people.length < 1) { + throw new Error("CourseController::computeNames( ... ) - must provide people"); + } + + // sort people alph by their id + people = people.sort(function compare(p1: Person, p2: Person) { + return p1.id.localeCompare(p2.id); + } + ); + + let postfix = ''; + for (const person of people) { + postfix = postfix + '_' + person.githubId; + } + + let tName = ''; + if (deliv.teamPrefix.length > 0) { + tName = deliv.teamPrefix + '_' + deliv.id + postfix; + } else { + tName = deliv.id + postfix; + } + + let rName = ''; + if (deliv.repoPrefix.length > 0) { + rName = deliv.repoPrefix + '_' + deliv.id + postfix; + } else { + rName = deliv.id + postfix; + } + + const db = DatabaseController.getInstance(); + const team = await db.getTeam(tName); + const repo = await db.getRepository(rName); + + if (team === null && repo === null) { + Log.info('CourseController::computeNames( ... ) - done; t: ' + tName + ', r: ' + rName); + return {teamName: tName, repoName: rName}; + } else { + // TODO: should really verify that the existing teams contain the right people already + return {teamName: tName, repoName: rName}; + } + } // NOTE: the default implementation is currently broken; do not use it. /** diff --git a/packages/portal/backend/src/controllers/cs310/CS310Controller.ts b/packages/portal/backend/src/controllers/cs310/CS310Controller.ts index cf71d330c..341bf25c7 100644 --- a/packages/portal/backend/src/controllers/cs310/CS310Controller.ts +++ b/packages/portal/backend/src/controllers/cs310/CS310Controller.ts @@ -1,27 +1,25 @@ import Log from "../../../../../common/Log"; -import {Deliverable, Grade, Person} from "../../Types"; +import {Deliverable, Grade} from "../../Types"; import {CourseController} from "../CourseController"; -import {DatabaseController} from "../DatabaseController"; import {IGitHubController} from "../GitHubController"; export class CS310Controller extends CourseController { - private readonly PROJD0 = 'd0'; - private readonly PROJD1 = 'd1'; - private readonly PROJD2 = 'd2'; - private readonly PROJD3 = 'd3'; - private readonly PROJD4 = 'd4'; - private readonly PROJ = 'proj'; // TECHDEBT: 310; should be 'project' + // private readonly PROJD0 = 'd0'; + // private readonly PROJD1 = 'd1'; + // private readonly PROJD2 = 'd2'; + // private readonly PROJD3 = 'd3'; + // private readonly PROJD4 = 'd4'; public constructor(ghController: IGitHubController) { super(ghController); Log.info("CS310Controller::"); } - public async handleUnknownUser(githubUsername: string): Promise { - Log.info("CS310Controller:handleUnknownUser( " + githubUsername + " ) - returning null"); - return null; - } + // public async handleUnknownUser(githubUsername: string): Promise { + // Log.info("CS310Controller:handleUnknownUser( " + githubUsername + " ) - returning null"); + // return null; + // } /** * Students get their highest grade before the deadline. @@ -33,70 +31,70 @@ export class CS310Controller extends CourseController { * @param {Grade} existingGrade * @returns {boolean} */ - public async handleNewAutoTestGrade(deliv: Deliverable, newGrade: Grade, existingGrade: Grade): Promise { - // just use the default implementation - let updateGrade = await super.handleNewAutoTestGrade(deliv, newGrade, existingGrade); - Log.info("CS310Controller::handleNewAutoTestGrade(..) - pId: " + newGrade.personId + - "; delivId: " + deliv.id + "; higher? " + updateGrade); - - // handlenewAutoTestGradeDefault checks deliverable deadline - // but after grades are released the deadline might be extended, so this allows for that - if (deliv.gradesReleased === true) { - // drop updates if grades have been released - // could use closeTimetamp, but this lets us run the deliverables for the whole term - // once the grades have been released - - Log.info("CS310Controller::handleNewAutoTestGrade(..) - pId: " + newGrade.personId + - "; delivId: " + deliv.id + "; grades released - not saving"); - - updateGrade = false; - } - - if (updateGrade === true) { - const personId = newGrade.personId; - - let d0Score = await this.getScore(this.PROJD0, personId); - let d1Score = await this.getScore(this.PROJD1, personId); - let d2Score = await this.getScore(this.PROJD2, personId); - let d3Score = await this.getScore(this.PROJD3, personId); - let d4Score = await this.getScore(this.PROJD4, personId); - - // this whole block is awkward because the new grade hasn't been written yet - // if we try to get it, we'll get the old one - if (deliv.id === this.PROJD0) { - d0Score = newGrade.score; - } - if (deliv.id === this.PROJD1) { - d1Score = newGrade.score; - } - if (deliv.id === this.PROJD2) { - d2Score = newGrade.score; - } - if (deliv.id === this.PROJD3) { - d3Score = newGrade.score; - } - if (deliv.id === this.PROJD4) { - d4Score = newGrade.score; - } - let projectScore = (d0Score + d1Score + d2Score + d3Score + d4Score) / 4; // 25% of project score each - projectScore = Number(projectScore.toFixed(2)); - - const pGrade: Grade = { - personId: personId, - delivId: this.PROJ, - score: projectScore, - comment: '', - timestamp: Date.now(), - urlName: null, - URL: null, - custom: {} - - }; - Log.trace("CS310Controller::handleNewAutoTestGrade(..) - pId: " + personId + "; new project score: " + projectScore); - await this.gc.saveGrade(pGrade); - } - return updateGrade; - } + // public async handleNewAutoTestGrade(deliv: Deliverable, newGrade: Grade, existingGrade: Grade): Promise { + // // just use the default implementation + // const updateGrade = await super.handleNewAutoTestGrade(deliv, newGrade, existingGrade); + // Log.info("CS310Controller::handleNewAutoTestGrade(..) - pId: " + newGrade.personId + + // "; delivId: " + deliv.id + "; higher? " + updateGrade); + + // // handlenewAutoTestGradeDefault checks deliverable deadline + // // but after grades are released the deadline might be extended, so this allows for that + // if (deliv.gradesReleased === true) { + // // drop updates if grades have been released + // // could use closeTimetamp, but this lets us run the deliverables for the whole term + // // once the grades have been released + // + // Log.info("CS310Controller::handleNewAutoTestGrade(..) - pId: " + newGrade.personId + + // "; delivId: " + deliv.id + "; grades released - not saving"); + // + // updateGrade = false; + // } + // + // if (updateGrade === true) { + // const personId = newGrade.personId; + // + // let d0Score = await this.getScore(this.PROJD0, personId); + // let d1Score = await this.getScore(this.PROJD1, personId); + // let d2Score = await this.getScore(this.PROJD2, personId); + // let d3Score = await this.getScore(this.PROJD3, personId); + // let d4Score = await this.getScore(this.PROJD4, personId); + // + // // this whole block is awkward because the new grade hasn't been written yet + // // if we try to get it, we'll get the old one + // if (deliv.id === this.PROJD0) { + // d0Score = newGrade.score; + // } + // if (deliv.id === this.PROJD1) { + // d1Score = newGrade.score; + // } + // if (deliv.id === this.PROJD2) { + // d2Score = newGrade.score; + // } + // if (deliv.id === this.PROJD3) { + // d3Score = newGrade.score; + // } + // if (deliv.id === this.PROJD4) { + // d4Score = newGrade.score; + // } + // let projectScore = (d0Score + d1Score + d2Score + d3Score + d4Score) / 4; // 25% of project score each + // projectScore = Number(projectScore.toFixed(2)); + // + // const pGrade: Grade = { + // personId: personId, + // delivId: this.PROJ, + // score: projectScore, + // comment: '', + // timestamp: Date.now(), + // urlName: null, + // URL: null, + // custom: {} + // + // }; + // Log.trace("CS310Controller::handleNewAutoTestGrade(..) - pId: " + personId + "; new project score: " + projectScore); + // await this.gc.saveGrade(pGrade); + // } + // return updateGrade; + // } /** * Returns the grade or 0 if there is not one saved yet. @@ -105,61 +103,61 @@ export class CS310Controller extends CourseController { * @param {string} personId * @returns {Promise} */ - private async getScore(delivId: string, personId: string): Promise { - const grade = await this.gc.getGrade(personId, delivId); - let score = 0; - if (grade === null || typeof grade.score === 'undefined' || grade.score === null || grade.score < 0) { - score = 0; - } else { - score = grade.score; - } - return score; - } - - public async computeNames(deliv: Deliverable, people: Person[]): Promise<{teamName: string | null, repoName: string | null}> { - Log.info('CS310Controller::computeNames( ' + deliv.id + ', ... ) - start'); - - if (people.length < 1) { - throw new Error("CS310Controller::computeNames( ... ) - must provide people"); - } - - // sort people alph by their id - people = people.sort(function compare(p1: Person, p2: Person) { - return p1.id.localeCompare(p2.id); - } - ); - - let postfix = ''; - for (const person of people) { - postfix = postfix + '_' + person.githubId; - } - - let tName = ''; - if (deliv.teamPrefix.length > 0) { - tName = deliv.teamPrefix + '_' + deliv.id + postfix; - } else { - tName = deliv.id + postfix; - } - - let rName = ''; - if (deliv.repoPrefix.length > 0) { - rName = deliv.repoPrefix + '_' + deliv.id + postfix; - } else { - rName = deliv.id + postfix; - } - - const db = DatabaseController.getInstance(); - const team = await db.getTeam(tName); - const repo = await db.getRepository(rName); - - if (team === null && repo === null) { - Log.info('CS310Controller::computeNames( ... ) - done; t: ' + tName + ', r: ' + rName); - return {teamName: tName, repoName: rName}; - } else { - // TODO: should really verify that the existing teams contain the right people already - return {teamName: tName, repoName: rName}; - // throw new Error("CS310Controller::computeNames( ... ) - names not available; t: " + tName + "; r: " + rName); - } - } + // private async getScore(delivId: string, personId: string): Promise { + // const grade = await this.gc.getGrade(personId, delivId); + // let score = 0; + // if (grade === null || typeof grade.score === 'undefined' || grade.score === null || grade.score < 0) { + // score = 0; + // } else { + // score = grade.score; + // } + // return score; + // } + + // public async computeNames(deliv: Deliverable, people: Person[]): Promise<{teamName: string | null, repoName: string | null}> { + // Log.info('CS310Controller::computeNames( ' + deliv.id + ', ... ) - start'); + // + // if (people.length < 1) { + // throw new Error("CS310Controller::computeNames( ... ) - must provide people"); + // } + // + // // sort people alph by their id + // people = people.sort(function compare(p1: Person, p2: Person) { + // return p1.id.localeCompare(p2.id); + // } + // ); + // + // let postfix = ''; + // for (const person of people) { + // postfix = postfix + '_' + person.githubId; + // } + // + // let tName = ''; + // if (deliv.teamPrefix.length > 0) { + // tName = deliv.teamPrefix + '_' + deliv.id + postfix; + // } else { + // tName = deliv.id + postfix; + // } + // + // let rName = ''; + // if (deliv.repoPrefix.length > 0) { + // rName = deliv.repoPrefix + '_' + deliv.id + postfix; + // } else { + // rName = deliv.id + postfix; + // } + // + // const db = DatabaseController.getInstance(); + // const team = await db.getTeam(tName); + // const repo = await db.getRepository(rName); + // + // if (team === null && repo === null) { + // Log.info('CS310Controller::computeNames( ... ) - done; t: ' + tName + ', r: ' + rName); + // return {teamName: tName, repoName: rName}; + // } else { + // // TODO: should really verify that the existing teams contain the right people already + // return {teamName: tName, repoName: rName}; + // // throw new Error("CS310Controller::computeNames( ... ) - names not available; t: " + tName + "; r: " + rName); + // } + // } } diff --git a/packages/portal/backend/src/server/common/AutoTestRoutes.ts b/packages/portal/backend/src/server/common/AutoTestRoutes.ts index 97345bd8c..7b134818b 100644 --- a/packages/portal/backend/src/server/common/AutoTestRoutes.ts +++ b/packages/portal/backend/src/server/common/AutoTestRoutes.ts @@ -83,7 +83,8 @@ export class AutoTestRoutes implements IREST { regressionDelivIds: deliv.autotest.regressionDelivIds, custom: deliv.autotest.custom, openTimestamp: deliv.openTimestamp, - closeTimestamp: deliv.closeTimestamp + closeTimestamp: deliv.closeTimestamp, + lateAutoTest: deliv.lateAutoTest }; payload = {success: at}; res.send(200, payload); @@ -454,14 +455,14 @@ export class AutoTestRoutes implements IREST { const atHost = config.getProp(ConfigKey.autotestUrl); const url = atHost + ':' + config.getProp(ConfigKey.autotestPort) + req.href().replace("/portal/at", ""); const options = { - uri: url, - method: 'GET', - json: true + uri: url, + method: 'GET', + json: true }; const githubId = req.headers.user; const pc = new PersonController(); const person = await pc.getGitHubPerson(githubId); - const privileges = await new AuthController().personPriviliged(person); + const privileges = await new AuthController().personPriviliged(person); if (privileges.isAdmin) { try { const atResponse = await rp(options); @@ -485,15 +486,15 @@ export class AutoTestRoutes implements IREST { const atHost = config.getProp(ConfigKey.autotestUrl); const url = atHost + ':' + config.getProp(ConfigKey.autotestPort) + '/docker/image'; const options = { - uri: url, - method: 'POST', - json: true, - body: req.body + uri: url, + method: 'POST', + json: true, + body: req.body }; const githubId = req.headers.user; const pc = new PersonController(); const person = await pc.getGitHubPerson(githubId); - const privileges = await new AuthController().personPriviliged(person); + const privileges = await new AuthController().personPriviliged(person); if (privileges.isAdmin) { try { // Use native request library. See https://github.com/request/request-promise#api-in-detail. diff --git a/packages/portal/frontend/src/app/views/AdminDeliverablesTab.ts b/packages/portal/frontend/src/app/views/AdminDeliverablesTab.ts index eb99baf6c..457c815c3 100644 --- a/packages/portal/frontend/src/app/views/AdminDeliverablesTab.ts +++ b/packages/portal/frontend/src/app/views/AdminDeliverablesTab.ts @@ -453,7 +453,8 @@ export class AdminDeliverablesTab extends AdminPage { regressionDelivIds, custom: atCustom, openTimestamp, - closeTimestamp + closeTimestamp, + lateAutoTest }; const deliv: DeliverableTransport = {