From b25c5a675f0a326dfd3a569da77573dd96cd7fb7 Mon Sep 17 00:00:00 2001 From: Peter Staab Date: Mon, 16 May 2022 06:26:35 -0400 Subject: [PATCH 01/34] WIP: change Course model to handle fields --- src/common/models/courses.ts | 85 ++++++++++++------------- tests/unit-tests/courses.spec.ts | 106 +++++++++++++++---------------- 2 files changed, 92 insertions(+), 99 deletions(-) diff --git a/src/common/models/courses.ts b/src/common/models/courses.ts index 39fddfc4..fd680b83 100644 --- a/src/common/models/courses.ts +++ b/src/common/models/courses.ts @@ -1,16 +1,16 @@ import { RequiredFieldsException, Model, Dictionary, generic } from 'src/common/models'; -import { parseNonNegInt, parseBoolean, parseUsername, parseUserRole, UserRole } from './parsers'; +import { parseUserRole } from './parsers'; export interface ParseableCourse { - course_id?: number | string; + course_id?: number; course_name?: string; - visible?: string | number | boolean; + visible?: boolean; course_dates? : ParseableCourseDates; } export interface ParseableCourseDates { - start?: number | string; - end?: number | string; + start?: number; + end?: number; } class CourseDates extends Model { @@ -35,10 +35,14 @@ class CourseDates extends Model { } get start() { return this._start; } - set start(value: number | string) { this._start = parseNonNegInt(value); } + set start(value: number) { this._start = value; } get end() { return this._end; } - set end(value: number | string) { this._end = parseNonNegInt(value); } + set end(value: number) { this._end = value; } + + isValid(): boolean { + return this.start <= this.end; + } } export class Course extends Model { @@ -71,7 +75,7 @@ export class Course extends Model { if (params.course_id != undefined) this.course_id = params.course_id; if (params.course_name != undefined) this.course_name = params.course_name; if (params.visible != undefined) this.visible = params.visible; - super.checkParams(params as Dictionary); + // super.checkParams(params as Dictionary); if (params.course_dates) this.setDates(params.course_dates); } @@ -80,23 +84,21 @@ export class Course extends Model { } get course_id(): number { return this._course_id; } - set course_id(value: string | number) { - this._course_id = parseNonNegInt(value); - } + set course_id(value: number) { this._course_id = value; } get course_name() { return this._course_name; } - set course_name(value: string) { - this._course_name = value; - } + set course_name(value: string) { this._course_name = value; } get visible() { return this._visible; } - set visible(value: string | number | boolean) { - this._visible = parseBoolean(value); - } + set visible(value: boolean) { this._visible = value; } clone() { return new Course(this.toObject() as ParseableCourse); } + + isValid(): boolean { + return this.course_name.length > 0 && this.course_id >= 0 && this.course_dates.isValid(); + } } /** @@ -104,11 +106,11 @@ export class Course extends Model { */ export interface ParseableUserCourse { - course_id?: number | string; - user_id?: number | string; + course_id?: number; + user_id?: number; course_name?: string; username?: string; - visible?: number | string | boolean; + visible?: boolean; role?: string; course_dates?: ParseableCourseDates; } @@ -118,7 +120,7 @@ export class UserCourse extends Model { private _course_name = ''; private _username = ''; private _visible = true; - private _role = UserRole.unknown; + private _role = 'UNKNOWN'; private course_dates = new CourseDates(); static ALL_FIELDS = ['course_id', 'course_name', 'visible', 'course_dates', @@ -134,12 +136,6 @@ export class UserCourse extends Model { constructor(params: ParseableUserCourse = {}) { super(); - if (params.course_name == undefined) { - throw new RequiredFieldsException('course_name'); - } - if (params.username == undefined) { - throw new RequiredFieldsException('username'); - } this.set(params); } @@ -148,9 +144,9 @@ export class UserCourse extends Model { if (params.course_name != undefined) this.course_name = params.course_name; if (params.visible != undefined) this.visible = params.visible; if (params.user_id != undefined) this.user_id = params.user_id; - if (params.username != undefined) this.username = params.username; - if (params.role != undefined) this.role = params.role; - super.checkParams(params as Dictionary); + if (params.username) this.username = params.username; + if (params.role) this.role = params.role; + // super.checkParams(params as Dictionary); } setDates(date_params: ParseableCourseDates = {}) { @@ -158,31 +154,28 @@ export class UserCourse extends Model { } get course_id(): number { return this._course_id; } - set course_id(value: string | number) { - this._course_id = parseNonNegInt(value); - } + set course_id(value: number) { this._course_id = value; } get user_id() { return this._user_id; } - set user_id(value: string | number) { - this._user_id = parseNonNegInt(value); - } + set user_id(value: number) { this._user_id = value;} get username() { return this._username;} - set username(value: string) { - this._username = parseUsername(value); - } + set username(value: string) { this._username = value; } get course_name() { return this._course_name; } - set course_name(value: string) { - this._course_name = value; - } + set course_name(value: string) { this._course_name = value; } get visible() { return this._visible; } - set visible(value: string | number | boolean) { - this._visible = parseBoolean(value); - } + set visible(value: boolean) { this._visible = value; } get role() { return this._role; } - set role(value: string) { this._role = parseUserRole(value); } + set role(value: string) { this._role = value; } + isValid(): boolean { + if (this.course_id < 0) return false; + if (this.user_id < 0) return false; + if (!parseUserRole(this.role)) return false; + if (this.course_name.length === 0) return false; + return true; + } } diff --git a/tests/unit-tests/courses.spec.ts b/tests/unit-tests/courses.spec.ts index 87701154..f5a9c975 100644 --- a/tests/unit-tests/courses.spec.ts +++ b/tests/unit-tests/courses.spec.ts @@ -6,13 +6,11 @@ import { InvalidFieldsException } from 'src/common/models'; describe('Test Course Models', () => { - const default_course_dates = { start: 0, end: 0 }; - const default_course = { course_id: 0, course_name: 'Arithmetic', visible: true, - course_dates: { ...default_course_dates } + course_dates: { start: 0, end: 0 } }; describe('Creation of a Course', () => { @@ -23,6 +21,8 @@ describe('Test Course Models', () => { expect(course.toObject()).toStrictEqual(default_course); + expect(course.isValid()).toBe(true); + }); test('Check that calling all_fields() and params() is correct', () => { @@ -44,35 +44,7 @@ describe('Test Course Models', () => { }); - describe('Checking valid and invalid creation parameters.', () => { - - test('Parsing of undefined and null values', () => { - const course1 = new Course({ course_name: 'Arithmetic' }); - const course2 = new Course({ course_name: 'Arithmetic', course_id: undefined }); - expect(course1).toStrictEqual(course2); - - // the following allow to pass in non-valid parameters for testing - const params = { course_name: 'Arithmetic', course_id: null }; - const course3 = new Course(params as unknown as ParseableCourse); - expect(course1).toStrictEqual(course3); - }); - - test('Create a course with invalid params', () => { - // make a generic object and cast it as a Course - const p = { course_name: 'Arithmetic', CourseNumber: -1 } as unknown as ParseableCourse; - expect(() => { new Course(p);}) - .toThrow(InvalidFieldsException); - }); - - test('Course with invalid course_id', () => { - expect(() => { - new Course({ course_name: 'Arithmetic', course_id: -1 }); - }).toThrow(NonNegIntException); - }); - }); - describe('Updating a course', () => { - test('set fields of a course', () => { const course = new Course({ course_name: 'Arithmetic' }); course.course_id = 5; @@ -81,39 +53,67 @@ describe('Test Course Models', () => { course.course_name = 'Geometry'; expect(course.course_name).toBe('Geometry'); - course.visible = true; - expect(course.visible).toBeTruthy(); - - course.visible = 0; - expect(course.visible).toBeFalsy(); - - course.visible = 'true'; - expect(course.visible).toBeTruthy(); - - course.visible = 'false'; - expect(course.visible).toBeFalsy(); + course.visible = false; + expect(course.visible).toBe(false); + expect(course.isValid()).toBe(true); }); test('set fields of a course using the set method', () => { const course = new Course({ course_name: 'Arithmetic' }); - course.set({ course_id: 5 }); + course.set({ + course_id: 5, + course_name: 'Geometry', + visible: false + }); expect(course.course_id).toBe(5); - - course.set({ course_name: 'Geometry' }); expect(course.course_name).toBe('Geometry'); + expect(course.visible).toBe(false); + expect(course.isValid()).toBe(true); + }); + }); - course.set({ visible: true }); - expect(course.visible).toBeTruthy(); + describe('Checking the course dates', () => { + test('checking for valid course dates', () => { + const course = new Course({ + course_name: 'Arithemetic', + course_dates: {start: 100, end: 100} + }); + expect(course.course_dates.isValid()).toBe(true); + expect(course.isValid()).toBe(true); + }); + }); - course.set({ visible: 'true' }); - expect(course.visible).toBeTruthy(); + describe('Checking valid and invalid creation parameters.', () => { - course.set({ visible: 'false' }); - expect(course.visible).toBeFalsy(); + test('Parsing of undefined and null values', () => { + const course1 = new Course({ course_name: 'Arithmetic' }); + const course2 = new Course({ course_name: 'Arithmetic', course_id: undefined }); + expect(course1).toStrictEqual(course2); + + // the following allow to pass in non-valid parameters for testing + const params = { course_name: 'Arithmetic', course_id: null }; + const course3 = new Course(params as unknown as ParseableCourse); + expect(course1).toStrictEqual(course3); + }); + + test('Create a course with invalid fields', () => { + const c1 = new Course({ course_name: 'Arithmetic', course_id: -1 }); + expect(c1.isValid()).toBe(false); + + const c2 = new Course({ course_name: '', course_id: 0 }); + expect(c2.isValid()).toBe(false); + }); + + test('Create a course with invalid dates', () => { + const c1 = new Course({ + course_name: 'Arithmetic', + course_dates: { start: 100, end: 0} + }); + expect(c1.isValid()).toBe(false); - course.set({ visible: 0 }); - expect(course.visible).toBeFalsy(); }); }); + + }); From e0de746fa21b9cce0ee5a86b471a4550710b4414 Mon Sep 17 00:00:00 2001 From: Peter Staab Date: Thu, 19 May 2022 18:33:33 -0400 Subject: [PATCH 02/34] WIP: updating courses and generic problem sets on improvements. --- src/common/models/courses.ts | 17 ++-- src/common/models/parsers.ts | 56 ++++++------ src/common/models/problem_sets.ts | 40 ++++---- tests/unit-tests/courses.spec.ts | 127 ++++++++++++++++++++++++-- tests/unit-tests/problem_sets.spec.ts | 73 +++++++++++++++ 5 files changed, 254 insertions(+), 59 deletions(-) create mode 100644 tests/unit-tests/problem_sets.spec.ts diff --git a/src/common/models/courses.ts b/src/common/models/courses.ts index fd680b83..415cf1a4 100644 --- a/src/common/models/courses.ts +++ b/src/common/models/courses.ts @@ -1,5 +1,5 @@ import { RequiredFieldsException, Model, Dictionary, generic } from 'src/common/models'; -import { parseUserRole } from './parsers'; +import { isNonNegInt, parseUserRole, isValidUserRole, isValidUsername } from './parsers'; export interface ParseableCourse { course_id?: number; @@ -97,7 +97,7 @@ export class Course extends Model { } isValid(): boolean { - return this.course_name.length > 0 && this.course_id >= 0 && this.course_dates.isValid(); + return this.course_name.length > 0 && isNonNegInt(this.course_id) && this.course_dates.isValid(); } } @@ -169,13 +169,14 @@ export class UserCourse extends Model { set visible(value: boolean) { this._visible = value; } get role() { return this._role; } - set role(value: string) { this._role = value; } + set role(value: string) { this._role = value.toUpperCase(); } + + clone(): UserCourse { + return new UserCourse(this.toObject()); + } isValid(): boolean { - if (this.course_id < 0) return false; - if (this.user_id < 0) return false; - if (!parseUserRole(this.role)) return false; - if (this.course_name.length === 0) return false; - return true; + return isNonNegInt(this.course_id) && isNonNegInt(this.user_id) && isValidUsername(this.username) + && isValidUserRole(this.role) && this.course_name.length > 0 && this.course_dates.isValid(); } } diff --git a/src/common/models/parsers.ts b/src/common/models/parsers.ts index 8f363a5d..90beb804 100644 --- a/src/common/models/parsers.ts +++ b/src/common/models/parsers.ts @@ -3,6 +3,8 @@ * for all of webwork3. */ +import { isValid } from 'ipaddr.js'; + /** * ParseError is a general Error class for any parsing errors. */ @@ -109,41 +111,40 @@ export class UserRoleException extends ParseError { } } -// Parsing functions +// Parsing Regular Expressions + +export const non_neg_int_re = /^\s*(\d+)\s*$/; +export const non_neg_decimal_re = /(^\s*(\d+)(\.\d*)?\s*$)|(^\s*\.\d+\s*$)/; +export const mail_re = /^[\w.]+@([a-zA-Z_.]+)+\.[a-zA-Z]{2,9}$/; +export const username_re = /^[_a-zA-Z]([a-zA-Z._0-9])+$/; + +// Checking functions + +export const isNonNegInt = (v: number | string) => non_neg_int_re.test(`${v}`); +export const isNonNegDecimal = (v: number | string) => non_neg_decimal_re.test(`${v}`); +export const isValidUsername = (v: string) => username_re.test(v) || mail_re.test(v); +export const isValidEmail = (v: string) => mail_re.test(v); + +// Parsing functionis export function parseNonNegInt(val: string | number) { - if (/^\s*(\d+)\s*$/.test(`${val}`)) { - return parseInt(`${val}`); - } else { - throw new NonNegIntException(`The value ${val} is not a non-negative integer`); - } + if (isNonNegInt(val)) return parseInt(`${val}`); + throw new NonNegIntException(`The value ${val} is not a non-negative integer`); } export function parseNonNegDecimal(val: string | number) { - if (/(^\s*(\d+)(\.\d*)?\s*$)|(^\s*\.\d+\s*$)/.test(`${val}`)) { - return parseFloat(`${val}`); - } else { - throw new NonNegDecimalException(`The value ${val} is not a non-negative decimal`); - } + if (isNonNegDecimal(val)) return parseFloat(`${val}`); + throw new NonNegDecimalException(`The value ${val} is not a non-negative decimal`); } -export const mailRE = /^[\w.]+@([a-zA-Z_.]+)+\.[a-zA-Z]{2,9}$/; -export const usernameRE = /^[_a-zA-Z]([a-zA-Z._0-9])+$/; - -export function parseUsername(val: string | undefined) { - if (typeof val === 'string' && (val === '' || mailRE.test(`${val ?? ''}`) || usernameRE.test(`${val}`))) { - return val; - } else { - throw new UsernameParseException(`The value '${val?.toString() ?? ''}' is not a value username`); - } +export function parseUsername(val: string) { + if (isValidUsername(val)) return val; + throw new UsernameParseException(`The value '${val?.toString() ?? ''}' is not a value username`); } -export function parseEmail(val: string | undefined) { - if (typeof val === 'string' && mailRE.test(`${val}`)) { - return val; - } else { - throw new EmailParseException(`The value '${val?.toString() ?? ''}' is not a value email`); - } +export function parseEmail(val: string) { + if (isValidEmail(val)) return val; + throw new EmailParseException(`The value '${val?.toString() ?? ''}' is not a value email`); } const booleanRE = /^([01])|(true)|(false)$/; @@ -177,6 +178,9 @@ export enum UserRole { unknown = 'UNKNOWN' } +const user_roles = ['admin', 'instructor','ta','student','unknown']; +export const isValidUserRole = (v: string) => user_roles.includes(v.toLowerCase()); + export function parseUserRole(role: string): UserRole { if (role.toLocaleLowerCase() === 'admin') return UserRole.admin; if (role.toLocaleLowerCase() === 'instructor') return UserRole.instructor; diff --git a/src/common/models/problem_sets.ts b/src/common/models/problem_sets.ts index 33ed00d1..2bcf1624 100644 --- a/src/common/models/problem_sets.ts +++ b/src/common/models/problem_sets.ts @@ -1,6 +1,5 @@ /* These are Problem Set interfaces */ -import { logger } from 'src/boot/logger'; import { Model } from '.'; import { parseBoolean, ParseError, parseNonNegInt } from './parsers'; @@ -11,6 +10,10 @@ export enum ProblemSetType { UNKNOWN = 'UNKNOWN' } +const set_types = ['hw','quiz','review','unknown'] + +export const isValidProblemSetType = (value: string) => set_types.includes(value); + /** * This takes in a general problem set and returns the specific subclassed ProblemSet * @param problem ParseableProblemSet @@ -36,11 +39,11 @@ export type ProblemSetDates = HomeworkSetDates | QuizDates | ReviewSetDates; /* Problem Set (HomeworkSet, Quiz, ReviewSet ) classes */ export interface ParseableProblemSet { - set_id?: string | number; + set_id?: number; set_name?: string; - course_id?: string | number; + course_id?: number; set_type?: string; - set_visible?: string | number | boolean; + set_visible?: boolean; set_params?: ParseableProblemSetParams; set_dates?: ParseableProblemSetDates; } @@ -49,7 +52,7 @@ export class ProblemSet extends Model { private _set_id = 0; private _set_visible = false; private _course_id = 0; - protected _set_type: ProblemSetType = ProblemSetType.UNKNOWN; + protected _set_type = 'UNKNOWN'; private _set_name = ''; constructor(params: ParseableProblemSet = {}) { @@ -68,7 +71,7 @@ export class ProblemSet extends Model { } set(params: ParseableProblemSet) { - if (params.set_id) this.set_id = params.set_id; + if (params.set_id !== undefined) this.set_id = params.set_id; if (params.set_visible !== undefined) this.set_visible = params.set_visible; if (params.course_id != undefined) this.course_id = params.course_id; if (params.set_type) this.set_type = params.set_type; @@ -76,36 +79,35 @@ export class ProblemSet extends Model { } public get set_id(): number { return this._set_id;} - public set set_id(value: number | string) { this._set_id = parseNonNegInt(value);} + public set set_id(value: number) { this._set_id = value; } public get course_id(): number { return this._course_id;} - public set course_id(value: number | string) { this._course_id = parseNonNegInt(value);} + public set course_id(value: number) { this._course_id = value;} public get set_visible() : boolean { return this._set_visible;} - public set set_visible(value: number | string | boolean) { this._set_visible = parseBoolean(value);} + public set set_visible(value: boolean) { this._set_visible = value;} - public get set_type(): ProblemSetType { return this._set_type; } - public set set_type(value: string) { - if (value === 'HW') { this._set_type = ProblemSetType.HW;} - else if (value === 'QUIZ') { this._set_type = ProblemSetType.QUIZ;} - else if (value === 'REVIEW') { this._set_type = ProblemSetType.REVIEW_SET;} - else { this._set_type = ProblemSetType.UNKNOWN; } - } + public get set_type(): string { return this._set_type; } + public set set_type(value: string) { this._set_type = value; } public get set_name() : string { return this._set_name;} public set set_name(value: string) { this._set_name = value;} public get set_params(): ProblemSetParams { - throw 'The subclass must override set_params();'; + throw 'The subclass must override set_params()'; } public get set_dates(): ProblemSetDates { - throw 'The subclass must override set_dates();'; + throw 'The subclass must override set_dates()'; } hasValidDates() { throw 'The hasValidDates() method must be overridden.'; } + + isValid() { + throw 'The isValid() method must be overridden.'; + } } export interface ParseableQuizDates { @@ -525,6 +527,6 @@ export function convertSet(old_set: ProblemSet, new_set_type: ProblemSetType) { throw new ParseError('ProblemSetType', `convertSet does not support conversion to ${new_set_type || 'EMPTY'}`); } - if (!new_set.hasValidDates()) logger.error('[problem_sets/convertSet] corrupt dates in conversion of set, TSNH?'); + // if (!new_set.hasValidDates()) logger.error('[problem_sets/convertSet] corrupt dates in conversion of set, TSNH?'); return new_set; } diff --git a/tests/unit-tests/courses.spec.ts b/tests/unit-tests/courses.spec.ts index f5a9c975..fa2b1aef 100644 --- a/tests/unit-tests/courses.spec.ts +++ b/tests/unit-tests/courses.spec.ts @@ -1,8 +1,4 @@ -// tests parsing and handling of users - -import { Course, ParseableCourse } from 'src/common/models/courses'; -import { NonNegIntException } from 'src/common/models/parsers'; -import { InvalidFieldsException } from 'src/common/models'; +import { Course, ParseableCourse, UserCourse } from 'src/common/models/courses'; describe('Test Course Models', () => { @@ -111,9 +107,128 @@ describe('Test Course Models', () => { course_dates: { start: 100, end: 0} }); expect(c1.isValid()).toBe(false); - }); }); + describe('Creating a UserCourse', () => { + const default_user_course = { + course_id: 0, + user_id: 0, + course_name: '', + username: '', + visible: true, + role: 'UNKNOWN', + course_dates : { start: 0, end: 0 } + }; + + describe('Creating a User Course', () => { + test('Create a Valid Course', () => { + const user_course = new UserCourse(); + expect(user_course).toBeInstanceOf(UserCourse); + expect(user_course.toObject()).toStrictEqual(default_user_course); + // The user course is not valid because the course name and username are empty strings + expect(user_course.isValid()).toBe(false); + }); + + test('Check that calling all_fields() and params() is correct', () => { + const user_course_fields = ['course_id', 'user_id', 'course_name', 'username', + 'visible', 'role', 'course_dates']; + const user_course = new UserCourse(); + expect(user_course.all_field_names.sort()).toStrictEqual(user_course_fields.sort()); + expect(UserCourse.ALL_FIELDS.sort()).toStrictEqual(user_course_fields.sort()); + expect(user_course.param_fields).toStrictEqual(['course_dates']); + }); + + test('Check that cloning works', () => { + const user_course = new UserCourse(); + expect(user_course.clone().toObject()).toStrictEqual(default_user_course); + expect(user_course.clone()).toBeInstanceOf(UserCourse); + }); + }); + + describe('Updating a UserCourse', () => { + test('set fields of a user course', () => { + const user_course = new UserCourse({ course_name: 'Arithmetic' }); + user_course.course_id = 5; + expect(user_course.course_id).toBe(5); + + user_course.course_name = 'Geometry'; + expect(user_course.course_name).toBe('Geometry'); + + user_course.visible = false; + expect(user_course.visible).toBe(false); + + user_course.username = 'homer'; + expect(user_course.username).toBe('homer'); + + user_course.role = 'student'; + expect(user_course.role).toBe('STUDENT'); + + expect(user_course.isValid()).toBe(true); + }); + }); + + describe('Checking valid and invalid creation parameters.', () => { + test('Parsing of undefined and null values', () => { + const course1 = new UserCourse({ course_name: 'Arithmetic' }); + const course2 = new UserCourse({ + course_name: 'Arithmetic', + user_id: undefined, + course_id: undefined + }); + expect(course1).toStrictEqual(course2); + + // the following allow to pass in non-valid parameters for testing + const params = { course_name: 'Arithmetic', course_id: null }; + const course3 = new UserCourse(params as unknown as ParseableCourse); + expect(course1).toStrictEqual(course3); + }); + test('Create a course with invalid fields', () => { + const c1 = new UserCourse({ course_name: 'Arithmetic', username: 'homer' }); + expect(c1.isValid()).toBe(true); + + c1.course_name = ''; + expect(c1.isValid()).toBe(false); + + c1.set({course_name: 'Arithmetic', user_id: -1}); + expect(c1.isValid()).toBe(false); + + c1.set({user_id: 10, course_id: -1}); + expect(c1.isValid()).toBe(false); + + c1.course_id = 10; + expect(c1.isValid()).toBe(true); + + c1.role ='wizard'; + expect(c1.isValid()).toBe(false); + + c1.role ='ta'; + expect(c1.isValid()).toBe(true); + + c1.username = ''; + expect(c1.isValid()).toBe(false); + + c1.username = 'invalid user'; + expect(c1.isValid()).toBe(false); + + c1.username = 'homer@msn.com'; + expect(c1.isValid()).toBe(true); + + }); + + test('Create a user course with invalid dates', () => { + const c1 = new UserCourse({ + course_name: 'Arithmetic', + username: 'homer', + course_dates: { start: 100, end: 200} + }); + expect(c1.isValid()).toBe(true); + + c1.setDates({start: 100, end: 0}); + expect(c1.isValid()).toBe(false); + }); + + }); + }); }); diff --git a/tests/unit-tests/problem_sets.spec.ts b/tests/unit-tests/problem_sets.spec.ts new file mode 100644 index 00000000..ba338ebb --- /dev/null +++ b/tests/unit-tests/problem_sets.spec.ts @@ -0,0 +1,73 @@ +import { ProblemSet } from 'src/common/models/problem_sets'; + +describe('Test generic ProblemSets', () => { + const default_problem_set = { + set_id: 0, + set_name: 'set #1', + course_id: 0, + set_type: 'UNKNOWN', + set_params: {}, + set_dates: {} + } + + describe('Creation of a ProblemSet', () => { + test('Create a valid ProblemSet', () => { + const set = new ProblemSet(); + expect(set).toBeInstanceOf(ProblemSet); + }); + + test('Ensure that there are overrides', () => { + const set = new ProblemSet(); + expect(() => {set.set_params;}).toThrowError('The subclass must override set_params()'); + expect(() => {set.set_dates; }).toThrowError('The subclass must override set_dates()'); + expect(() => {set.hasValidDates(); }).toThrowError('The hasValidDates() method must be overridden.'); + expect(() => {set.isValid(); }).toThrowError('The isValid() method must be overridden.'); + expect(() => {set.clone(); }).toThrowError('The clone method must be overridden in a subclass.'); + }); + }); + + + describe('Check setting generic fields', () => { + test('Check that all fields can be set directly', () => { + const set = new ProblemSet(); + set.set_id = 5; + expect(set.set_id).toBe(5); + + set.course_id = 10; + expect(set.course_id).toBe(10); + + set.set_visible = true; + expect(set.set_visible).toBe(true); + + set.set_name = 'Set #1'; + expect(set.set_name).toBe('Set #1'); + }); + + test('Check that calling all_fields() and params() is correct', () => { + const set_fields = ['set_id', 'set_visible', 'course_id', 'set_type', + 'set_name', 'set_params', 'set_dates']; + const problem_set = new ProblemSet(); + + expect(problem_set.all_field_names.sort()).toStrictEqual(set_fields.sort()); + expect(problem_set.param_fields.sort()).toStrictEqual(['set_dates', 'set_params']); + expect(ProblemSet.ALL_FIELDS.sort()).toStrictEqual(set_fields.sort()); + + }); + + + test('Check that all fields can be set using the set() method', () => { + const set = new ProblemSet(); + set.set({set_id: 5}); + expect(set.set_id).toBe(5); + + set.set({course_id: 10}); + expect(set.course_id).toBe(10); + + set.set({set_visible: true}); + expect(set.set_visible).toBe(true); + + set.set({set_name: 'Set #1'}); + expect(set.set_name).toBe('Set #1'); + }); + }); +}); From b7ff6fb8c07f53e821a1ca686ed488a408b7d193 Mon Sep 17 00:00:00 2001 From: Peter Staab Date: Fri, 20 May 2022 07:04:22 -0400 Subject: [PATCH 03/34] WIP: more work on switching models. --- src/common/models/courses.ts | 2 +- src/common/models/parsers.ts | 6 +- src/common/models/problem_sets.ts | 5 +- src/common/models/users.ts | 228 ++++++++-------- tests/unit-tests/course_users.spec.ts | 363 +++++++++++++++----------- tests/unit-tests/courses.spec.ts | 21 +- tests/unit-tests/problem_sets.spec.ts | 20 +- tests/unit-tests/users.spec.ts | 205 ++++++--------- 8 files changed, 411 insertions(+), 439 deletions(-) diff --git a/src/common/models/courses.ts b/src/common/models/courses.ts index 415cf1a4..436624ed 100644 --- a/src/common/models/courses.ts +++ b/src/common/models/courses.ts @@ -1,5 +1,5 @@ import { RequiredFieldsException, Model, Dictionary, generic } from 'src/common/models'; -import { isNonNegInt, parseUserRole, isValidUserRole, isValidUsername } from './parsers'; +import { isNonNegInt, isValidUserRole, isValidUsername } from './parsers'; export interface ParseableCourse { course_id?: number; diff --git a/src/common/models/parsers.ts b/src/common/models/parsers.ts index 90beb804..f7e193b7 100644 --- a/src/common/models/parsers.ts +++ b/src/common/models/parsers.ts @@ -3,8 +3,6 @@ * for all of webwork3. */ -import { isValid } from 'ipaddr.js'; - /** * ParseError is a general Error class for any parsing errors. */ @@ -178,7 +176,7 @@ export enum UserRole { unknown = 'UNKNOWN' } -const user_roles = ['admin', 'instructor','ta','student','unknown']; +const user_roles = ['admin', 'instructor', 'ta', 'student', 'unknown']; export const isValidUserRole = (v: string) => user_roles.includes(v.toLowerCase()); export function parseUserRole(role: string): UserRole { @@ -186,7 +184,7 @@ export function parseUserRole(role: string): UserRole { if (role.toLocaleLowerCase() === 'instructor') return UserRole.instructor; if (role.toLocaleLowerCase() === 'ta') return UserRole.ta; if (role.toLocaleLowerCase() === 'student') return UserRole.student; - throw new UserRoleException(`The value '${role}' is not a valid role.`); + return UserRole.unknown; } export function parseString(_value: string | number | boolean) { diff --git a/src/common/models/problem_sets.ts b/src/common/models/problem_sets.ts index 2bcf1624..683571be 100644 --- a/src/common/models/problem_sets.ts +++ b/src/common/models/problem_sets.ts @@ -10,7 +10,7 @@ export enum ProblemSetType { UNKNOWN = 'UNKNOWN' } -const set_types = ['hw','quiz','review','unknown'] +const set_types = ['hw', 'quiz', 'review', 'unknown']; export const isValidProblemSetType = (value: string) => set_types.includes(value); @@ -527,6 +527,7 @@ export function convertSet(old_set: ProblemSet, new_set_type: ProblemSetType) { throw new ParseError('ProblemSetType', `convertSet does not support conversion to ${new_set_type || 'EMPTY'}`); } - // if (!new_set.hasValidDates()) logger.error('[problem_sets/convertSet] corrupt dates in conversion of set, TSNH?'); + // if (!new_set.hasValidDates()) + // logger.error('[problem_sets/convertSet] corrupt dates in conversion of set, TSNH?'); return new_set; } diff --git a/src/common/models/users.ts b/src/common/models/users.ts index 13715e1c..f29c66d6 100644 --- a/src/common/models/users.ts +++ b/src/common/models/users.ts @@ -1,16 +1,15 @@ - -import { parseNonNegInt, parseUsername, parseBoolean, parseEmail, parseUserRole, - UserRole } from 'src/common/models/parsers'; -import { RequiredFieldsException, Model } from 'src/common/models'; +import { isNonNegInt, isValidUsername, isValidEmail, parseNonNegInt, parseUsername, + parseBoolean, parseEmail, parseUserRole, UserRole } from 'src/common/models/parsers'; +import { Model } from 'src/common/models'; export interface ParseableUser { - user_id?: number | string; + user_id?: number; username?: string; email?: string; first_name?: string; last_name?: string; - is_admin?: string | number | boolean; - student_id?: string | number; + is_admin?: boolean; + student_id?: string; } /** @@ -20,10 +19,10 @@ export class User extends Model { private _user_id = 0; private _username = ''; private _is_admin = false; - private _email?: string; - private _first_name?: string; - private _last_name?: string; - private _student_id?: string; + private _email = ''; + private _first_name = ''; + private _last_name = ''; + private _student_id = ''; static ALL_FIELDS = ['user_id', 'username', 'is_admin', 'email', 'first_name', 'last_name', 'student_id']; @@ -33,16 +32,13 @@ export class User extends Model { constructor(params: ParseableUser = {}) { super(); - if (params.username == undefined) { - throw new RequiredFieldsException('username'); - } this.set(params); } set(params: ParseableUser) { if (params.username) this.username = params.username; - if (params.user_id) this.user_id = params.user_id; - if (params.is_admin) this.is_admin = params.is_admin; + if (params.user_id != undefined) this.user_id = params.user_id; + if (params.is_admin != undefined) this.is_admin = params.is_admin; if (params.email) this.email = params.email; if (params.first_name) this.first_name = params.first_name; if (params.last_name) this.last_name = params.last_name; @@ -51,52 +47,44 @@ export class User extends Model { // otherwise, typescript identifies this as number | string get user_id(): number { return this._user_id; } - set user_id(value: number | string) { - this._user_id = parseNonNegInt(value); - } + set user_id(value: number) { this._user_id = value; } get username() { return this._username; } - set username(value: string) { - this._username = parseUsername(value); - } + set username(value: string) { this._username = value; } get is_admin() { return this._is_admin; } - set is_admin(value: string | number | boolean) { - this._is_admin = parseBoolean(value); - } + set is_admin(value: boolean) { this._is_admin = value; } - get email(): string | undefined { return this._email; } - set email(value: string | undefined) { - if (value != undefined) this._email = parseEmail(value); - } + get email(): string { return this._email; } + set email(value: string) { this._email = value; } - get first_name(): string | undefined { return this._first_name; } - set first_name(value: string | undefined) { - if (value != undefined) this._first_name = value; - } + get first_name(): string { return this._first_name; } + set first_name(value: string) { this._first_name = value; } - get last_name(): string | undefined { return this._last_name; } - set last_name(value: string | undefined) { - if (value != undefined) this._last_name = value; - } + get last_name(): string { return this._last_name; } + set last_name(value: string) { this._last_name = value; } - get student_id(): string | undefined { return this._student_id; } - set student_id(value: string | number | undefined) { - if (value != undefined) this._student_id = `${value}`; - } + get student_id(): string { return this._student_id; } + set student_id(value: string) { this._student_id = value; } clone() { return new User(this.toObject() as ParseableUser); } + + // The email can be the empty string or valid email address. + isValid(): boolean { + return isNonNegInt(this.user_id) && isValidUsername(this.username) && + (this.email === '' || isValidEmail(this.email)); + } } export interface ParseableDBCourseUser { - course_user_id?: number | string; - user_id?: number | string; - course_id?: number | string; + course_user_id?: number; + user_id?: number; + course_id?: number; role?: string; - section?: string | number; - recitation?: string | number; + section?: string; + recitation?: string; } /** @@ -131,53 +119,52 @@ export class DBCourseUser extends Model { } get course_user_id(): number { return this._course_user_id; } - set course_user_id(value: string | number) { - this._course_user_id = parseNonNegInt(value); - } - - get course_id(): number | undefined { return this._course_id; } - set course_id(value: string | number | undefined) { - if (value != undefined) this._course_id = parseNonNegInt(value); - } + set course_user_id(value: number) { this._course_user_id = value; } - get user_id() { return this._user_id; } - set user_id(value: number | string | undefined) { - if (value != undefined) this._user_id = parseNonNegInt(value); - } + get course_id(): number { return this._course_id; } + set course_id(value: number) { this._course_id = value; } - get role(): string | undefined { return this._role; } - set role(value: string | undefined) { - if (value != undefined) this._role = parseUserRole(value); + get user_id(): number { return this._user_id; } + set user_id(value: number) { this._user_id = value; } + + get role(): UserRole { return this._role; } + set role(value: UserRole | string) { + if (typeof value === 'string') { + this._role = parseUserRole(value); + } else { + this._role = value; + } } get section(): string | undefined { return this._section; } - set section(value: string | number | undefined) { - if (value != undefined) this._section = `${value}`; - } + set section(value: string | undefined) { if (value != undefined) this._section = value; } get recitation(): string | undefined { return this._recitation; } - set recitation(value: string | number | undefined) { - if (value != undefined) this._recitation = `${value}`; - } + set recitation(value: string | undefined) { if (value != undefined) this._recitation = value; } clone() { return new DBCourseUser(this.toObject() as ParseableDBCourseUser); } + + isValid(): boolean { + return isNonNegInt(this.user_id) && isNonNegInt(this.course_user_id) && + isNonNegInt(this.course_id); + } } export interface ParseableCourseUser { - course_user_id?: number | string; - user_id?: number | string; - course_id?: number | string; + course_user_id?: number; + user_id?: number; + course_id?: number; username?: string; email?: string; first_name?: string; last_name?: string; - is_admin?: boolean | number | string; + is_admin?: boolean; student_id?: string; - role?: string; - section?: string | number; - recitation?: string | number; + role?: string | UserRole; + section?: string; + recitation?: string; } /** @@ -191,12 +178,12 @@ export class CourseUser extends Model { private _course_id = 0; private _user_id = 0; private _is_admin = false; - private _username?: string; - private _email?: string; - private _first_name?: string; - private _last_name?: string; - private _student_id?: string; - private _role?: UserRole; + private _username = ''; + private _email = ''; + private _first_name = ''; + private _last_name = ''; + private _student_id = ''; + private _role = UserRole.unknown; private _section?: string; private _recitation?: string; @@ -211,10 +198,10 @@ export class CourseUser extends Model { this.set(params); } set(params: ParseableCourseUser) { - if (params.course_user_id) this.course_user_id = params.course_user_id; - if (params.course_id) this.course_id = params.course_id; - if (params.user_id) this.user_id = params.user_id; - if (params.is_admin) this.is_admin = params.is_admin; + if (params.course_user_id != undefined) this.course_user_id = params.course_user_id; + if (params.course_id != undefined) this.course_id = params.course_id; + if (params.user_id != undefined) this.user_id = params.user_id; + if (params.is_admin != undefined) this.is_admin = params.is_admin; if (params.username) this.username = params.username; if (params.email) this.email = params.email; if (params.first_name) this.first_name = params.first_name; @@ -226,66 +213,55 @@ export class CourseUser extends Model { } get course_user_id(): number { return this._course_user_id; } - set course_user_id(value: string | number) { - this._course_user_id = parseNonNegInt(value); - } + set course_user_id(value: number) { this._course_user_id = value; } get course_id(): number { return this._course_id; } - set course_id(value: string | number) { - this._course_id = parseNonNegInt(value); - } + set course_id(value: number) { this._course_id = value; } get user_id(): number { return this._user_id; } - set user_id(value: number | string) { - this._user_id = parseNonNegInt(value); - } + set user_id(value: number) { this._user_id = value; } - get username() { return this._username; } - set username(value: string | undefined) { - if (value != undefined) this._username = parseUsername(value); - } + get username(): string { return this._username; } + set username(value: string) { this._username = value; } - get is_admin() { return this._is_admin; } - set is_admin(value: string | number | boolean | undefined) { - if (value != undefined) this._is_admin = parseBoolean(value); - } + get is_admin(): boolean { return this._is_admin; } + set is_admin(value: boolean) { this._is_admin = value; } - get email(): string | undefined { return this._email; } - set email(value: string | undefined) { - if (value != undefined) this._email = parseEmail(value); - } + get email(): string { return this._email; } + set email(value: string) { this._email = value; } - get role(): string | undefined { return this._role; } - set role(value: string | undefined) { - if (value != undefined) this._role = parseUserRole(value); + get role(): UserRole { return this._role; } + set role(value: UserRole | string) { + if (typeof value === 'string') { + this._role = parseUserRole(value); + } else { + this._role = value; + } } get section(): string | undefined { return this._section; } - set section(value: string | number | undefined) { - if (value != undefined) this._section = `${value}`; - } + set section(value: string | undefined) { if (value != undefined) this._section = value; } get recitation(): string | undefined { return this._recitation; } - set recitation(value: string | number | undefined) { - if (value != undefined) this._recitation = `${value}`; - } + set recitation(value: string | undefined) { if (value != undefined) this._recitation = value; } - get first_name(): string | undefined { return this._first_name; } - set first_name(value: string | undefined) { - if (value != undefined) this._first_name = value; - } + get first_name(): string { return this._first_name; } + set first_name(value: string) { this._first_name = value; } - get last_name(): string | undefined { return this._last_name; } - set last_name(value: string | undefined) { - if (value != undefined) this._last_name = value; - } + get last_name(): string { return this._last_name; } + set last_name(value: string) { this._last_name = value; } - get student_id(): string | undefined { return this._student_id; } - set student_id(value: string | number | undefined) { - if (value != undefined) this._student_id = `${value}`; - } + get student_id(): string { return this._student_id; } + set student_id(value: string) { this._student_id = value; } clone() { return new CourseUser(this.toObject() as ParseableCourseUser); } + + // The email can be the empty string or valid email address. + isValid(): boolean { + return isNonNegInt(this.user_id) && isNonNegInt(this.course_user_id) && + isNonNegInt(this.course_id) && isValidUsername(this.username) && + (this.email === '' || isValidEmail(this.email)); + } } diff --git a/tests/unit-tests/course_users.spec.ts b/tests/unit-tests/course_users.spec.ts index 30b95d65..9c89358a 100644 --- a/tests/unit-tests/course_users.spec.ts +++ b/tests/unit-tests/course_users.spec.ts @@ -7,118 +7,121 @@ // tests parsing and handling of merged users import { EmailParseException, NonNegIntException, UsernameParseException, - UserRoleException } from 'src/common/models/parsers'; + UserRoleException, UserRole } from 'src/common/models/parsers'; import { CourseUser, DBCourseUser, ParseableDBCourseUser } from 'src/common/models/users'; describe('Testing Database and client-side Course Users', () => { - - const default_db_course_user: ParseableDBCourseUser = { - course_user_id: 0, - user_id: 0, - course_id: 0, - role: 'UNKNOWN' - }; - - describe('Testing Database course users', () => { - - test('Create a new database course user', () => { + describe('Creating a DBCourseUser', () => { + const default_db_course_user = { + course_user_id: 0, + course_id: 0, + user_id: 0, + role: UserRole.unknown + }; + test('Checking the creation of a dBCourseUser', () => { const db_course_user = new DBCourseUser(); expect(db_course_user).toBeInstanceOf(DBCourseUser); expect(db_course_user.toObject()).toStrictEqual(default_db_course_user); + expect(db_course_user.isValid()).toBe(true); }); test('Check that calling all_fields() and params() is correct', () => { - const course_user_fields = ['course_user_id', 'course_id', 'user_id', 'role', + const db_course_user_fields = ['course_user_id', 'course_id', 'user_id', 'role', 'section', 'recitation']; - const course_user = new DBCourseUser(); - - expect(course_user.all_field_names.sort()).toStrictEqual(course_user_fields.sort()); - expect(course_user.param_fields.sort()).toStrictEqual([]); - - expect(DBCourseUser.ALL_FIELDS.sort()).toStrictEqual(course_user_fields.sort()); + const course = new DBCourseUser(); + expect(course.all_field_names.sort()).toStrictEqual(db_course_user_fields.sort()); + expect(course.param_fields.sort()).toStrictEqual([]); + expect(DBCourseUser.ALL_FIELDS.sort()).toStrictEqual(db_course_user_fields.sort()); }); - test('Check that cloning a DBCourseUser works', () => { - const course_user = new DBCourseUser({ role: 'student' }); - const course_user2: ParseableDBCourseUser = { ...default_db_course_user }; - course_user2.role = 'STUDENT'; - expect(course_user.clone().toObject()).toStrictEqual(course_user2); - expect(course_user.clone() instanceof DBCourseUser).toBe(true); + test('Check that cloning works', () => { + const db_course_user = new DBCourseUser(); + expect(db_course_user.clone().toObject()).toStrictEqual(default_db_course_user); + expect(db_course_user.clone()).toBeInstanceOf(DBCourseUser); }); + }); - test('create DBCourseUser with invalid role', () => { - expect(() => { - new DBCourseUser({ role: 'superhero' }); - }).toThrow(UserRoleException); - }); + describe('Updating a DBCourseUser', () => { + test('Update DBCourseUser directly', () => { + const db_course_user = new DBCourseUser(); + expect(db_course_user.isValid()).toBe(true); - test('set fields of DBCourseUser', () => { - const course_user = new DBCourseUser(); + db_course_user.course_user_id = 10; + expect(db_course_user.course_user_id).toBe(10); - course_user.role = 'student'; - expect(course_user.role).toBe('STUDENT'); + db_course_user.user_id = 20; + expect(db_course_user.user_id).toBe(20); - course_user.course_id = 34; - expect(course_user.course_id).toBe(34); + db_course_user.course_id = 5; + expect(db_course_user.course_id).toBe(5); - course_user.user_id = 3; - expect(course_user.user_id).toBe(3); + db_course_user.role = UserRole.admin; + expect(db_course_user.role).toBe(UserRole.admin); - course_user.user_id = '5'; - expect(course_user.user_id).toBe(5); + db_course_user.role = 'student'; + expect(db_course_user.role).toBe(UserRole.student); + }); - course_user.section = 2; - expect(course_user.section).toBe('2'); + test('Update DBCourseUser using the set method', () => { + const db_course_user = new DBCourseUser(); + expect(db_course_user.isValid()).toBe(true); - course_user.section = '12'; - expect(course_user.section).toBe('12'); + db_course_user.set({ course_user_id: 10 }); + expect(db_course_user.course_user_id).toBe(10); - }); + db_course_user.set({ user_id: 20 }); + expect(db_course_user.user_id).toBe(20); - test('set fields of DBCourseUser using set()', () => { - const course_user = new DBCourseUser(); - course_user.set({ role: 'student' }); - expect(course_user.role).toBe('STUDENT'); + db_course_user.set({ course_id: 5 }); + expect(db_course_user.course_id).toBe(5); - course_user.set({ course_id: 34 }); - expect(course_user.course_id).toBe(34); + db_course_user.set({ role: UserRole.admin }); + expect(db_course_user.role).toBe(UserRole.admin); - course_user.set({ user_id: 3 }); - expect(course_user.user_id).toBe(3); + db_course_user.set({ role: 'student' }); + expect(db_course_user.role).toBe(UserRole.student); + }); + }); - course_user.set({ user_id: '5' }); - expect(course_user.user_id).toBe(5); + describe('Checking for valid and invalid DBCourseUsers', () => { + test('Checking for invalid course_user_id', () => { + const db_course_user = new DBCourseUser(); + expect(db_course_user.isValid()).toBe(true); - course_user.set({ section: 2 }); - expect(course_user.section).toBe('2'); + db_course_user.course_user_id = -3; + expect(db_course_user.isValid()).toBe(false); - course_user.set({ section: '12' }); - expect(course_user.section).toBe('12'); + db_course_user.course_user_id = 3.14; + expect(db_course_user.isValid()).toBe(false); - }); + db_course_user.course_user_id = 7; + expect(db_course_user.isValid()).toBe(true); - test('set invalid role', () => { - const course_user = new DBCourseUser({ role: 'student' }); - expect(() => { course_user.role = 'superhero';}).toThrow(UserRoleException); }); - test('set invalid user_id', () => { - const course_user = new DBCourseUser(); - expect(() => { course_user.user_id = -1; }).toThrow(NonNegIntException); - expect(() => { course_user.user_id = '-1'; }).toThrow(NonNegIntException); - }); + test('Checking for invalid user_id', () => { + const db_course_user = new DBCourseUser(); + db_course_user.user_id = -5; + expect(db_course_user.isValid()).toBe(false); + + db_course_user.user_id = 1.34; + expect(db_course_user.isValid()).toBe(false); - test('set invalid course_id', () => { - const course_user = new DBCourseUser(); - expect(() => { course_user.course_id = -1;}).toThrow(NonNegIntException); - expect(() => { course_user.course_id = '-1'; }).toThrow(NonNegIntException); + db_course_user.user_id = 5; + expect(db_course_user.isValid()).toBe(true); }); - test('set invalid course_user_id', () => { - const course_user = new DBCourseUser(); - expect(() => { course_user.course_user_id = -1; }).toThrow(NonNegIntException); - expect(() => { course_user.course_user_id = '-1'; }).toThrow(NonNegIntException); + test('Checking for invalid course_id', () => { + const db_course_user = new DBCourseUser(); + db_course_user.course_id = -9; + expect(db_course_user.isValid()).toBe(false); + + db_course_user.course_id = 1.39; + expect(db_course_user.isValid()).toBe(false); + + db_course_user.course_id = 9; + expect(db_course_user.isValid()).toBe(true); }); }); @@ -128,15 +131,19 @@ describe('Testing Database and client-side Course Users', () => { course_user_id: 0, user_id: 0, course_id: 0, - is_admin: false + is_admin: false, + username: '', + email: '', + first_name: '', + last_name: '', + role: 'UNKNOWN', + student_id: '' }; test('Create a Valid CourseUser', () => { - const course_user1 = new CourseUser(); - - expect(course_user1).toBeInstanceOf(CourseUser); - expect(course_user1.toObject()).toStrictEqual(default_course_user); - + const course_user = new CourseUser(); + expect(course_user).toBeInstanceOf(CourseUser); + expect(course_user.toObject()).toStrictEqual(default_course_user); }); test('Check that calling all_fields() and params() is correct', () => { @@ -153,101 +160,139 @@ describe('Testing Database and client-side Course Users', () => { test('Check that cloning a merged user works', () => { const course_user = new CourseUser(); expect(course_user.clone().toObject()).toStrictEqual(default_course_user); - expect(course_user.clone() instanceof CourseUser).toBe(true); - }); + expect(course_user.clone()).toBeInstanceOf(CourseUser); - test('Invalid user_id', () => { - expect(() => { - new CourseUser({ username: 'test', user_id: -1 }); - }).toThrow(NonNegIntException); - expect(() => { - new CourseUser({ username: 'test', user_id: '-1' }); - }).toThrow(NonNegIntException); - expect(() => { - new CourseUser({ username: 'test', user_id: 'one' }); - }).toThrow(NonNegIntException); + // The default user is not valid (The username is the empty string.) + expect(course_user.isValid()).toBe(false); }); - test('Invalid username', () => { - expect(() => { - new CourseUser({ username: '@test' }); - }).toThrow(UsernameParseException); - expect(() => { - new CourseUser({ username: '123test' }); - }).toThrow(UsernameParseException); - expect(() => { - new CourseUser({ username: 'user name' }); - }).toThrow(UsernameParseException); - }); + describe('Setting fields of a CourseUser', () => { + test('Set CourseUser fields directly', () => { + const course_user = new CourseUser(); - test('Invalid email', () => { - expect(() => { - new CourseUser({ username: 'test', email: 'bad email' }); - }).toThrow(EmailParseException); - expect(() => { - new CourseUser({ username: 'test', email: 'user@info@site.com' }); - }).toThrow(EmailParseException); - }); + course_user.username = 'test2'; + expect(course_user.username).toBe('test2'); - test('set invalid role', () => { - const course_user = new CourseUser({ username: 'test', role: 'student' }); - expect(() => { - course_user.set({ role: 'superhero' }); - }).toThrow(UserRoleException); - }); + course_user.email = 'test@site.com'; + expect(course_user.email).toBe('test@site.com'); - test('set fields of CourseUser', () => { - const course_user = new CourseUser({ username: 'test' }); - course_user.set({ role: 'student' }); - expect(course_user.role).toBe('STUDENT'); + course_user.user_id = 15; + expect(course_user.user_id).toBe(15); - course_user.set({ course_id: 34 }); - expect(course_user.course_id).toBe(34); + course_user.first_name = 'Homer'; + expect(course_user.first_name).toBe('Homer'); - course_user.set({ user_id: 3 }); - expect(course_user.user_id).toBe(3); + course_user.last_name = 'Simpson'; + expect(course_user.last_name).toBe('Simpson'); - course_user.set({ user_id: '5' }); - expect(course_user.user_id).toBe(5); + course_user.is_admin = true; + expect(course_user.is_admin).toBe(true); - course_user.set({ section: 2 }); - expect(course_user.section).toBe('2'); + course_user.student_id = '1234'; + expect(course_user.student_id).toBe('1234'); - course_user.set({ section: '12' }); - expect(course_user.section).toBe('12'); + course_user.course_user_id = 10; + expect(course_user.course_user_id).toBe(10); - }); + course_user.course_id = 5; + expect(course_user.course_id).toBe(5); - test('set invalid user_id', () => { - const course_user = new CourseUser({ username: 'test' }); - expect(() => { - course_user.set({ user_id: -1 }); - }).toThrow(NonNegIntException); + course_user.role = UserRole.admin; + expect(course_user.role).toBe(UserRole.admin); - expect(() => { - course_user.set({ user_id: '-1' }); - }).toThrow(NonNegIntException); + course_user.role = 'student'; + expect(course_user.role).toBe(UserRole.student); + }); - }); + test('Update CourseUser using the set method', () => { + const course_user = new CourseUser({ username: 'homer' }); + expect(course_user.isValid()).toBe(true); + + course_user.set({ course_user_id: 10 }); + expect(course_user.course_user_id).toBe(10); + + course_user.set({ user_id: 20 }); + expect(course_user.user_id).toBe(20); + + course_user.set({ course_id: 5 }); + expect(course_user.course_id).toBe(5); + + course_user.set({ role: UserRole.admin }); + expect(course_user.role).toBe(UserRole.admin); + + course_user.set({ role: 'student' }); + expect(course_user.role).toBe(UserRole.student); + + course_user.set({ username: 'test2' }); + expect(course_user.username).toBe('test2'); + + course_user.set({ email: 'test@site.com' }); + expect(course_user.email).toBe('test@site.com'); + + course_user.set({ first_name: 'Homer' }); + expect(course_user.first_name).toBe('Homer'); - test('set invalid course_id', () => { - const course_user = new CourseUser({ username: 'test' }); - expect(() => { - course_user.set({ course_id: -1 }); - }).toThrow(NonNegIntException); - expect(() => { - course_user.set({ course_id: '-1' }); - }).toThrow(NonNegIntException); + course_user.set({ last_name: 'Simpson' }); + expect(course_user.last_name).toBe('Simpson'); + + course_user.set({ is_admin: true }); + expect(course_user.is_admin).toBe(true); + + course_user.set({ student_id: '1234' }); + expect(course_user.student_id).toBe('1234'); + }); }); - test('set invalid course_user_id', () => { - const course_user = new CourseUser({ username: 'test' }); - expect(() => { - course_user.set({ course_user_id: -1 }); - }).toThrow(NonNegIntException); - expect(() => { - course_user.set({ course_user_id: '-1' }); - }).toThrow(NonNegIntException); + describe('Testing for valid and invalid users.', () => { + test('setting invalid user_id', () => { + const user = new CourseUser({ username: 'homer' }); + expect(user.isValid()).toBe(true); + + user.user_id = -15; + expect(user.isValid()).toBe(false); + + user.user_id = 1.23; + expect(user.isValid()).toBe(false); + }); + + test('setting invalid course_user_id', () => { + const course_user = new CourseUser({ username: 'homer' }); + expect(course_user.isValid()).toBe(true); + + course_user.course_user_id = -3; + expect(course_user.isValid()).toBe(false); + + course_user.course_user_id = 3.14; + expect(course_user.isValid()).toBe(false); + + course_user.course_user_id = 7; + expect(course_user.isValid()).toBe(true); + }); + + test('setting invalid course_id', () => { + const course_user = new CourseUser({ username: 'homer' }); + course_user.course_id = -9; + expect(course_user.isValid()).toBe(false); + + course_user.course_id = 1.39; + expect(course_user.isValid()).toBe(false); + + course_user.course_id = 9; + expect(course_user.isValid()).toBe(true); + }); + + test('setting invalid email', () => { + const user = new CourseUser({ username: 'test' }); + expect(user.isValid()).toBe(true); + + user.email = 'bad@email@address.com'; + expect(user.isValid()).toBe(false); + }); + + test('setting invalid username', () => { + const user = new CourseUser({ username: 'my username' }); + expect(user.isValid()).toBe(false); + }); }); }); }); diff --git a/tests/unit-tests/courses.spec.ts b/tests/unit-tests/courses.spec.ts index fa2b1aef..52672f71 100644 --- a/tests/unit-tests/courses.spec.ts +++ b/tests/unit-tests/courses.spec.ts @@ -14,11 +14,8 @@ describe('Test Course Models', () => { test('Create a Valid Course', () => { const course = new Course({ course_name: 'Arithmetic' }); expect(course).toBeInstanceOf(Course); - expect(course.toObject()).toStrictEqual(default_course); - expect(course.isValid()).toBe(true); - }); test('Check that calling all_fields() and params() is correct', () => { @@ -27,9 +24,7 @@ describe('Test Course Models', () => { expect(course.all_field_names.sort()).toStrictEqual(course_fields.sort()); expect(course.param_fields.sort()).toStrictEqual(['course_dates']); - expect(Course.ALL_FIELDS.sort()).toStrictEqual(course_fields.sort()); - }); test('Check that cloning works', () => { @@ -73,7 +68,7 @@ describe('Test Course Models', () => { test('checking for valid course dates', () => { const course = new Course({ course_name: 'Arithemetic', - course_dates: {start: 100, end: 100} + course_dates: { start: 100, end: 100 } }); expect(course.course_dates.isValid()).toBe(true); expect(course.isValid()).toBe(true); @@ -104,7 +99,7 @@ describe('Test Course Models', () => { test('Create a course with invalid dates', () => { const c1 = new Course({ course_name: 'Arithmetic', - course_dates: { start: 100, end: 0} + course_dates: { start: 100, end: 0 } }); expect(c1.isValid()).toBe(false); }); @@ -191,19 +186,19 @@ describe('Test Course Models', () => { c1.course_name = ''; expect(c1.isValid()).toBe(false); - c1.set({course_name: 'Arithmetic', user_id: -1}); + c1.set({ course_name: 'Arithmetic', user_id: -1 }); expect(c1.isValid()).toBe(false); - c1.set({user_id: 10, course_id: -1}); + c1.set({ user_id: 10, course_id: -1 }); expect(c1.isValid()).toBe(false); c1.course_id = 10; expect(c1.isValid()).toBe(true); - c1.role ='wizard'; + c1.role = 'wizard'; expect(c1.isValid()).toBe(false); - c1.role ='ta'; + c1.role = 'ta'; expect(c1.isValid()).toBe(true); c1.username = ''; @@ -221,11 +216,11 @@ describe('Test Course Models', () => { const c1 = new UserCourse({ course_name: 'Arithmetic', username: 'homer', - course_dates: { start: 100, end: 200} + course_dates: { start: 100, end: 200 } }); expect(c1.isValid()).toBe(true); - c1.setDates({start: 100, end: 0}); + c1.setDates({ start: 100, end: 0 }); expect(c1.isValid()).toBe(false); }); diff --git a/tests/unit-tests/problem_sets.spec.ts b/tests/unit-tests/problem_sets.spec.ts index ba338ebb..9af64a67 100644 --- a/tests/unit-tests/problem_sets.spec.ts +++ b/tests/unit-tests/problem_sets.spec.ts @@ -1,17 +1,9 @@ import { ProblemSet } from 'src/common/models/problem_sets'; describe('Test generic ProblemSets', () => { - const default_problem_set = { - set_id: 0, - set_name: 'set #1', - course_id: 0, - set_type: 'UNKNOWN', - set_params: {}, - set_dates: {} - } describe('Creation of a ProblemSet', () => { - test('Create a valid ProblemSet', () => { + test('Test the class of a ProblemSet', () => { const set = new ProblemSet(); expect(set).toBeInstanceOf(ProblemSet); }); @@ -26,7 +18,6 @@ describe('Test generic ProblemSets', () => { }); }); - describe('Check setting generic fields', () => { test('Check that all fields can be set directly', () => { const set = new ProblemSet(); @@ -54,19 +45,18 @@ describe('Test generic ProblemSets', () => { }); - test('Check that all fields can be set using the set() method', () => { const set = new ProblemSet(); - set.set({set_id: 5}); + set.set({ set_id: 5 }); expect(set.set_id).toBe(5); - set.set({course_id: 10}); + set.set({ course_id: 10 }); expect(set.course_id).toBe(10); - set.set({set_visible: true}); + set.set({ set_visible: true }); expect(set.set_visible).toBe(true); - set.set({set_name: 'Set #1'}); + set.set({ set_name: 'Set #1' }); expect(set.set_name).toBe('Set #1'); }); }); diff --git a/tests/unit-tests/users.spec.ts b/tests/unit-tests/users.spec.ts index e8877831..31cdb58c 100644 --- a/tests/unit-tests/users.spec.ts +++ b/tests/unit-tests/users.spec.ts @@ -1,152 +1,119 @@ // tests parsing and handling of users -import { BooleanParseException, EmailParseException, NonNegIntException, - UsernameParseException } from 'src/common/models/parsers'; -import { RequiredFieldsException } from 'src/common/models'; import { User } from 'src/common/models/users'; -const default_user = { - user_id: 0, - username: 'test', - is_admin: false, -}; +describe('Testing User and CourseUsers', () => { + const default_user = { + user_id: 0, + username: '', + is_admin: false, + email: '', + first_name: '', + last_name: '', + student_id: '' + }; -test('Create a default User', () => { - const user = new User({ username: 'test' }); - expect(user instanceof User).toBe(true); - expect(user.toObject()).toStrictEqual(default_user); + describe('Create a new User', () => { + test('Create a default User', () => { + const user = new User(); + expect(user instanceof User).toBe(true); + expect(user.toObject()).toStrictEqual(default_user); + }); -}); + test('Check that calling all_fields() and params() is correct', () => { + const user_fields = ['user_id', 'username', 'is_admin', 'email', 'first_name', + 'last_name', 'student_id']; + const user = new User(); -test('Missing Username', () => { - // missing username - expect(() => { new User();}).toThrow(RequiredFieldsException); -}); + expect(user.all_field_names.sort()).toStrictEqual(user_fields.sort()); + expect(user.param_fields.sort()).toStrictEqual([]); + expect(User.ALL_FIELDS.sort()).toStrictEqual(user_fields.sort()); + }); -test('Check that calling all_fields() and params() is correct', () => { - const user_fields = ['user_id', 'username', 'is_admin', 'email', 'first_name', - 'last_name', 'student_id']; - const user = new User({ username: 'test' }); + test('Check that cloning a User works', () => { + const user = new User(); + expect(user.clone().toObject()).toStrictEqual(default_user); + expect(user.clone()).toBeInstanceOf(User); - expect(user.all_field_names.sort()).toStrictEqual(user_fields.sort()); - expect(user.param_fields.sort()).toStrictEqual([]); + // The default user is not valid. The username cannot be the empty string. + expect(user.isValid()).toBe(false); + }); - expect(User.ALL_FIELDS.sort()).toStrictEqual(user_fields.sort()); + }); -}); + describe('Setting fields of a User', () => { + test('Set User field directly', () => { + const user = new User(); -test('Check that cloning a User works', () => { - const user = new User({ username: 'test' }); - expect(user.clone().toObject()).toStrictEqual(default_user); - expect(user.clone() instanceof User).toBe(true); -}); + user.username = 'test2'; + expect(user.username).toBe('test2'); -test('Invalid user_id', () => { - expect(() => { - new User({ username: 'test', user_id: -1 }); - }).toThrow(NonNegIntException); - expect(() => { - new User({ username: 'test', user_id: '-1' }); - }).toThrow(NonNegIntException); - expect(() => { - new User({ username: 'test', user_id: 'one' }); - }).toThrow(NonNegIntException); - expect(() => { - new User({ username: 'test', user_id: 'false' }); - }).toThrow(NonNegIntException); -}); + user.email = 'test@site.com'; + expect(user.email).toBe('test@site.com'); -test('Invalid username', () => { - expect(() => { - new User({ username: '@test' }); - }).toThrow(UsernameParseException); - expect(() => { - new User({ username: '123test' }); - }).toThrow(UsernameParseException); - expect(() => { - new User({ username: 'user name' }); - }).toThrow(UsernameParseException); -}); + user.user_id = 15; + expect(user.user_id).toBe(15); -test('Invalid email', () => { - expect(() => { - new User({ username: 'test', email: 'bad email' }); - }).toThrow(EmailParseException); - expect(() => { - new User({ username: 'test', email: 'user@info@site.com' }); - }).toThrow(EmailParseException); -}); + user.first_name = 'Homer'; + expect(user.first_name).toBe('Homer'); -test('setting user fields', () => { - const user = new User({ username: 'test' }); + user.last_name = 'Simpson'; + expect(user.last_name).toBe('Simpson'); - user.username = 'test2'; - expect(user.username).toBe('test2'); + user.is_admin = true; + expect(user.is_admin).toBe(true); - user.email = 'test@site.com'; - expect(user.email).toBe('test@site.com'); + user.student_id = '1234'; + expect(user.student_id).toBe('1234'); - user.user_id = 15; - expect(user.user_id).toBe(15); + }); - user.first_name = 'Homer'; - expect(user.first_name).toBe('Homer'); + test('set fields using set() method', () => { + const user = new User({ username: 'test' }); - user.last_name = 'Simpson'; - expect(user.last_name).toBe('Simpson'); + user.set({ username: 'test2' }); + expect(user.username).toBe('test2'); + user.set({ email: 'test@site.com' }); + expect(user.email).toBe('test@site.com'); - user.is_admin = true; - expect(user.is_admin).toBe(true); + user.set({ user_id: 15 }); + expect(user.user_id).toBe(15); - user.is_admin = 1; - expect(user.is_admin).toBe(true); + user.set({ first_name: 'Homer' }); + expect(user.first_name).toBe('Homer'); - user.is_admin = '0'; - expect(user.is_admin).toBe(false); + user.set({ last_name: 'Simpson' }); + expect(user.last_name).toBe('Simpson'); -}); + user.set({ is_admin: true }); + expect(user.is_admin).toBe(true); -test('set fields using set() method', () => { - const user = new User({ username: 'test' }); + user.set({ student_id: '1234' }); + expect(user.student_id).toBe('1234'); + }); + }); - user.set({ username: 'test2' }); - expect(user.username).toBe('test2'); - user.set({ email: 'test@site.com' }); - expect(user.email).toBe('test@site.com'); + describe('Testing for valid and invalid users.', () => { - user.set({ user_id: 15 }); - expect(user.user_id).toBe(15); + test('setting invalid email', () => { + const user = new User({ username: 'test' }); + expect(user.isValid()).toBe(true); - user.set({ first_name: 'Homer' }); - expect(user.first_name).toBe('Homer'); + user.email = 'bad@email@address.com'; + expect(user.isValid()).toBe(false); + }); - user.set({ last_name: 'Simpson' }); - expect(user.last_name).toBe('Simpson'); + test('setting invalid user_id', () => { + const user = new User({ username: 'test' }); + expect(user.isValid()).toBe(true); - user.set({ is_admin: true }); - expect(user.is_admin).toBe(true); - - user.set({ is_admin: 1 }); - expect(user.is_admin).toBe(true); - - user.set({ is_admin: '0' }); - expect(user.is_admin).toBe(false); -}); - -test('setting invalid email', () => { - const user = new User({ username: 'test' }); - expect(() => { user.email = 'bad@email@address.com'; }) - .toThrow(EmailParseException); -}); - -test('setting invalid user_id', () => { - const user = new User({ username: 'test' }); - expect(() => { user.user_id = -15; }) - .toThrow(NonNegIntException); -}); + user.user_id = -15; + expect(user.isValid()).toBe(false); + }); -test('setting invalid admin', () => { - const user = new User({ username: 'test' }); - expect(() => { user.is_admin = 'FALSE'; }) - .toThrow(BooleanParseException); + test('setting invalid username', () => { + const user = new User({ username: 'my username' }); + expect(user.isValid()).toBe(false); + }); + }); }); From fc7dd5dabc9d921c4081deaef7dd6b498b28a113 Mon Sep 17 00:00:00 2001 From: Peter Staab Date: Fri, 20 May 2022 10:43:55 -0400 Subject: [PATCH 04/34] WIP: work on getting the UI simpler with newer models. --- src/common/models/users.ts | 7 +- src/components/common/UserCourses.vue | 6 +- .../AddUsersFromFile.vue | 338 ++++++++++-------- .../AddUsersManually.vue | 78 ++-- src/stores/users.ts | 20 ++ tests/stores/users.spec.ts | 2 +- tests/unit-tests/course_users.spec.ts | 5 +- tests/unit-tests/homework_sets.spec.ts | 5 +- tests/unit-tests/quizzes.spec.ts | 5 +- tests/unit-tests/review_sets.spec.ts | 5 +- 10 files changed, 259 insertions(+), 212 deletions(-) diff --git a/src/common/models/users.ts b/src/common/models/users.ts index f29c66d6..fea8183b 100644 --- a/src/common/models/users.ts +++ b/src/common/models/users.ts @@ -1,5 +1,8 @@ -import { isNonNegInt, isValidUsername, isValidEmail, parseNonNegInt, parseUsername, - parseBoolean, parseEmail, parseUserRole, UserRole } from 'src/common/models/parsers'; +/* This file contains the definitions of a User, DBCourseUser and Course User + in terms of a model. */ + +import { isNonNegInt, isValidUsername, isValidEmail, parseUserRole, UserRole } + from 'src/common/models/parsers'; import { Model } from 'src/common/models'; export interface ParseableUser { diff --git a/src/components/common/UserCourses.vue b/src/components/common/UserCourses.vue index 86e55313..d33e4759 100644 --- a/src/components/common/UserCourses.vue +++ b/src/components/common/UserCourses.vue @@ -66,12 +66,14 @@ export default defineComponent({ name: 'UserCourses', setup() { const session = useSessionStore(); + // If this is the first page on load, then user_course is undefined. the ?? '' prevents + // an error. return { student_courses: computed(() => - session.user_courses.filter(user_course => parseUserRole(user_course.role) === 'STUDENT') + session.user_courses.filter(user_course => parseUserRole(user_course.role ?? '') === 'STUDENT') ), instructor_courses: computed(() => - session.user_courses.filter(user_course => parseUserRole(user_course.role) === 'INSTRUCTOR') + session.user_courses.filter(user_course => parseUserRole(user_course.role ?? '') === 'INSTRUCTOR') ), user: computed(() => session.user) }; diff --git a/src/components/instructor/ClasslistManagerComponents/AddUsersFromFile.vue b/src/components/instructor/ClasslistManagerComponents/AddUsersFromFile.vue index 547c52ce..8023edf7 100644 --- a/src/components/instructor/ClasslistManagerComponents/AddUsersFromFile.vue +++ b/src/components/instructor/ClasslistManagerComponents/AddUsersFromFile.vue @@ -1,100 +1,106 @@ diff --git a/src/components/instructor/ClasslistManagerComponents/AddUsersManually.vue b/src/components/instructor/ClasslistManagerComponents/AddUsersManually.vue index 1d82b032..7773ebac 100644 --- a/src/components/instructor/ClasslistManagerComponents/AddUsersManually.vue +++ b/src/components/instructor/ClasslistManagerComponents/AddUsersManually.vue @@ -10,7 +10,7 @@
- +
- +
- +
- +
- +
- +
diff --git a/src/stores/users.ts b/src/stores/users.ts index a47c4e0e..8ca2a253 100644 --- a/src/stores/users.ts +++ b/src/stores/users.ts @@ -110,6 +110,11 @@ export const useUserStore = defineStore('user', { * @returns {User} -- the updated user. */ async updateUser(user: User): Promise { + if (!user.isValid()) { + logger.error('The updated user is invalid'); + logger.error(JSON.stringify(user.toObject())); + throw 'The updated user is invalid.'; + } const response = await api.put(`users/${user.user_id}`, user.toObject()); if (response.status === 200) { const updated_user = new User(response.data as ParseableUser); @@ -142,6 +147,11 @@ export const useUserStore = defineStore('user', { * @returns {User} the added user */ async addUser(user: User): Promise { + if (!user.isValid()) { + logger.error('The updated user is invalid'); + logger.error(JSON.stringify(user.toObject())); + throw 'The added user is invalid.'; + } const response = await api.post('users', user.toObject()); if (response.status === 200) { const new_user = new User(response.data as ParseableUser); @@ -173,6 +183,11 @@ export const useUserStore = defineStore('user', { * @returns the added course user. */ async addCourseUser(course_user: CourseUser): Promise { + if (!course_user.isValid()) { + logger.error('The added course user is invalid'); + logger.error(JSON.stringify(course_user.toObject())); + throw 'The added course user is invalid'; + } // When sending, only send the DBCourseUser fields. const response = await api.post(`courses/${course_user.course_id}/users`, course_user.toObject(DBCourseUser.ALL_FIELDS)); @@ -192,6 +207,11 @@ export const useUserStore = defineStore('user', { * @returns the updated course user. */ async updateCourseUser(course_user: CourseUser): Promise { + if (!course_user.isValid()) { + logger.error('The updated course user is invalid'); + logger.error(JSON.stringify(course_user.toObject())); + throw 'The updated course user is invalid'; + } const url = `courses/${course_user.course_id || 0}/users/${course_user.user_id ?? 0}`; // When sending, only send the DBCourseUser fields. const response = await api.put(url, course_user.toObject(DBCourseUser.ALL_FIELDS)); diff --git a/tests/stores/users.spec.ts b/tests/stores/users.spec.ts index 6d078b20..dfc24f6c 100644 --- a/tests/stores/users.spec.ts +++ b/tests/stores/users.spec.ts @@ -122,7 +122,7 @@ describe('User store tests', () => { const course_user_to_update = user_store.course_users .find(user => user.user_id === user_not_in_precalc.user_id)?.clone(); if (course_user_to_update) { - course_user_to_update.recitation = 3; + course_user_to_update.recitation = '3'; const updated_user = await user_store.updateCourseUser(course_user_to_update); expect(updated_user).toStrictEqual(course_user_to_update); } diff --git a/tests/unit-tests/course_users.spec.ts b/tests/unit-tests/course_users.spec.ts index 9c89358a..45ef0bc2 100644 --- a/tests/unit-tests/course_users.spec.ts +++ b/tests/unit-tests/course_users.spec.ts @@ -6,9 +6,8 @@ // tests parsing and handling of merged users -import { EmailParseException, NonNegIntException, UsernameParseException, - UserRoleException, UserRole } from 'src/common/models/parsers'; -import { CourseUser, DBCourseUser, ParseableDBCourseUser } from 'src/common/models/users'; +import { UserRole } from 'src/common/models/parsers'; +import { CourseUser, DBCourseUser } from 'src/common/models/users'; describe('Testing Database and client-side Course Users', () => { describe('Creating a DBCourseUser', () => { diff --git a/tests/unit-tests/homework_sets.spec.ts b/tests/unit-tests/homework_sets.spec.ts index 17790754..f7f167ec 100644 --- a/tests/unit-tests/homework_sets.spec.ts +++ b/tests/unit-tests/homework_sets.spec.ts @@ -123,15 +123,12 @@ describe('Tests for Homework Sets', () => { test('Test valid Homework Set params', () => { const set2 = new HomeworkSet(); - set2.set({ set_visible: 1 }); + set2.set({ set_visible: true }); expect(set2.set_visible).toBeTruthy(); set2.set({ set_visible: false }); expect(set2.set_visible).toBeFalsy(); - set2.set({ set_visible: 'true' }); - expect(set2.set_visible).toBeTruthy(); - set2.set({ set_name: 'HW #9' }); expect(set2.set_name).toBe('HW #9'); }); diff --git a/tests/unit-tests/quizzes.spec.ts b/tests/unit-tests/quizzes.spec.ts index 5bb12bd3..a18bf959 100644 --- a/tests/unit-tests/quizzes.spec.ts +++ b/tests/unit-tests/quizzes.spec.ts @@ -126,15 +126,12 @@ describe('Testing for Quizzes', () => { test('Test valid Quiz params', () => { const quiz2 = new Quiz(); - quiz2.set({ set_visible: 1 }); + quiz2.set({ set_visible: true }); expect(quiz2.set_visible).toBeTruthy(); quiz2.set({ set_visible: false }); expect(quiz2.set_visible).toBeFalsy(); - quiz2.set({ set_visible: 'true' }); - expect(quiz2.set_visible).toBeTruthy(); - quiz2.set({ set_name: 'HW #9' }); expect(quiz2.set_name).toBe('HW #9'); }); diff --git a/tests/unit-tests/review_sets.spec.ts b/tests/unit-tests/review_sets.spec.ts index ac38defe..e6bb5d64 100644 --- a/tests/unit-tests/review_sets.spec.ts +++ b/tests/unit-tests/review_sets.spec.ts @@ -110,15 +110,12 @@ describe('Testing for Review Sets', () => { test('Test valid review_set params', () => { const review_set2 = new ReviewSet(); - review_set2.set({ set_visible: 1 }); + review_set2.set({ set_visible: true }); expect(review_set2.set_visible).toBeTruthy(); review_set2.set({ set_visible: false }); expect(review_set2.set_visible).toBeFalsy(); - review_set2.set({ set_visible: 'true' }); - expect(review_set2.set_visible).toBeTruthy(); - review_set2.set({ set_name: 'HW #9' }); expect(review_set2.set_name).toBe('HW #9'); }); From bc83691c04d954ee1c37868f744fced0c832241e Mon Sep 17 00:00:00 2001 From: Peter Staab Date: Fri, 20 May 2022 17:48:05 -0400 Subject: [PATCH 05/34] WIP: fixed loading of users by file. --- src/common/api-requests/user.ts | 17 +- src/common/models/users.ts | 15 +- src/components/admin/AddCourse.vue | 2 +- .../AddUsersFromFile.vue | 147 ++++++------ .../AddUsersManually.vue | 28 ++- tests/unit-tests/course_users.spec.ts | 213 ++++++++++-------- 6 files changed, 241 insertions(+), 181 deletions(-) diff --git a/src/common/api-requests/user.ts b/src/common/api-requests/user.ts index 1b5e7cc9..f0b8036c 100644 --- a/src/common/api-requests/user.ts +++ b/src/common/api-requests/user.ts @@ -1,23 +1,18 @@ import { api } from 'boot/axios'; -import { ParseableCourseUser, ParseableUser } from 'src/common/models/users'; +import { ParseableUser, User } from 'src/common/models/users'; import { ResponseError } from 'src/common/api-requests/interfaces'; -export async function checkIfUserExists(course_id: number, username: string) { - const response = await api.get(`courses/${course_id}/users/${username}/exists`); - if (response.status === 250) { - throw response.data as ResponseError; - } - return response.data as ParseableCourseUser; -} /** - * queries the database to determine the user. + * queries the database to determine if the user exists. * @param {string} username -- the username of the user. + * @return {User} the user if they exist. + * @throws {ResponseError} if the user doesen't exist, this is thrown. */ -export async function getUser(username: string): Promise { +export async function getUser(username: string): Promise { const response = await api.get(`users/${username}`); if (response.status === 200) { - return response.data as ParseableUser; + return new User(response.data as ParseableUser); } else { throw response.data as ResponseError; } diff --git a/src/common/models/users.ts b/src/common/models/users.ts index fea8183b..e531e8a3 100644 --- a/src/common/models/users.ts +++ b/src/common/models/users.ts @@ -3,7 +3,7 @@ import { isNonNegInt, isValidUsername, isValidEmail, parseUserRole, UserRole } from 'src/common/models/parsers'; -import { Model } from 'src/common/models'; +import { Dictionary, Model } from 'src/common/models'; export interface ParseableUser { user_id?: number; @@ -265,6 +265,17 @@ export class CourseUser extends Model { isValid(): boolean { return isNonNegInt(this.user_id) && isNonNegInt(this.course_user_id) && isNonNegInt(this.course_id) && isValidUsername(this.username) && - (this.email === '' || isValidEmail(this.email)); + this.role !== UserRole.unknown && (this.email === '' || isValidEmail(this.email)); + } + + validate(): Dictionary { + return { + course_id: isNonNegInt(this.course_id) || 'The course_id must be a non negative integer.', + course_user_id: isNonNegInt(this.course_user_id) || 'The course_user_id must be a non negative integer.', + user_id: isNonNegInt(this.user_id) || 'The user_id must be a non negative integer.', + username: isValidUsername(this.username) || 'The username must be valid.', + email: (this.email === '' || isValidEmail(this.email)) || 'The email must be valid', + role: this.role !== UserRole.unknown || 'The role is not valid.', + }; } } diff --git a/src/components/admin/AddCourse.vue b/src/components/admin/AddCourse.vue index 431334d1..0df47150 100644 --- a/src/components/admin/AddCourse.vue +++ b/src/components/admin/AddCourse.vue @@ -201,7 +201,7 @@ export default defineComponent({ await getUser(username.value) .then((_user) => { logger.debug(`[AddCourse/checkUser] Found user: ${username.value}`); - user.value = new User(_user); + user.value.set(_user.toObject()); instructor_exists.value = true; }) .catch((e) => { diff --git a/src/components/instructor/ClasslistManagerComponents/AddUsersFromFile.vue b/src/components/instructor/ClasslistManagerComponents/AddUsersFromFile.vue index 8023edf7..1ca176c6 100644 --- a/src/components/instructor/ClasslistManagerComponents/AddUsersFromFile.vue +++ b/src/components/instructor/ClasslistManagerComponents/AddUsersFromFile.vue @@ -15,7 +15,7 @@
-
+
First row is Header @@ -50,13 +50,13 @@
-
+
@@ -140,9 +140,9 @@ const settings = useSettingsStore(); const $q = useQuasar(); const file = ref(new File([], '')); // Stores all users from the file as well as parsing errors. -const merged_users = ref>([]); +const course_users = ref>([]); // Stores the selected users. -const selected = ref>([]); +const selected_users = ref>([]); const column_headers = ref>({}); // Provides a map from column number to field name. It doesn't need to be reactive. const user_param_map: Dictionary = {}; @@ -152,7 +152,7 @@ const loading = ref(false); // used to indicate parsing is occurring const first_row_header = ref(false); const header_row = ref({}); -const merged_users_to_add = ref>([]); +const course_users_to_add = ref>([]); const selected_user_error = ref(false); const users_already_in_course = ref(false); @@ -186,28 +186,28 @@ const fillHeaders = () => { }); }; -// This converts converts each selected row to a ParseableMerged User +// This converts converts each selected row to a ParseableCourseUser // based on the column headers. -const getMergedUser = (row: UserFromFile) => { +const getCourseUser = (row: UserFromFile) => { // The following pulls the keys out from the user_param_map and the the values out of row // to get the merged user. - const merged_user = Object.entries(user_param_map).reduce( + const course_user = Object.entries(user_param_map).reduce( (acc, [k, v]) => ({ ...acc, [v]: row[k as keyof UserFromFile] }), {} ) as ParseableCourseUser; - // Set the role if a common role for all users is selected. - merged_user.role = use_single_role.value ? common_role.value ?? 'UNKOWN' : 'UNKNOWN'; - return merged_user; + // Set the role if a common role for all users is selected_users. + course_user.role = use_single_role.value ? common_role.value ?? 'UNKOWN' : 'UNKNOWN'; + return course_user; }; // Parse the selected users from the file. const parseUsers = () => { // Clear Errors and reset reactive variables. loading.value = true; - merged_users_to_add.value = []; + course_users_to_add.value = []; selected_user_error.value = false; users_already_in_course.value = false; - merged_users.value + course_users.value .filter((u: UserFromFile) => u._error?.type !== 'none') .forEach((u) => { // reset the error for each selected row @@ -220,59 +220,76 @@ const parseUsers = () => { // This is needed for parsing errors. const inverse_param_map = invert(user_param_map) as Dictionary; - selected.value.forEach((params: UserFromFile) => { + selected_users.value.forEach((params: UserFromFile) => { let parse_error: ParseError | null = null; const row = parseInt(`${params?._row || -1}`); - try { - const merged_user = getMergedUser(params); - // If the user is already in the course, show a warning - const u = users.course_users.find((_u) => _u.username === merged_user.username); - if (u) { - users_already_in_course.value = true; - parse_error = { - type: 'warn', - message: - `The user with username '${merged_user.username ?? ''}'` + - ' is already enrolled in the course.', - entire_row: true, - }; - } else { - merged_users_to_add.value.push(new CourseUser(merged_user)); - } - } catch (error) { - const err = error as ParseError; - selected_user_error.value = true; - + const course_user_params = getCourseUser(params); + // If the user is already in the course, show a warning + const u = users.course_users.find((_u) => _u.username === course_user_params.username); + if (u) { + users_already_in_course.value = true; parse_error = { - type: 'error', - message: err.message, + type: 'warn', + message: + `The user with username '${course_user_params.username ?? ''}'` + + ' is already enrolled in the course.', + entire_row: true, }; + } else { + const course_user = new CourseUser(course_user_params); + console.log(course_user.toObject()); + console.log(course_user.validate()); + if (course_user.isValid()) { + course_users_to_add.value.push(course_user); + } else { + const validate = course_user.validate(); + // Find the field which didn't validate. + try { + Object.entries(validate).forEach(([k, v]) => { + if (typeof v === 'string') { + throw { + message: v, + field: k, + }; + } + }); + } catch (error) { + const err = error as ParseError; + console.log(err); + selected_user_error.value = true; + + parse_error = { + type: 'error', + message: err.message, + }; - if (err.field === '_all') { - Object.assign(parse_error, { entire_row: true }); - } else if ( - err.field && - (User.ALL_FIELDS.indexOf(err.field) >= 0 || - CourseUser.ALL_FIELDS.indexOf(err.field) >= 0) - ) { - if (inverse_param_map[err.field]) { - parse_error.col = inverse_param_map[err.field]; - } else { - parse_error.entire_row = true; + if (err.field === '_all') { + Object.assign(parse_error, { entire_row: true }); + } else if ( + err.field && + (User.ALL_FIELDS.indexOf(err.field) >= 0 || + CourseUser.ALL_FIELDS.indexOf(err.field) >= 0) + ) { + if (inverse_param_map[err.field]) { + parse_error.col = inverse_param_map[err.field]; + } else { + parse_error.entire_row = true; + } + } else if (err.field != undefined) { + // missing field + parse_error.entire_row = true; + } } - } else if (err.field != undefined) { - // missing field - parse_error.entire_row = true; } } if (parse_error) { - const row_index = merged_users.value.findIndex((u: UserFromFile) => u._row === row); + const row_index = course_users.value.findIndex((u: UserFromFile) => u._row === row); if (row_index >= 0) { // Copy the user, update and splice in. This is needed to make the load file table reactive. - const user = { ...merged_users.value[row_index] }; + const user = { ...course_users.value[row_index] }; user._error = parse_error; - merged_users.value.splice(row_index, 1, user); + course_users.value.splice(row_index, 1, user); } } }); @@ -314,7 +331,7 @@ const loadFile = () => { }); users.push(d); }); - merged_users.value = users; + course_users.value = users; } } }; @@ -322,12 +339,12 @@ const loadFile = () => { // Add the Merged Users to the course. const addMergedUsers = async () => { - for await (const user of merged_users_to_add.value) { + for await (const user of course_users_to_add.value) { user.course_id = session.course.course_id; let global_user: User | undefined; try { // Skip if username is undefined? - global_user = (await getUser(user.username ?? '')) as User; + global_user = await getUser(user.username); } catch (err) { const error = err as ResponseError; // this will occur is the user is not a global user @@ -370,7 +387,7 @@ const addMergedUsers = async () => { } }; -watch([selected, common_role], parseUsers, { deep: true }); +watch([selected_users, common_role], parseUsers, { deep: true }); watch( () => column_headers, @@ -391,22 +408,22 @@ watch( ); watch([first_row_header], () => { - selected.value = []; + selected_users.value = []; if (first_row_header.value) { - const first_row = merged_users.value.shift(); + const first_row = course_users.value.shift(); if (first_row) { header_row.value = first_row; fillHeaders(); } } else { - merged_users.value.unshift(header_row.value); + course_users.value.unshift(header_row.value); } }); const columns = computed(() => { - return merged_users.value.length === 0 + return course_users.value.length === 0 ? [] - : Object.keys(merged_users.value[0]) + : Object.keys(course_users.value[0]) .filter((v: string) => v !== '_row' && v !== '_error') .map((v) => ({ name: v, label: v, field: v })); }); diff --git a/src/components/instructor/ClasslistManagerComponents/AddUsersManually.vue b/src/components/instructor/ClasslistManagerComponents/AddUsersManually.vue index 7773ebac..abf89a5a 100644 --- a/src/components/instructor/ClasslistManagerComponents/AddUsersManually.vue +++ b/src/components/instructor/ClasslistManagerComponents/AddUsersManually.vue @@ -73,12 +73,12 @@ import { ref, computed, defineEmits } from 'vue'; import { useQuasar } from 'quasar'; import { logger } from 'boot/logger'; -import { checkIfUserExists } from 'src/common/api-requests/user'; +import { getUser } from 'src/common/api-requests/user'; import { useUserStore } from 'src/stores/users'; import { useSessionStore } from 'src/stores/session'; import { useSettingsStore } from 'src/stores/settings'; -import { CourseUser, User } from 'src/common/models/users'; +import { CourseUser, ParseableCourseUser, User } from 'src/common/models/users'; import type { ResponseError } from 'src/common/api-requests/interfaces'; import { AxiosError } from 'axios'; import { isValidEmail, isValidUsername, parseNonNegInt } from 'src/common/models/parsers'; @@ -103,16 +103,20 @@ const settings = useSettingsStore(); const checkUser = async () => { // If the user doesn't exist, the catch statement will handle this. try { - const course_id = session.course.course_id; - const user_params = await checkIfUserExists(course_id, course_user.value.username); - const user = new User(user_params); - - user_exists.value = true; - course_user.value.user_id = user.user_id; - course_user.value.username = user.username; - course_user.value.first_name = user.first_name; - course_user.value.last_name = user.last_name; - course_user.value.email = user.email; + const existing_user = await getUser(course_user.value.username); + console.log(existing_user); + course_user.value.set(existing_user.toObject() as ParseableCourseUser); + + // const course_id = session.course.course_id; + // const user_params = await checkIfUserExists(course_id, course_user.value.username); + // const user = new User(user_params); + + // user_exists.value = true; + // course_user.value.user_id = user.user_id; + // course_user.value.username = user.username; + // course_user.value.first_name = user.first_name; + // course_user.value.last_name = user.last_name; + // course_user.value.email = user.email; } catch (err) { const error = err as ResponseError; // this will occur is the user is not a global user diff --git a/tests/unit-tests/course_users.spec.ts b/tests/unit-tests/course_users.spec.ts index 45ef0bc2..f40d69d4 100644 --- a/tests/unit-tests/course_users.spec.ts +++ b/tests/unit-tests/course_users.spec.ts @@ -164,134 +164,167 @@ describe('Testing Database and client-side Course Users', () => { // The default user is not valid (The username is the empty string.) expect(course_user.isValid()).toBe(false); }); + }); + + describe('Setting fields of a CourseUser', () => { + test('Set CourseUser fields directly', () => { + const course_user = new CourseUser(); - describe('Setting fields of a CourseUser', () => { - test('Set CourseUser fields directly', () => { - const course_user = new CourseUser(); + course_user.username = 'test2'; + expect(course_user.username).toBe('test2'); - course_user.username = 'test2'; - expect(course_user.username).toBe('test2'); + course_user.email = 'test@site.com'; + expect(course_user.email).toBe('test@site.com'); - course_user.email = 'test@site.com'; - expect(course_user.email).toBe('test@site.com'); + course_user.user_id = 15; + expect(course_user.user_id).toBe(15); - course_user.user_id = 15; - expect(course_user.user_id).toBe(15); + course_user.first_name = 'Homer'; + expect(course_user.first_name).toBe('Homer'); - course_user.first_name = 'Homer'; - expect(course_user.first_name).toBe('Homer'); + course_user.last_name = 'Simpson'; + expect(course_user.last_name).toBe('Simpson'); - course_user.last_name = 'Simpson'; - expect(course_user.last_name).toBe('Simpson'); + course_user.is_admin = true; + expect(course_user.is_admin).toBe(true); - course_user.is_admin = true; - expect(course_user.is_admin).toBe(true); + course_user.student_id = '1234'; + expect(course_user.student_id).toBe('1234'); - course_user.student_id = '1234'; - expect(course_user.student_id).toBe('1234'); + course_user.course_user_id = 10; + expect(course_user.course_user_id).toBe(10); - course_user.course_user_id = 10; - expect(course_user.course_user_id).toBe(10); + course_user.course_id = 5; + expect(course_user.course_id).toBe(5); + + course_user.role = UserRole.admin; + expect(course_user.role).toBe(UserRole.admin); + + course_user.role = 'student'; + expect(course_user.role).toBe(UserRole.student); + }); - course_user.course_id = 5; - expect(course_user.course_id).toBe(5); + test('Update CourseUser using the set method', () => { + const course_user = new CourseUser({ username: 'homer' }); + expect(course_user.isValid()).toBe(true); - course_user.role = UserRole.admin; - expect(course_user.role).toBe(UserRole.admin); + course_user.set({ course_user_id: 10 }); + expect(course_user.course_user_id).toBe(10); - course_user.role = 'student'; - expect(course_user.role).toBe(UserRole.student); - }); + course_user.set({ user_id: 20 }); + expect(course_user.user_id).toBe(20); - test('Update CourseUser using the set method', () => { - const course_user = new CourseUser({ username: 'homer' }); - expect(course_user.isValid()).toBe(true); + course_user.set({ course_id: 5 }); + expect(course_user.course_id).toBe(5); - course_user.set({ course_user_id: 10 }); - expect(course_user.course_user_id).toBe(10); + course_user.set({ role: UserRole.admin }); + expect(course_user.role).toBe(UserRole.admin); - course_user.set({ user_id: 20 }); - expect(course_user.user_id).toBe(20); + course_user.set({ role: 'student' }); + expect(course_user.role).toBe(UserRole.student); - course_user.set({ course_id: 5 }); - expect(course_user.course_id).toBe(5); + course_user.set({ username: 'test2' }); + expect(course_user.username).toBe('test2'); - course_user.set({ role: UserRole.admin }); - expect(course_user.role).toBe(UserRole.admin); + course_user.set({ email: 'test@site.com' }); + expect(course_user.email).toBe('test@site.com'); - course_user.set({ role: 'student' }); - expect(course_user.role).toBe(UserRole.student); + course_user.set({ first_name: 'Homer' }); + expect(course_user.first_name).toBe('Homer'); - course_user.set({ username: 'test2' }); - expect(course_user.username).toBe('test2'); + course_user.set({ last_name: 'Simpson' }); + expect(course_user.last_name).toBe('Simpson'); - course_user.set({ email: 'test@site.com' }); - expect(course_user.email).toBe('test@site.com'); + course_user.set({ is_admin: true }); + expect(course_user.is_admin).toBe(true); - course_user.set({ first_name: 'Homer' }); - expect(course_user.first_name).toBe('Homer'); + course_user.set({ student_id: '1234' }); + expect(course_user.student_id).toBe('1234'); + }); + }); - course_user.set({ last_name: 'Simpson' }); - expect(course_user.last_name).toBe('Simpson'); + describe('Testing for valid and invalid users.', () => { + test('setting invalid user_id', () => { + const user = new CourseUser({ username: 'homer' }); + expect(user.isValid()).toBe(true); - course_user.set({ is_admin: true }); - expect(course_user.is_admin).toBe(true); + user.user_id = -15; + expect(user.isValid()).toBe(false); - course_user.set({ student_id: '1234' }); - expect(course_user.student_id).toBe('1234'); - }); + user.user_id = 1.23; + expect(user.isValid()).toBe(false); }); - describe('Testing for valid and invalid users.', () => { - test('setting invalid user_id', () => { - const user = new CourseUser({ username: 'homer' }); - expect(user.isValid()).toBe(true); + test('setting invalid course_user_id', () => { + const course_user = new CourseUser({ username: 'homer' }); + expect(course_user.isValid()).toBe(true); + + course_user.course_user_id = -3; + expect(course_user.isValid()).toBe(false); + + course_user.course_user_id = 3.14; + expect(course_user.isValid()).toBe(false); - user.user_id = -15; - expect(user.isValid()).toBe(false); + course_user.course_user_id = 7; + expect(course_user.isValid()).toBe(true); + }); - user.user_id = 1.23; - expect(user.isValid()).toBe(false); - }); + test('setting invalid course_id', () => { + const course_user = new CourseUser({ username: 'homer' }); + course_user.course_id = -9; + expect(course_user.isValid()).toBe(false); - test('setting invalid course_user_id', () => { - const course_user = new CourseUser({ username: 'homer' }); - expect(course_user.isValid()).toBe(true); + course_user.course_id = 1.39; + expect(course_user.isValid()).toBe(false); - course_user.course_user_id = -3; - expect(course_user.isValid()).toBe(false); + course_user.course_id = 9; + expect(course_user.isValid()).toBe(true); + }); - course_user.course_user_id = 3.14; - expect(course_user.isValid()).toBe(false); + test('setting invalid email', () => { + const user = new CourseUser({ username: 'test' }); + expect(user.isValid()).toBe(true); - course_user.course_user_id = 7; - expect(course_user.isValid()).toBe(true); - }); + user.email = 'bad@email@address.com'; + expect(user.isValid()).toBe(false); + }); - test('setting invalid course_id', () => { - const course_user = new CourseUser({ username: 'homer' }); - course_user.course_id = -9; - expect(course_user.isValid()).toBe(false); + test('setting invalid username', () => { + const user = new CourseUser({ username: 'my username' }); + expect(user.isValid()).toBe(false); + }); + }); - course_user.course_id = 1.39; - expect(course_user.isValid()).toBe(false); + describe('Test validation of CourseUsers', () => { + test('Test the validation of the user role', () => { + const user = new CourseUser({ username: 'homer' }); + const validate = user.validate(); + expect(validate.role).toBe('The role is not valid.'); + }); - course_user.course_id = 9; - expect(course_user.isValid()).toBe(true); - }); + test('Test the validation of the course_id', () => { + const user = new CourseUser({ username: 'homer', course_id: -1 }); + const validate = user.validate(); + expect(validate.course_id).toBe('The course_id must be a non negative integer.'); + }); - test('setting invalid email', () => { - const user = new CourseUser({ username: 'test' }); - expect(user.isValid()).toBe(true); + test('Test the validation of the user_id', () => { + const user = new CourseUser({ username: 'homer', user_id: 23.5 }); + const validate = user.validate(); + expect(validate.user_id).toBe('The user_id must be a non negative integer.'); + }); - user.email = 'bad@email@address.com'; - expect(user.isValid()).toBe(false); - }); + test('Test the validation of the course_user_id', () => { + const user = new CourseUser({ username: 'homer', course_user_id: -10 }); + const validate = user.validate(); + expect(validate.course_user_id).toBe('The course_user_id must be a non negative integer.'); + }); - test('setting invalid username', () => { - const user = new CourseUser({ username: 'my username' }); - expect(user.isValid()).toBe(false); - }); + test('Test the validation of the user role', () => { + const user = new CourseUser({ username: 'homer', role: 'programmer' }); + const validate = user.validate(); + expect(validate.role).toBe('The role is not valid.'); }); + }); }); From bd8538bdbb0367f4d4299169005b48286793c155 Mon Sep 17 00:00:00 2001 From: Peter Staab Date: Sat, 21 May 2022 06:35:23 -0400 Subject: [PATCH 06/34] WIP: cleanup --- src/common/models/problems.ts | 28 ++- .../AddUsersFromFile.vue | 207 +++++++++--------- .../AddUsersManually.vue | 12 - tests/unit-tests/set_problems.spec.ts | 4 +- 4 files changed, 126 insertions(+), 125 deletions(-) diff --git a/src/common/models/problems.ts b/src/common/models/problems.ts index cc2a815e..d05577ef 100644 --- a/src/common/models/problems.ts +++ b/src/common/models/problems.ts @@ -1,4 +1,6 @@ -import { MergeError, parseNonNegDecimal, parseNonNegInt, parseUsername } from './parsers'; +// Definition of Problems (SetProblems, LibraryProblems and UserProblems) + +import { isNonNegInt, MergeError, parseNonNegDecimal, parseNonNegInt, parseUsername } from './parsers'; import { Model, Dictionary, generic } from '.'; import { RenderParams, ParseableRenderParams } from './renderer'; import { UserSet } from './user_sets'; @@ -60,9 +62,9 @@ export class Problem extends Model { * ParseableLocationParams stores information about a library problem. */ export interface ParseableLocationParams extends Partial> { - library_id?: string | number; + library_id?: number; file_path?: string; - problem_pool_id?: string | number; + problem_pool_id?: number; } export interface ParseableLibraryProblem { @@ -91,8 +93,8 @@ class ProblemLocationParams extends Model { } public get library_id() : number | undefined { return this._library_id; } - public set library_id(val: string | number | undefined) { - if (val != undefined) this._library_id = parseNonNegInt(val); + public set library_id(val: number | undefined) { + if (val != undefined) this._library_id = val; } public get file_path() : string | undefined { return this._file_path;} @@ -100,8 +102,20 @@ class ProblemLocationParams extends Model { if (value != undefined) this._file_path = value;} public get problem_pool_id() : number | undefined { return this._problem_pool_id; } - public set problem_pool_id(val: string | number | undefined) { - if (val != undefined) this._problem_pool_id = parseNonNegInt(val); + public set problem_pool_id(val: number | undefined) { + if (val != undefined) this._problem_pool_id = val; + } + + clone(): ProblemLocationParams { + return new ProblemLocationParams(this.toObject() as ParseableLocationParams); + } + + // Ensure that the _id fields are non-negative integers and that at least one + // of the three fields are defined. + isValid() { + if (this.library_id != undefined && !isNonNegInt(this.library_id)) return false; + if (this.problem_pool_id != undefined && !isNonNegInt(this.problem_pool_id)) return false; + return this.problem_pool_id != undefined && this.library_id != undefined && this.file_path != undefined; } } diff --git a/src/components/instructor/ClasslistManagerComponents/AddUsersFromFile.vue b/src/components/instructor/ClasslistManagerComponents/AddUsersFromFile.vue index 1ca176c6..f8acfc33 100644 --- a/src/components/instructor/ClasslistManagerComponents/AddUsersFromFile.vue +++ b/src/components/instructor/ClasslistManagerComponents/AddUsersFromFile.vue @@ -1,106 +1,106 @@ diff --git a/t/mojolicious/004_course_users.t b/t/mojolicious/004_course_users.t index 46134b73..f4d2a772 100644 --- a/t/mojolicious/004_course_users.t +++ b/t/mojolicious/004_course_users.t @@ -110,8 +110,8 @@ $t->delete_ok("/webwork3/api/courses/2/users/$new_user_id")->status_is(200) ->content_type_is('application/json;charset=UTF-8')->json_is('/user_id' => $new_user_id); # Delete the added global user -$t->delete_ok("/webwork3/api/users/$new_user_id")->status_is(200) - ->content_type_is('application/json;charset=UTF-8')->json_is('/user_id' => $new_user_id); +$t->delete_ok("/webwork3/api/users/$new_user_id")->status_is(200)->content_type_is('application/json;charset=UTF-8') + ->json_is('/user_id' => $new_user_id); # Logout of the admin user account. $t->post_ok('/webwork3/api/logout')->status_is(200)->json_is('/logged_in' => 0); From e32866574a0ff3569bb83ab7a20851f3faef8275 Mon Sep 17 00:00:00 2001 From: Peter Staab Date: Fri, 17 Jun 2022 15:56:39 -0400 Subject: [PATCH 20/34] FIX: testing errors --- src/common/api-requests/user.ts | 2 +- t/mojolicious/004_course_users.t | 3 +-- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/src/common/api-requests/user.ts b/src/common/api-requests/user.ts index cc331cd9..63ab37b2 100644 --- a/src/common/api-requests/user.ts +++ b/src/common/api-requests/user.ts @@ -1,7 +1,7 @@ import { api } from 'boot/axios'; import { ParseableUser, User } from 'src/common/models/users'; -import { ResponseError } from 'src/common/api-requests/interfaces'; +import { ResponseError } from 'src/common/api-requests/errors'; /** * Gets the global user in the database given by username. This returns a user or throws a diff --git a/t/mojolicious/004_course_users.t b/t/mojolicious/004_course_users.t index 68348fac..76ec44f9 100644 --- a/t/mojolicious/004_course_users.t +++ b/t/mojolicious/004_course_users.t @@ -110,8 +110,7 @@ $t->delete_ok("/webwork3/api/courses/2/users/$new_user_id")->status_is(200) ->content_type_is('application/json;charset=UTF-8')->json_is('/user_id' => $new_user_id); # Delete the added users. -$t->delete_ok("/webwork3/api/users/$new_user_id")->status_is(200) - ->json_is('/username' => $new_user->{username}); +$t->delete_ok("/webwork3/api/users/$new_user_id")->status_is(200)->json_is('/username' => $new_user->{username}); # Logout of the admin user account. $t->post_ok('/webwork3/api/logout')->status_is(200)->json_is('/logged_in' => 0); From f6a09773b355588a23dd59602345ad7323bfdde9 Mon Sep 17 00:00:00 2001 From: Peter Staab Date: Fri, 17 Jun 2022 16:28:31 -0400 Subject: [PATCH 21/34] FIX: cleanup and using true/false in tests. FIX: cleanup and using true/false in tests. --- t/db/003_users.t | 9 +++++---- t/mojolicious/002_courses.t | 8 ++++---- t/mojolicious/003_users.t | 16 +++++++--------- t/mojolicious/004_course_users.t | 6 +++--- t/mojolicious/005_problem_sets.t | 13 +++++++------ t/mojolicious/006_quizzes.t | 28 +++++++++++++--------------- 6 files changed, 39 insertions(+), 41 deletions(-) diff --git a/t/db/003_users.t b/t/db/003_users.t index 924e34bd..54b43b9d 100644 --- a/t/db/003_users.t +++ b/t/db/003_users.t @@ -17,6 +17,7 @@ use Test::More; use Test::Exception; use Clone qw/clone/; use List::MoreUtils qw/firstval/; +use Mojo::JSON qw/true false/; use DB::Schema; use DB::TestUtils qw/loadCSV removeIDs/; @@ -43,7 +44,7 @@ for my $student (@students) { for my $key (qw/course_name recitation section params role/) { delete $student->{$key}; } - $student->{is_admin} = Mojo::JSON::false; + $student->{is_admin} = false; } # Add the admin user @@ -52,7 +53,7 @@ push( { username => 'admin', email => 'admin@google.com', - is_admin => Mojo::JSON::true, + is_admin => true, first_name => 'Andrea', last_name => 'Administrator', student_id => undef @@ -104,7 +105,7 @@ $user = { first_name => 'Clancy', email => 'wiggam@springfieldpd.gov', student_id => '', - is_admin => Mojo::JSON::false, + is_admin => false, }; my $new_user = $users_rs->addGlobalUser(params => $user); @@ -152,7 +153,7 @@ my $user2 = { first_name => 'Selma', email => 'selma@google.com', student_id => '', - is_admin => Mojo::JSON::false, + is_admin => false, }; my $added_user2 = $users_rs->addGlobalUser(params => $user2); diff --git a/t/mojolicious/002_courses.t b/t/mojolicious/002_courses.t index 1e9c7930..e3196c78 100644 --- a/t/mojolicious/002_courses.t +++ b/t/mojolicious/002_courses.t @@ -4,7 +4,7 @@ use Mojo::Base -strict; use Test::More; use Test::Mojo; -use Mojo::JSON; +use Mojo::JSON qw/true false/; BEGIN { use File::Basename qw/dirname/; @@ -51,11 +51,11 @@ my $new_course_id = $t->tx->res->json('/course_id'); $new_course->{course_id} = $new_course_id; # The default for visible is true: -$new_course->{visible} = Mojo::JSON::true; +$new_course->{visible} = true; is_deeply($new_course, $t->tx->res->json, "addCourse: courses match"); # Update the course -$new_course->{visible} = Mojo::JSON::true; +$new_course->{visible} = true; $t->put_ok("/webwork3/api/courses/$new_course_id" => json => $new_course)->status_is(200) ->json_is('/course_name' => $new_course->{course_name}); @@ -120,7 +120,7 @@ $t->get_ok('/webwork3/api/courses/1')->json_is('/course_name', 'Precalculus'); my $precalc = $t->tx->res->json; ok($precalc->{visible}, 'Testing that visible field is truthy.'); -is($precalc->{visible}, Mojo::JSON::true, 'Testing that the visible field compares to JSON::true'); +is($precalc->{visible}, true, 'Testing that the visible field compares to JSON::true'); ok(JSON::PP::is_bool($precalc->{visible}), 'Testing that the visible field is a JSON boolean'); ok(JSON::PP::is_bool($precalc->{visible}) && $precalc->{visible}, 'testing that the visible field is a JSON::true'); diff --git a/t/mojolicious/003_users.t b/t/mojolicious/003_users.t index b728ef9f..fad7b2a9 100644 --- a/t/mojolicious/003_users.t +++ b/t/mojolicious/003_users.t @@ -4,7 +4,7 @@ use Mojo::Base -strict; use Test::More; use Test::Mojo; -use Mojo::JSON; +use Mojo::JSON qw/true false/; BEGIN { use File::Basename qw/dirname/; @@ -158,18 +158,16 @@ $t->get_ok('/webwork3/api/users/1')->json_is('/username', 'admin'); my $admin_user = $t->tx->res->json; ok($admin_user->{is_admin}, 'testing that is_admin compares to 1.'); -is($admin_user->{is_admin}, Mojo::JSON::true, 'testing that is_admin compares to Mojo::JSON::true'); -ok(JSON::PP::is_bool($admin_user->{is_admin}), 'testing that is_admin is a Mojo::JSON::true or Mojo::JSON::false'); -ok(JSON::PP::is_bool($admin_user->{is_admin}) && $admin_user->{is_admin}, - 'testing that is_admin is a Mojo::JSON::true'); +is($admin_user->{is_admin}, true, 'testing that is_admin compares to true'); +ok(JSON::PP::is_bool($admin_user->{is_admin}), 'testing that is_admin is a true or false'); +ok(JSON::PP::is_bool($admin_user->{is_admin}) && $admin_user->{is_admin}, 'testing that is_admin is a true'); ok(not(JSON::PP::is_bool($admin_user->{user_id})), 'testing that $admin->{user_id} is not a JSON boolean'); ok(!$new_user_from_db->{is_admin}, 'testing new_user->{is_admin} is not truthy.'); -is($new_user_from_db->{is_admin}, Mojo::JSON::false, 'testing that new_user->{is_admin} compares to Mojo::JSON::false'); -ok(JSON::PP::is_bool($new_user_from_db->{is_admin}), - 'testing that new_user->{is_admin} is a Mojo::JSON::true or Mojo::JSON::false'); +is($new_user_from_db->{is_admin}, false, 'testing that new_user->{is_admin} compares to false'); +ok(JSON::PP::is_bool($new_user_from_db->{is_admin}), 'testing that new_user->{is_admin} is a true or false'); ok(JSON::PP::is_bool($new_user_from_db->{is_admin}) && !$new_user_from_db->{is_admin}, - 'testing that new_user->{is_admin} is a Mojo::JSON::false'); + 'testing that new_user->{is_admin} is a false'); done_testing; diff --git a/t/mojolicious/004_course_users.t b/t/mojolicious/004_course_users.t index 76ec44f9..d5821aed 100644 --- a/t/mojolicious/004_course_users.t +++ b/t/mojolicious/004_course_users.t @@ -4,7 +4,7 @@ use Mojo::Base -strict; use Test::More; use Test::Mojo; -use Mojo::JSON; +use Mojo::JSON qw/true false/; BEGIN { use File::Basename qw/dirname/; @@ -62,7 +62,7 @@ my $course_user_params = { role => 'student', course_user_params => { comment => "I love my big sister", - useMathQuill => Mojo::JSON::true + useMathQuill => true } }; @@ -130,7 +130,7 @@ $t->get_ok('/webwork3/api/courses/1/users')->status_is(200)->content_type_is('ap ok($added_user->{course_user_params}->{useMathQuill}, 'Testing that useMathQuill param is truthy.'); is($added_user->{course_user_params}->{useMathQuill}, - Mojo::JSON::true, 'Testing that the useMathQuill param compares to JSON::true'); + true, 'Testing that the useMathQuill param compares to JSON::true'); ok( JSON::PP::is_bool($added_user->{course_user_params}->{useMathQuill}), 'Testing that the useMathQuill param is a JSON boolean' diff --git a/t/mojolicious/005_problem_sets.t b/t/mojolicious/005_problem_sets.t index 4bc8a6ef..99336ba6 100644 --- a/t/mojolicious/005_problem_sets.t +++ b/t/mojolicious/005_problem_sets.t @@ -4,6 +4,7 @@ use Mojo::Base -strict; use Test::More; use Test::Mojo; +use Mojo::JSON qw/true false/; use DateTime::Format::Strptime; @@ -74,7 +75,7 @@ my $new_set_id = $t->tx->res->json('/set_id'); # Check that set_visible is a JSON boolean my $set_visible = $t->tx->res->json('/set_visible'); ok(!$set_visible, 'testing that set_visible is falsy'); -is($set_visible, Mojo::JSON::false, 'Test that set_visible compares to Mojo::JSON::false'); +is($set_visible, false, 'Test that set_visible compares to Mojo::JSON::false'); ok(JSON::PP::is_bool($set_visible), 'Test that set_visible is a JSON boolean'); ok(JSON::PP::is_bool($set_visible) && !$set_visible, 'Test that set_visible is a JSON::false'); @@ -82,13 +83,13 @@ ok(JSON::PP::is_bool($set_visible) && !$set_visible, 'Test that set_visible is a $t->put_ok( "/webwork3/api/courses/2/sets/$new_set_id" => json => { set_name => 'HW #11', - set_visible => Mojo::JSON::true + set_visible => true } )->content_type_is('application/json;charset=UTF-8')->json_is('/set_name' => 'HW #11'); $set_visible = $t->tx->res->json('/set_visible'); ok($set_visible, 'testing that set_visible is truthy'); -is($set_visible, Mojo::JSON::true, 'Test that set_visible compares to Mojo::JSON::true'); +is($set_visible, true, 'Test that set_visible compares to Mojo::JSON::true'); ok(JSON::PP::is_bool($set_visible), 'Test that set_visible is a JSON boolean'); ok(JSON::PP::is_bool($set_visible) && $set_visible, 'Test that set_visible is a JSON:: true'); @@ -114,21 +115,21 @@ $t->get_ok('/webwork3/api/courses/1/sets/1'); my $hw1 = $t->tx->res->json; my $enabled = $hw1->{set_dates}->{enable_reduced_scoring}; ok($enabled, 'testing that enabled_reduced_scoring compares to 1.'); -is($enabled, Mojo::JSON::true, 'testing that enabled_reduced_scoring compares to Mojo::JSON::true'); +is($enabled, true, 'testing that enabled_reduced_scoring compares to Mojo::JSON::true'); ok(JSON::PP::is_bool($enabled), 'testing that enabled_reduced_scoring is a Mojo::JSON::true or Mojo::JSON::false'); ok(JSON::PP::is_bool($enabled) && $enabled, 'testing that enabled_reduced_scoring is a Mojo::JSON::true'); # Check that updating a boolean parameter is working: $t->put_ok( "/webwork3/api/courses/2/sets/$new_set_id" => json => { - set_params => { hide_hint => Mojo::JSON::false } + set_params => { hide_hint => false } } )->content_type_is('application/json;charset=UTF-8'); my $hw2 = $t->tx->res->json; my $hide_hint = $hw2->{set_params}->{hide_hint}; ok(!$hide_hint, 'testing that hide_hint is falsy.'); -is($hide_hint, Mojo::JSON::false, 'testing that hide_hint compares to Mojo::JSON::false'); +is($hide_hint, false, 'testing that hide_hint compares to Mojo::JSON::false'); ok(JSON::PP::is_bool($hide_hint), 'testing that hide_hint is a Mojo::JSON::true or Mojo::JSON::false'); ok(JSON::PP::is_bool($hide_hint) && !$hide_hint, 'testing that hide_hint is a Mojo::JSON::false'); diff --git a/t/mojolicious/006_quizzes.t b/t/mojolicious/006_quizzes.t index 964941c0..660d3990 100644 --- a/t/mojolicious/006_quizzes.t +++ b/t/mojolicious/006_quizzes.t @@ -4,6 +4,7 @@ use Mojo::Base -strict; use Test::More; use Test::Mojo; +use Mojo::JSON qw/true false/; use DateTime::Format::Strptime; @@ -66,9 +67,9 @@ $t->get_ok("/webwork3/api/courses/2/sets/$quiz1->{set_id}")->content_type_is('ap $quiz1 = $t->tx->res->json; my $timed = $quiz1->{set_params}->{timed}; ok($timed, 'testing that timed compares to 1.'); -is($timed, Mojo::JSON::true, 'testing that timed compares to Mojo::JSON::true'); -ok(JSON::PP::is_bool($timed), 'testing that timed is a Mojo::JSON::true or Mojo::JSON::false'); -ok(JSON::PP::is_bool($timed) && $timed, 'testing that timed is a Mojo::JSON::true'); +is($timed, true, 'testing that timed compares to true'); +ok(JSON::PP::is_bool($timed), 'testing that timed is a true or false'); +ok(JSON::PP::is_bool($timed) && $timed, 'testing that timed is a true'); # Make a new quiz @@ -76,9 +77,9 @@ my $new_quiz_params = { set_name => 'Quiz #20', set_type => 'QUIZ', set_params => { - timed => Mojo::JSON::true, + timed => true, quiz_duration => 30, - problem_randorder => Mojo::JSON::true + problem_randorder => true }, set_dates => { open => 100, @@ -95,16 +96,16 @@ my $returned_quiz = $t->tx->res->json; my $new_quiz = $t->tx->res->json; my $problem_randorder = $new_quiz->{set_params}->{problem_randorder}; ok($problem_randorder, 'testing that problem_randorder compares to 1.'); -is($problem_randorder, Mojo::JSON::true, 'testing that problem_randorder compares to Mojo::JSON::true'); -ok(JSON::PP::is_bool($problem_randorder), 'testing that problem_randorder is a Mojo::JSON::true or Mojo::JSON::false'); -ok(JSON::PP::is_bool($problem_randorder) && $problem_randorder, 'testing that problem_randorder is a Mojo::JSON::true'); +is($problem_randorder, true, 'testing that problem_randorder compares to true'); +ok(JSON::PP::is_bool($problem_randorder), 'testing that problem_randorder is a true or false'); +ok(JSON::PP::is_bool($problem_randorder) && $problem_randorder, 'testing that problem_randorder is a true'); # Check that updating a boolean parameter is working: $t->put_ok( "/webwork3/api/courses/2/sets/$returned_quiz->{set_id}" => json => { set_params => { - problem_randorder => Mojo::JSON::false + problem_randorder => false } } )->content_type_is('application/json;charset=UTF-8'); @@ -113,12 +114,9 @@ my $updated_quiz = $t->tx->res->json; $problem_randorder = $updated_quiz->{set_params}->{problem_randorder}; ok(!$problem_randorder, 'testing that hide_hint is falsy.'); -is($problem_randorder, Mojo::JSON::false, 'testing that problem_randorder compares to Mojo::JSON::false'); -ok(JSON::PP::is_bool($problem_randorder), 'testing that problem_randorder is a Mojo::JSON::true or Mojo::JSON::false'); -ok( - JSON::PP::is_bool($problem_randorder) && !$problem_randorder, - 'testing that problem_randorder is a Mojo::JSON::false' -); +is($problem_randorder, false, 'testing that problem_randorder compares to false'); +ok(JSON::PP::is_bool($problem_randorder), 'testing that problem_randorder is a true or false'); +ok(JSON::PP::is_bool($problem_randorder) && !$problem_randorder, 'testing that problem_randorder is a false'); # delete the added quiz $t->delete_ok("/webwork3/api/courses/2/sets/$returned_quiz->{set_id}") From c02696c550442c867920e17636eeccecc4c3fff5 Mon Sep 17 00:00:00 2001 From: Peter Staab Date: Wed, 29 Jun 2022 12:28:16 -0400 Subject: [PATCH 22/34] WIP: check that enable_reduced_scoring is a boolean. --- lib/DB/Schema/Result/ProblemSet.pm | 4 +-- lib/DB/Schema/Result/ProblemSet/HWSet.pm | 11 ++++----- lib/DB/Schema/Result/ProblemSet/Quiz.pm | 2 +- lib/DB/Schema/Result/ProblemSet/ReviewSet.pm | 2 +- lib/DB/Schema/Result/UserSet.pm | 4 +-- lib/DB/WithDates.pm | 21 ++++++++++++++-- t/db/005_hwsets.t | 26 +++++++++++++++----- 7 files changed, 50 insertions(+), 20 deletions(-) diff --git a/lib/DB/Schema/Result/ProblemSet.pm b/lib/DB/Schema/Result/ProblemSet.pm index 5cb598bc..7a8b651f 100644 --- a/lib/DB/Schema/Result/ProblemSet.pm +++ b/lib/DB/Schema/Result/ProblemSet.pm @@ -108,8 +108,8 @@ __PACKAGE__->add_columns( size => 256, is_nullable => 0, default_value => '{}', - serializer_class => 'Boolean::JSON', - serializer_options => { boolean_fields => ['enable_reduced_scoring'] } + serializer_class => 'JSON', + serializer_options => { utf8 => 1 } }, # Store params as a JSON object. set_params => { diff --git a/lib/DB/Schema/Result/ProblemSet/HWSet.pm b/lib/DB/Schema/Result/ProblemSet/HWSet.pm index 690cdff3..43b08645 100644 --- a/lib/DB/Schema/Result/ProblemSet/HWSet.pm +++ b/lib/DB/Schema/Result/ProblemSet/HWSet.pm @@ -28,7 +28,7 @@ sub valid_dates ($=) { } sub optional_fields_in_dates ($=) { - return ['enable_reduced_scoring']; + return { enable_reduced_scoring => 'bool' }; } =head2 C @@ -83,11 +83,10 @@ This is a description of the homework set. sub valid_params ($=) { return { - enable_reduced_scoring => 'bool', - hide_hint => 'bool', - hardcopy_header => q{.*}, - set_header => q{.*}, - description => q{.*} + hide_hint => 'bool', + hardcopy_header => q{.*}, + set_header => q{.*}, + description => q{.*} }; } diff --git a/lib/DB/Schema/Result/ProblemSet/Quiz.pm b/lib/DB/Schema/Result/ProblemSet/Quiz.pm index ac784f23..dbf14196 100644 --- a/lib/DB/Schema/Result/ProblemSet/Quiz.pm +++ b/lib/DB/Schema/Result/ProblemSet/Quiz.pm @@ -27,7 +27,7 @@ sub valid_dates ($=) { return [ 'open', 'due', 'answer' ]; } -sub optional_fields_in_dates ($=) { return []; } +sub optional_fields_in_dates ($=) { return {}; } =head2 C diff --git a/lib/DB/Schema/Result/ProblemSet/ReviewSet.pm b/lib/DB/Schema/Result/ProblemSet/ReviewSet.pm index 40b37bbf..b3f81cf4 100644 --- a/lib/DB/Schema/Result/ProblemSet/ReviewSet.pm +++ b/lib/DB/Schema/Result/ProblemSet/ReviewSet.pm @@ -26,7 +26,7 @@ sub valid_dates ($=) { return [ 'open', 'closed' ]; } -sub optional_fields_in_dates ($=) { return []; } +sub optional_fields_in_dates ($=) { return {}; } =head2 C diff --git a/lib/DB/Schema/Result/UserSet.pm b/lib/DB/Schema/Result/UserSet.pm index b27e5f52..efb82b24 100644 --- a/lib/DB/Schema/Result/UserSet.pm +++ b/lib/DB/Schema/Result/UserSet.pm @@ -122,8 +122,8 @@ __PACKAGE__->add_columns( size => 256, is_nullable => 0, default_value => '{}', - serializer_class => 'Boolean::JSON', - serializer_options => { boolean_fields => ['enable_reduced_scoring'] } + serializer_class => 'JSON', + serializer_options => { utf8 => 1 } }, # Store params as a JSON object. set_params => { diff --git a/lib/DB/WithDates.pm b/lib/DB/WithDates.pm index 2cca1150..e04becef 100644 --- a/lib/DB/WithDates.pm +++ b/lib/DB/WithDates.pm @@ -12,7 +12,7 @@ use DB::Exception; my $valid_dates; # Arrayref of allowed/valid dates my $required_dates; # Arrayref of required dates -my $optional_fields_in_dates; # Arrayref of other non-date fields in the hash. +my $optional_fields_in_dates; # hashref of other non-date fields in the hash and the type. sub validDates ($self, $field_name) { $valid_dates = ref($self)->valid_dates; @@ -23,12 +23,13 @@ sub validDates ($self, $field_name) { $self->hasRequiredDateFields($field_name); $self->validDateFormat($field_name); $self->checkDates($field_name); + $self->validateOptionalFields($field_name); return 1; } sub validDateFields ($self, $field_name) { my @fields = keys %{ $self->get_inflated_column($field_name) }; - my @all_fields = (@$valid_dates, @$optional_fields_in_dates); + my @all_fields = (@$valid_dates, keys %$optional_fields_in_dates); # If this is not empty, there are illegal fields. my @bad_fields = array_minus(@fields, @all_fields); @@ -74,4 +75,20 @@ sub checkDates ($self, $field_name) { return 1; } +# This checks the options fields that aren't dates +sub validateOptionalFields ($self, $field_name) { + my $params_hash = $self->get_inflated_column($field_name); + # if it doesn't exist, it is valid + return 1 unless defined $params_hash; + + for my $key (keys %$optional_fields_in_dates) { + next unless defined $params_hash->{$key}; + my $re = $params_hash->{$key}; + my $valid = $re eq 'bool' ? JSON::PP::is_bool($params_hash->{$key}) : $params_hash->{$key} =~ qr/^$re$/x; + DB::Exception::InvalidParameter->throw( + message => "The parameter named $key is not valid. It has value $params_hash->{$key}") + unless $valid; + } +} + 1; diff --git a/t/db/005_hwsets.t b/t/db/005_hwsets.t index f488610d..e689fd21 100644 --- a/t/db/005_hwsets.t +++ b/t/db/005_hwsets.t @@ -269,9 +269,9 @@ throws_ok { # Check for undefined parameter fields my $new_set7 = { set_name => 'HW #11', - set_dates => { open => 100, due => 140, answer => 200 }, + set_dates => { open => 100, due => 140, answer => 200, enable_reduced_scoring => false }, set_type => 'HW', - set_params => { enable_reduced_scoring => false, not_a_valid_field => 5 } + set_params => { not_a_valid_field => 5 } }; throws_ok { $problem_set_rs->addProblemSet( @@ -289,9 +289,9 @@ throws_ok { params => { course_name => 'Precalculus', set_name => 'HW #11', - set_dates => { open => 100, due => 140, answer => 200 }, + set_dates => { open => 100, due => 140, answer => 200, enable_reduced_scoring => false }, set_type => 'HW', - set_params => { enable_reduced_scoring => false, hide_hint => 'yes' } + set_params => { hide_hint => 'yes' } } ); } @@ -303,14 +303,28 @@ throws_ok { params => { course_name => 'Precalculus', set_name => 'HW #11', - set_dates => { open => 100, due => 140, answer => 200 }, + set_dates => { open => 100, due => 140, answer => 200, enable_reduced_scoring => false }, set_type => 'HW', - set_params => { enable_reduced_scoring => 0, hide_hint => true } + set_params => { hide_hint => 0 } } ); } 'DB::Exception::InvalidParameter', 'addProblemSet: adding an non-valid boolean parameter'; +# Check to ensure true/false are passed into the enable_reduced_scoring in set_dates, not 0/1 +throws_ok { + $problem_set_rs->addProblemSet( + params => { + course_name => 'Precalculus', + set_name => 'HW #11', + set_dates => { open => 100, due => 140, answer => 200, enable_reduced_scoring => 0 }, + set_type => 'HW', + set_params => { hide_hint => 0 } + } + ); +} +'DB::Exception::InvalidParameter', 'addProblemSet: adding an non-valid boolean parameter in set_dates'; + # Update a set $new_set_params->{set_name} = "HW #8"; $new_set_params->{set_params} = { hide_hint => true }; From 5165dbe421c8684f021bb1b366e88e40181873e8 Mon Sep 17 00:00:00 2001 From: Peter Staab Date: Wed, 29 Jun 2022 13:11:44 -0400 Subject: [PATCH 23/34] FIX: perltidy/perlcritic errors --- lib/DB/Schema/ResultSet/UserSet.pm | 19 ++++++++++++++----- lib/DB/WithDates.pm | 1 + 2 files changed, 15 insertions(+), 5 deletions(-) diff --git a/lib/DB/Schema/ResultSet/UserSet.pm b/lib/DB/Schema/ResultSet/UserSet.pm index 95262c7b..349337de 100644 --- a/lib/DB/Schema/ResultSet/UserSet.pm +++ b/lib/DB/Schema/ResultSet/UserSet.pm @@ -395,14 +395,23 @@ sub addUserSet ($self, %args) { # If any of the dates are 0, then remove them. # Only check if the date_fields are 0. There is the option to store non-dates in the # set_dates field. - my $valid_date_str = $DB::Schema::Result::UserSet::set_type->{ $params->{type} } . '::valid_dates()'; - my $date_fields = eval($valid_date_str); + my $date_fields; + + # Note: although this works okay, if another subtype of a UserSet is created, this needs to be updated. + if ($params->{type} == 1) { + $date_fields = \&DB::Schema::Result::UserSet::HWSet::valid_dates; + } elsif ($params->{type} == 2) { + $date_fields = \&DB::Schema::Result::UserSet::Quiz::valid_dates; + } elsif ($params->{type} == 4) { + $date_fields = \&DB::Schema::Result::UserSet::ReviewSet::valid_dates; + } else { + die "The type $params->{type} is not valid."; + } if ($args{params}->{set_dates}) { - for my $key (@$date_fields) { + for my $key (@{ $date_fields->() }) { delete $args{params}->{set_dates}->{$key} - if defined($args{params}->{set_dates}->{$key}) - && $args{params}->{set_dates}->{$key} == 0; + if defined($args{params}->{set_dates}->{$key}) && $args{params}->{set_dates}->{$key} == 0; } } diff --git a/lib/DB/WithDates.pm b/lib/DB/WithDates.pm index e04becef..ca8a431f 100644 --- a/lib/DB/WithDates.pm +++ b/lib/DB/WithDates.pm @@ -89,6 +89,7 @@ sub validateOptionalFields ($self, $field_name) { message => "The parameter named $key is not valid. It has value $params_hash->{$key}") unless $valid; } + return 1; } 1; From f16d56d8354585cb4252910a5c735e6e7f62afb3 Mon Sep 17 00:00:00 2001 From: Peter Staab Date: Sat, 2 Jul 2022 11:22:27 -0400 Subject: [PATCH 24/34] FIX: issues after merge with main. --- src/common/models/problem_sets.ts | 20 +++++++++++--------- src/common/models/user_sets.ts | 14 +++++++++----- t/db/005_hwsets.t | 2 +- t/db/007_user_set.t | 2 +- t/db/012_set_versions.t | 2 +- tests/stores/problem_sets.spec.ts | 2 +- tests/stores/set_problems.spec.ts | 2 +- tests/stores/user_sets.spec.ts | 4 ++-- tests/unit-tests/review_sets.spec.ts | 14 +++++++------- tests/unit-tests/user_review_set.spec.ts | 4 ++-- 10 files changed, 36 insertions(+), 30 deletions(-) diff --git a/src/common/models/problem_sets.ts b/src/common/models/problem_sets.ts index f4fd9339..ea43e7d5 100644 --- a/src/common/models/problem_sets.ts +++ b/src/common/models/problem_sets.ts @@ -379,13 +379,12 @@ export class HomeworkSet extends ProblemSet { isValid(): boolean { return super.isValid() && this.set_params.isValid() && this.set_dates.isValid(); } - } // Review Set model and interfaces. export interface ParseableReviewSetParams { - can_retake?: boolean | string | number; + can_retake?: boolean; } export class ReviewSetParams extends Model { @@ -396,19 +395,22 @@ export class ReviewSetParams extends Model { this.set(params); } - set(params: ParseableReviewSetParams) { - if (params.can_retake != undefined) this.can_retake = params.can_retake; - } + static ALL_FIELDS = ['can_retake']; get all_field_names(): string[] { - return ['can_retake']; + return ReviewSetParams.ALL_FIELDS; } get param_fields(): string[] { return [];} - public get can_retake() : boolean { return this._can_retake;} - public set can_retake(value: number | string | boolean) { - this._can_retake = parseBoolean(value); + set(params: ParseableReviewSetParams) { + if (params.can_retake != undefined) this.can_retake = params.can_retake; } + + public get can_retake() : boolean { return this._can_retake;} + public set can_retake(value: boolean) { this._can_retake = value;} + + public isValid(): boolean { return true; } + public clone(): ReviewSetParams { return new ReviewSetParams(this.toObject()); } } export interface ParseableReviewSetDates { diff --git a/src/common/models/user_sets.ts b/src/common/models/user_sets.ts index 8c961fd3..855849c8 100644 --- a/src/common/models/user_sets.ts +++ b/src/common/models/user_sets.ts @@ -410,7 +410,7 @@ export class UserReviewSetDates extends Model { */ export class UserReviewSetParams extends Model { - private _test_param?: boolean; + private _can_retake?: boolean; constructor(params: ParseableReviewSetParams = {}) { super(); @@ -418,15 +418,19 @@ export class UserReviewSetParams extends Model { } set(params: ParseableReviewSetParams) { - this.test_param = params.test_param; + this.can_retake = params.can_retake; } - static ALL_FIELDS = ['test_params']; + static ALL_FIELDS = ['can_retakes']; get all_field_names(): string[] { return UserReviewSetParams.ALL_FIELDS; } get param_fields(): string[] { return [];} - public get test_param() : boolean | undefined { return this._test_param;} - public set test_param(value: boolean | undefined) { this._test_param = value; } + public get can_retake() : boolean | undefined { return this._can_retake;} + public set can_retake(value: boolean | undefined) { this._can_retake = value; } + + isValid(): boolean { + return true; + } } export class DBUserReviewSet extends DBUserSet { diff --git a/t/db/005_hwsets.t b/t/db/005_hwsets.t index fcb400db..c436efb7 100644 --- a/t/db/005_hwsets.t +++ b/t/db/005_hwsets.t @@ -68,7 +68,7 @@ my @review_sets = loadCSV( "$main::ww3_dir/t/db/sample_data/review_sets.csv", { boolean_fields => ['set_visible'], - param_boolean_fields => ['test_param'] + param_boolean_fields => ['can_retake'] } ); for my $set (@review_sets) { diff --git a/t/db/007_user_set.t b/t/db/007_user_set.t index 9ac2d480..06200497 100644 --- a/t/db/007_user_set.t +++ b/t/db/007_user_set.t @@ -72,7 +72,7 @@ my @review_sets = loadCSV( "$main::ww3_dir/t/db/sample_data/review_sets.csv", { boolean_fields => ['set_visible'], - param_boolean_fields => ['test_param'] + param_boolean_fields => ['can_retake'] } ); diff --git a/t/db/012_set_versions.t b/t/db/012_set_versions.t index d0f1a815..99b94077 100644 --- a/t/db/012_set_versions.t +++ b/t/db/012_set_versions.t @@ -64,7 +64,7 @@ my @review_sets = loadCSV( "$main::ww3_dir/t/db/sample_data/review_sets.csv", { boolean_fields => ['set_visible'], - param_boolean_fields => ['test_param'] + param_boolean_fields => ['can_retake'] } ); for my $set (@review_sets) { diff --git a/tests/stores/problem_sets.spec.ts b/tests/stores/problem_sets.spec.ts index 9fb18ad6..d10fa311 100644 --- a/tests/stores/problem_sets.spec.ts +++ b/tests/stores/problem_sets.spec.ts @@ -39,7 +39,7 @@ describe('Problem Set store tests', () => { const problem_set_config = { params: ['set_params', 'set_dates' ], boolean_fields: ['set_visible'], - param_boolean_fields: ['timed', 'enable_reduced_scoring', 'test_param'], + param_boolean_fields: ['timed', 'enable_reduced_scoring', 'can_retake'], param_non_neg_int_fields: ['quiz_duration'] }; diff --git a/tests/stores/set_problems.spec.ts b/tests/stores/set_problems.spec.ts index 7334366b..246d0311 100644 --- a/tests/stores/set_problems.spec.ts +++ b/tests/stores/set_problems.spec.ts @@ -49,7 +49,7 @@ describe('Problem Set store tests', () => { const problem_set_config = { params: ['set_params', 'set_dates' ], boolean_fields: ['set_visible'], - param_boolean_fields: ['timed', 'enable_reduced_scoring', 'test_param'], + param_boolean_fields: ['timed', 'enable_reduced_scoring', 'can_retake'], param_non_neg_int_fields: ['quiz_duration'] }; diff --git a/tests/stores/user_sets.spec.ts b/tests/stores/user_sets.spec.ts index 23874fba..deb4d81e 100644 --- a/tests/stores/user_sets.spec.ts +++ b/tests/stores/user_sets.spec.ts @@ -48,7 +48,7 @@ describe('Tests user sets and merged user sets in the problem set store', () => const problem_set_config = { params: ['set_params', 'set_dates' ], boolean_fields: ['set_visible'], - param_boolean_fields: ['timed', 'enable_reduced_scoring', 'test_param'], + param_boolean_fields: ['timed', 'enable_reduced_scoring', 'can_retake'], param_non_neg_int_fields: ['quiz_duration'] }; @@ -97,7 +97,7 @@ describe('Tests user sets and merged user sets in the problem set store', () => const user_sets_to_parse = await loadCSV('t/db/sample_data/user_sets.csv', { params: ['set_dates', 'set_params'], boolean_fields: ['set_visible'], - param_boolean_fields: ['timed', 'enable_reduced_scoring', 'test_param'], + param_boolean_fields: ['timed', 'enable_reduced_scoring', 'can_retake'], param_non_neg_int_fields: ['quiz_duration'] }); diff --git a/tests/unit-tests/review_sets.spec.ts b/tests/unit-tests/review_sets.spec.ts index 0c7a14b5..69dde9ab 100644 --- a/tests/unit-tests/review_sets.spec.ts +++ b/tests/unit-tests/review_sets.spec.ts @@ -10,7 +10,7 @@ describe('Testing for Review Sets', () => { closed: 0 }, set_params: { - test_param: false + can_retake: false }, set_id: 0, course_id: 0, @@ -46,7 +46,7 @@ describe('Testing for Review Sets', () => { set_id: 7, set_name: 'Review Set #1', set_params: { - can_retake: true, + can_retake: false }, set_visible: true, set_type: 'REVIEW' @@ -191,7 +191,7 @@ describe('Testing for Review Sets', () => { }); test('Check that calling all_fields() and params() is correct', () => { - const review_params_fields = ['test_param']; + const review_params_fields = ['can_retake']; const review_params = new ReviewSetParams(); expect(review_params.all_field_names.sort()).toStrictEqual(review_params_fields.sort()); @@ -209,14 +209,14 @@ describe('Testing for Review Sets', () => { describe('Check setting review set params', () => { test('Set review set params directly', () => { const hw_params = new ReviewSetParams(); - hw_params.test_param = true; - expect(hw_params.test_param).toBe(true); + hw_params.can_retake = true; + expect(hw_params.can_retake).toBe(true); }); test('Set homework set params using the set method', () => { const hw_params = new ReviewSetParams(); - hw_params.set({ test_param: true }); - expect(hw_params.test_param).toBe(true); + hw_params.set({ can_retake: true }); + expect(hw_params.can_retake).toBe(true); }); // No tests for validity for this currently because there is nothing diff --git a/tests/unit-tests/user_review_set.spec.ts b/tests/unit-tests/user_review_set.spec.ts index 38016189..0f143de2 100644 --- a/tests/unit-tests/user_review_set.spec.ts +++ b/tests/unit-tests/user_review_set.spec.ts @@ -18,7 +18,7 @@ describe('Testing db user Review Sets and User Review Sets', () => { course_user_id: 0, set_version: 1, set_type: 'REVIEW', - set_params: { can_retake: false }, + set_params: {}, set_dates: {} }; @@ -88,7 +88,7 @@ describe('Testing db user Review Sets and User Review Sets', () => { set_name: '', username: '', set_type: 'REVIEW', - set_params: { can_retake: false }, + set_params: {}, set_dates: { open: 0, closed: 0 } }; expect(user_review_set.toObject()).toStrictEqual(defaults); From e4a5dc9eda8473cd5b295a58e47e7b3440e2ad3c Mon Sep 17 00:00:00 2001 From: Peter Staab Date: Tue, 5 Jul 2022 06:58:33 -0400 Subject: [PATCH 25/34] FIX: cleanup after merge --- lib/DB/Schema/Result/ProblemSet.pm | 8 -------- src/common/models/courses.ts | 2 -- src/common/models/problem_sets.ts | 3 --- 3 files changed, 13 deletions(-) diff --git a/lib/DB/Schema/Result/ProblemSet.pm b/lib/DB/Schema/Result/ProblemSet.pm index 1876bdb9..3de8446c 100644 --- a/lib/DB/Schema/Result/ProblemSet.pm +++ b/lib/DB/Schema/Result/ProblemSet.pm @@ -130,14 +130,6 @@ __PACKAGE__->inflate_column( } ); -__PACKAGE__->inflate_column( - 'set_visible', - { - inflate => sub { return shift ? Mojo::JSON->true : Mojo::JSON->false; }, - deflate => sub { return shift; } - } -); - # This defines the non-abstract classes of ProblemSets. __PACKAGE__->typecast_map( diff --git a/src/common/models/courses.ts b/src/common/models/courses.ts index 77206951..37892d1a 100644 --- a/src/common/models/courses.ts +++ b/src/common/models/courses.ts @@ -75,7 +75,6 @@ export class Course extends Model { if (params.course_id != undefined) this.course_id = params.course_id; if (params.course_name != undefined) this.course_name = params.course_name; if (params.visible != undefined) this.visible = params.visible; - // super.checkParams(params as Dictionary); if (params.course_dates) this.setDates(params.course_dates); } @@ -146,7 +145,6 @@ export class UserCourse extends Model { if (params.user_id != undefined) this.user_id = params.user_id; if (params.username) this.username = params.username; if (params.role) this.role = params.role; - // super.checkParams(params as Dictionary); } setDates(date_params: ParseableCourseDates = {}) { diff --git a/src/common/models/problem_sets.ts b/src/common/models/problem_sets.ts index ea43e7d5..177adeac 100644 --- a/src/common/models/problem_sets.ts +++ b/src/common/models/problem_sets.ts @@ -532,8 +532,5 @@ export function convertSet(old_set: ProblemSet, new_set_type: ProblemSetType) { default: throw new ParseError('ProblemSetType', `convertSet does not support conversion to ${new_set_type || 'EMPTY'}`); } - - // if (!new_set.set_dates.isValid()) - // logger.error('[problem_sets/convertSet] corrupt dates in conversion of set, TSNH?'); return new_set; } From 282c1ea4bce2ac26d172478d8d199fa567b96b7c Mon Sep 17 00:00:00 2001 From: Peter Staab Date: Wed, 6 Jul 2022 16:07:53 -0400 Subject: [PATCH 26/34] FIX: unit tests for review sets. --- src/common/models/problem_sets.ts | 8 ++++---- tests/unit-tests/review_sets.spec.ts | 4 +--- 2 files changed, 5 insertions(+), 7 deletions(-) diff --git a/src/common/models/problem_sets.ts b/src/common/models/problem_sets.ts index 177adeac..163d5dcc 100644 --- a/src/common/models/problem_sets.ts +++ b/src/common/models/problem_sets.ts @@ -388,7 +388,7 @@ export interface ParseableReviewSetParams { } export class ReviewSetParams extends Model { - private _can_retake = false; + private _can_retake?: boolean; constructor(params: ParseableReviewSetParams = {}) { super(); @@ -403,11 +403,11 @@ export class ReviewSetParams extends Model { get param_fields(): string[] { return [];} set(params: ParseableReviewSetParams) { - if (params.can_retake != undefined) this.can_retake = params.can_retake; + this.can_retake = params.can_retake; } - public get can_retake() : boolean { return this._can_retake;} - public set can_retake(value: boolean) { this._can_retake = value;} + public get can_retake() : boolean | undefined { return this._can_retake;} + public set can_retake(value: boolean | undefined) { this._can_retake = value;} public isValid(): boolean { return true; } public clone(): ReviewSetParams { return new ReviewSetParams(this.toObject()); } diff --git a/tests/unit-tests/review_sets.spec.ts b/tests/unit-tests/review_sets.spec.ts index 69dde9ab..6c32d541 100644 --- a/tests/unit-tests/review_sets.spec.ts +++ b/tests/unit-tests/review_sets.spec.ts @@ -9,9 +9,7 @@ describe('Testing for Review Sets', () => { open: 0, closed: 0 }, - set_params: { - can_retake: false - }, + set_params: {}, set_id: 0, course_id: 0, set_name: '', From 7f6baed2c35cec2c5836e244166cb2606ad5a75e Mon Sep 17 00:00:00 2001 From: Peter Staab Date: Fri, 8 Jul 2022 13:57:43 -0400 Subject: [PATCH 27/34] FIX: linting after merge. --- .eslintrc.js | 1 + lib/DB/TestUtils.pm | 8 +++++--- src/common/api-requests/user.ts | 10 +++++++++- .../ClasslistManagerComponents/AddUsersFromFile.vue | 4 ++-- t/db/build_db.pl | 3 ++- 5 files changed, 19 insertions(+), 7 deletions(-) diff --git a/.eslintrc.js b/.eslintrc.js index 42c8664d..03c29ce9 100644 --- a/.eslintrc.js +++ b/.eslintrc.js @@ -25,6 +25,7 @@ const baseRules = { 'keyword-spacing': ['error'], 'space-before-blocks': ['error', 'always'], 'arrow-spacing': ['error'], + 'template-curly-spacing': ['error', 'never'], // allow console and debugger during development only 'no-console': process.env.NODE_ENV === 'development' ? 'off' : 'error', diff --git a/lib/DB/TestUtils.pm b/lib/DB/TestUtils.pm index 9fe530cc..4dd6b11b 100644 --- a/lib/DB/TestUtils.pm +++ b/lib/DB/TestUtils.pm @@ -42,6 +42,8 @@ sub buildHash ($input, $config) { } elsif (defined($input->{$key}) && $input->{$key} =~ /^\d{4}-\d{2}-\d{2}T\d\d:\d\d:\d\dZ$/) { my $dt = $strp_datetime->parse_datetime($input->{$key}); $output->{$field}->{$subfield} = $dt->epoch; + } elsif (grep {/^$subfield$/} @{ $config->{param_boolean_fields} }) { + $output->{$field}->{$subfield} = int($input->{$key}) ? true : false if defined($input->{$key}); } } elsif (grep { $_ eq $subfield } @{ $config->{param_boolean_fields} }) { $output->{$field}->{$subfield} = int($input->{$key}) ? true : false if defined($input->{$key}); @@ -52,11 +54,11 @@ sub buildHash ($input, $config) { } else { $output->{$field}->{$subfield} = $input->{$key} if defined($input->{$key}); } - } elsif (grep {/^$key$/} @{ $config->{boolean_fields} }) { + } elsif (grep { $_ eq $key } @{ $config->{boolean_fields} }) { $output->{$key} = defined($input->{$key}) && int($input->{$key}) ? true : false; - } elsif (grep {/^$key$/} @{ $config->{non_neg_int_fields} }) { + } elsif (grep { $_ eq $key } @{ $config->{non_neg_int_fields} }) { $output->{$key} = int($input->{$key}) if defined($input->{$key}); - } elsif (grep {/^$key$/} @{ $config->{non_neg_float_fields} }) { + } elsif (grep { $_ eq $key } @{ $config->{non_neg_float_fields} }) { $output->{$key} = 0 + $input->{$key} if defined($input->{$key}); } else { $output->{$key} = $input->{$key}; diff --git a/src/common/api-requests/user.ts b/src/common/api-requests/user.ts index 63ab37b2..94e1b94b 100644 --- a/src/common/api-requests/user.ts +++ b/src/common/api-requests/user.ts @@ -1,8 +1,16 @@ import { api } from 'boot/axios'; -import { ParseableUser, User } from 'src/common/models/users'; +import { ParseableCourseUser, ParseableUser, User } from 'src/common/models/users'; import { ResponseError } from 'src/common/api-requests/errors'; +export async function checkIfUserExists(course_id: number, username: string) { + const response = await api.get(`courses/${course_id}/users/${username}/exists`); + if (response.status === 250) { + throw response.data as ResponseError; + } + return response.data as ParseableCourseUser; +} + /** * Gets the global user in the database given by username. This returns a user or throws a * ResponseError if the user is not found. diff --git a/src/components/instructor/ClasslistManagerComponents/AddUsersFromFile.vue b/src/components/instructor/ClasslistManagerComponents/AddUsersFromFile.vue index 0276dd41..9c8a5ae1 100644 --- a/src/components/instructor/ClasslistManagerComponents/AddUsersFromFile.vue +++ b/src/components/instructor/ClasslistManagerComponents/AddUsersFromFile.vue @@ -341,7 +341,7 @@ const addMergedUsers = async () => { let global_user: User | undefined; try { // Skip if username is undefined? - global_user = await getUser(user.username ?? '') as User; + global_user = await getUser(user.username ?? ''); } catch (err) { const error = err as ResponseError; // this will occur is the user is not a global user @@ -366,7 +366,7 @@ const addMergedUsers = async () => { } try { await users.addCourseUser(new CourseUser(user)); - const full_name = `${user.first_name as string} ${user.last_name as string}`; + const full_name = `${user.first_name} ${user.last_name}`; $q.notify({ message: `The user ${full_name} was successfully added to the course.`, color: 'green', diff --git a/t/db/build_db.pl b/t/db/build_db.pl index f85f6cdd..c85acafb 100755 --- a/t/db/build_db.pl +++ b/t/db/build_db.pl @@ -132,9 +132,10 @@ sub addSets { "$main::ww3_dir/t/db/sample_data/hw_sets.csv", { boolean_fields => ['set_visible'], - param_boolean_fields => [ 'enable_reduced_scoring', 'hide_hint' ] + param_boolean_fields => ['enable_reduced_scoring', 'hide_hint'] } ); + for my $set (@hw_sets) { my $course = $course_rs->find({ course_name => $set->{course_name} }); if (!defined($course)) { From f688760a9405dba80173781c6305cd80b033cad1 Mon Sep 17 00:00:00 2001 From: Peter Staab Date: Mon, 11 Jul 2022 10:18:41 -0400 Subject: [PATCH 28/34] FIX: perltidy --- t/db/build_db.pl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/t/db/build_db.pl b/t/db/build_db.pl index c85acafb..06ea4450 100755 --- a/t/db/build_db.pl +++ b/t/db/build_db.pl @@ -132,7 +132,7 @@ sub addSets { "$main::ww3_dir/t/db/sample_data/hw_sets.csv", { boolean_fields => ['set_visible'], - param_boolean_fields => ['enable_reduced_scoring', 'hide_hint'] + param_boolean_fields => [ 'enable_reduced_scoring', 'hide_hint' ] } ); From dbe2f02938ce21b8f4494340927813e4d8f4ec1d Mon Sep 17 00:00:00 2001 From: Peter Staab Date: Fri, 15 Jul 2022 14:50:17 -0400 Subject: [PATCH 29/34] FIX: add users manually wasn't working. There was related model/store issues too. --- lib/WeBWorK3.pm | 39 ++++--- lib/WeBWorK3/Controller/User.pm | 29 ++++- src/common/api-requests/user.ts | 18 ++- src/common/models/users.ts | 1 + .../AddUsersManually.vue | 105 ++++++++---------- src/stores/users.ts | 35 +++++- 6 files changed, 143 insertions(+), 84 deletions(-) diff --git a/lib/WeBWorK3.pm b/lib/WeBWorK3.pm index fd997db6..50f8e57b 100644 --- a/lib/WeBWorK3.pm +++ b/lib/WeBWorK3.pm @@ -104,29 +104,40 @@ sub userRoutes ($self) { my $user_routes = $self->routes->any('/webwork3/api/users')->requires(authenticated => 1)->to(controller => 'User'); $user_routes->get('/')->to(action => 'getGlobalUsers'); $user_routes->post('/')->to(action => 'addGlobalUser'); - $user_routes->get('/:user_id')->to(action => 'getGlobalUser'); + $user_routes->get('/:user')->to(action => 'getGlobalUser'); $user_routes->put('/:user_id')->to(action => 'updateGlobalUser'); $user_routes->delete('/:user_id')->to(action => 'deleteGlobalUser'); $user_routes->get('/:user_id/courses')->to(action => 'getUserCourses'); # Get global users for a course. $self->routes->get('/webwork3/api/courses/:course_id/global-users')->to('User#getGlobalCourseUsers'); - # This is needed to get global users as instructor permission. Need to have - # the parameter course_id. - $self->routes->any('/webwork3/api/courses/:course_id/users/:user/exists')->requires(authenticated => 1) - ->to('User#getGlobalUser'); + return; } sub courseUserRoutes ($self) { - my $course_user_routes = $self->routes->any('/webwork3/api/courses/:course_id/users')->requires(authenticated => 1) - ->to(controller => 'User'); - $course_user_routes->get('/')->to(action => 'getCourseUsers'); - $course_user_routes->post('/')->to(action => 'addCourseUser'); - $course_user_routes->get('/:user_id')->to(action => 'getCourseUser'); - $course_user_routes->put('/:user_id')->to(action => 'updateCourseUser'); - $course_user_routes->delete('/:user_id')->to(action => 'deleteCourseUser'); - $self->routes->any('/webwork3/api/courses/:course_id/courseusers')->requires(authenticated => 1) - ->to('User#getMergedCourseUsers'); + my $course_routes = + $self->routes->any('/webwork3/api/courses/:course_id')->requires(authenticated => 1)->to(controller => 'User'); + $course_routes->get('/users')->to(action => 'getCourseUsers'); + $course_routes->get('/users')->to(action => 'getCourseUsers'); + $course_routes->post('/users')->to(action => 'addCourseUser'); + $course_routes->get('/users/:user_id')->to(action => 'getCourseUser'); + $course_routes->put('/users/:user_id')->to(action => 'updateCourseUser'); + $course_routes->delete('/users/:user_id')->to(action => 'deleteCourseUser'); + + # There are some routes needed for global user crud, but the permssions require that the + # user has permissions within a course. + + $course_routes->get('/global-courseusers')->to('User#getGlobalCourseUsers'); + $course_routes->post('/global-users')->to('User#addGlobalUserFromCourse'); + $course_routes->get('/global-users/:user')->to('User#getGlobalUserFromCourse'); + $course_routes->put('/global-users/:user_id')->to('User#updateGlobalUserFromCourse'); + $course_routes->delete('/global-users/:user_id')->to('User#deleteGlobalUserFromCourse'); + + $course_routes->get('/courseusers')->to('User#getMergedCourseUsers'); + + # This is used to check if a user with given username exists. + $course_routes->get('/users/:username/exists')->to('User#checkGlobalUser'); + return; } diff --git a/lib/WeBWorK3/Controller/User.pm b/lib/WeBWorK3/Controller/User.pm index 5612c8d1..61f939b6 100644 --- a/lib/WeBWorK3/Controller/User.pm +++ b/lib/WeBWorK3/Controller/User.pm @@ -1,17 +1,32 @@ package WeBWorK3::Controller::User; use Mojo::Base 'Mojolicious::Controller', -signatures; +use Try::Tiny; + sub getGlobalUsers ($self) { my @global_users = $self->schema->resultset('User')->getAllGlobalUsers; $self->render(json => \@global_users); return; } +# Passing the username into the getGlobalUser results in a problem with permssions. This route +# should be used to pass in the username. +sub checkGlobalUser ($self) { + try { + my $user = $self->schema->resultset('User')->getGlobalUser(info => { username => $self->param('username') }); + $self->render(json => $user); + return; + } catch { + $self->render(json => {}) if ref($_) eq 'DB::Exception::UserNotFound'; + }; + return; +} + sub getGlobalUser ($self) { my $user = - $self->param('user_id') =~ /^\d+$/ - ? $self->schema->resultset('User')->getGlobalUser(info => { user_id => int($self->param('user_id')) }) - : $self->schema->resultset('User')->getGlobalUser(info => { username => $self->param('user_id') }); + $self->param('user') =~ /^\d+$/ + ? $self->schema->resultset('User')->getGlobalUser(info => { user_id => int($self->param('user')) }) + : $self->schema->resultset('User')->getGlobalUser(info => { username => $self->param('user') }); $self->render(json => $user); return; } @@ -48,6 +63,14 @@ sub getGlobalCourseUsers ($self) { return; } +# The following are needed for handling global users from instructors in a course + +sub getGlobalUsersFromCourse ($self) { $self->getGlobalUsers; return } +sub getGlobalUserFromCourse ($self) { $self->getGlobalUser; return } +sub addGlobalUserFromCourse ($self) { $self->addGlobalUser; return } +sub updateGlobalUserFromCourse ($self) { $self->updateGlobalUser; return } +sub deleteGlobalUserFromCourse ($self) { $self->deleteGlobalUser; return } + # The following subs are related to users within a given course. sub getMergedCourseUsers ($self) { diff --git a/src/common/api-requests/user.ts b/src/common/api-requests/user.ts index 94e1b94b..18fcefad 100644 --- a/src/common/api-requests/user.ts +++ b/src/common/api-requests/user.ts @@ -3,12 +3,20 @@ import { api } from 'boot/axios'; import { ParseableCourseUser, ParseableUser, User } from 'src/common/models/users'; import { ResponseError } from 'src/common/api-requests/errors'; -export async function checkIfUserExists(course_id: number, username: string) { - const response = await api.get(`courses/${course_id}/users/${username}/exists`); - if (response.status === 250) { - throw response.data as ResponseError; +/** + * Checks if a global user exists. Both the course_id and username need to be passed in + * in order to check permissions. + * + * @returns either an existing user or an empty object. + */ + +export async function checkIfUserExists(course_id: number, username: string): Promise { + try { + const response = await api.get(`courses/${course_id}/users/${username}/exists`); + return response.data as ParseableCourseUser; + } catch (_err) { + return {}; } - return response.data as ParseableCourseUser; } /** diff --git a/src/common/models/users.ts b/src/common/models/users.ts index e531e8a3..104d2622 100644 --- a/src/common/models/users.ts +++ b/src/common/models/users.ts @@ -153,6 +153,7 @@ export class DBCourseUser extends Model { return isNonNegInt(this.user_id) && isNonNegInt(this.course_user_id) && isNonNegInt(this.course_id); } + } export interface ParseableCourseUser { diff --git a/src/components/instructor/ClasslistManagerComponents/AddUsersManually.vue b/src/components/instructor/ClasslistManagerComponents/AddUsersManually.vue index a80c0a55..a5770152 100644 --- a/src/components/instructor/ClasslistManagerComponents/AddUsersManually.vue +++ b/src/components/instructor/ClasslistManagerComponents/AddUsersManually.vue @@ -15,7 +15,7 @@ @blur="checkUser" ref="username_ref" lazy-rules - :rules="[(val) => validateUsername(val)]" + :rules="[val => validateUsername(val)]" />
@@ -30,7 +30,7 @@ v-model="course_user.email" label="Email" lazy-rules - :rules="[(val) => validateEmail(val)]" + :rules="[val => validateEmail(val)]" :disable="user_exists" />
@@ -53,7 +53,7 @@ :options="roles" label="Role *" :validate="validateRole" - :rules="[(val) => validateRole(val)]" + :rules="[val => validateRole(val)]" />
@@ -73,14 +73,13 @@ import { ref, computed, defineEmits } from 'vue'; import { useQuasar } from 'quasar'; import { logger } from 'boot/logger'; -import { getUser } from 'src/common/api-requests/user'; +import { checkIfUserExists } from 'src/common/api-requests/user'; import { useUserStore } from 'src/stores/users'; import { useSessionStore } from 'src/stores/session'; import { useSettingsStore } from 'src/stores/settings'; -import { CourseUser, ParseableCourseUser, User } from 'src/common/models/users'; +import { CourseUser, User } from 'src/common/models/users'; import type { ResponseError } from 'src/common/api-requests/errors'; -import { AxiosError } from 'axios'; import { isValidEmail, isValidUsername, parseNonNegInt } from 'src/common/models/parsers'; interface QRef { @@ -101,18 +100,14 @@ const settings = useSettingsStore(); // see if the user exists already and fill in the known fields const checkUser = async () => { - // If the user doesn't exist, the catch statement will handle this. - try { - const existing_user = await getUser(course_user.value.username); - course_user.value.set(existing_user.toObject() as ParseableCourseUser); - } catch (err) { - const error = err as ResponseError; - // this will occur is the user is not a global user - if (error.exception === 'DB::Exception::UserNotFound') { - user_exists.value = false; - } else { - logger.error(error.message); - } + const existing_user = await checkIfUserExists(session.course.course_id, course_user.value.username); + if (existing_user.username) { + course_user.value.set(existing_user); + user_exists.value = true; + } else { + // make sure the other fields are emptied. This can happen if they were prefilled. + course_user.value = new CourseUser({ username: course_user.value.username }); + user_exists.value = false; } }; @@ -121,55 +116,53 @@ const roles = computed(() => (settings.getCourseSetting('roles').value as string[]).filter(v => v !== 'admin')); const addUser = async (close: boolean) => { - try { - // Check to ensure username is correct and a role is selected. + + // Check that the user fields are valid. + if (! course_user.value.isValid()) { if (username_ref.value && role_ref.value) { username_ref.value.validate(); role_ref.value.validate(); - if (username_ref.value.hasError || role_ref.value.hasError) return; } + return; + } - // if the user is not global, add them. - if (!user_exists.value) { - try { - logger.debug(`Trying to add the new global user ${course_user.value.username ?? 'UNKNOWN'}`); - const global_user = await user_store.addUser(new User(course_user.value)); - if (global_user == undefined) throw `There is an error adding the user ${course_user.value.username}`; - const msg = `The global user with username ${global_user?.username ?? 'UNKNOWN'} was created.`; - $q.notify({ message: msg, color: 'green' }); - logger.debug(msg); - course_user.value.user_id = global_user.user_id; - } catch (err) { - const error = err as ResponseError; - $q.notify({ message: error.message, color: 'red' }); - } + // if the user is not global, add them. + if (!user_exists.value) { + try { + logger.debug(`Trying to add the new global user ${course_user.value.username ?? 'UNKNOWN'}`); + const global_user = await user_store.addUser(new User(course_user.value)); + if (global_user == undefined) throw `There is an error adding the user ${course_user.value.username}`; + const msg = `The global user with username ${global_user?.username ?? 'UNKNOWN'} was created.`; + $q.notify({ message: msg, color: 'green' }); + logger.debug(msg); + course_user.value.user_id = global_user.user_id; + } catch (err) { + const error = err as ResponseError; + $q.notify({ message: error.message, color: 'red' }); } + } - course_user.value.course_id = session.course.course_id; - const user = await user_store.addCourseUser(new CourseUser(course_user.value)); - const u = user_store.findCourseUser({ user_id: parseNonNegInt(user.user_id ?? 0) }); - $q.notify({ - message: `The user with username '${u.username ?? ''}' was added successfully.`, - color: 'green' - }); - if (close) { - emit('closeDialog'); - } else { - course_user.value = new CourseUser(); - } - } catch (err) { - const error = err as AxiosError; - logger.error(error); - const data = error?.response?.data as ResponseError || { exception: '' }; - $q.notify({ - message: data.exception, - color: 'red' - }); + course_user.value.course_id = session.course.course_id; + const user = await user_store.addCourseUser(new CourseUser(course_user.value)); + + // If the user exist globally, fetch the user to be added to the store + if (user_exists.value) { + await user_store.fetchUser(user.user_id); + } + const u = user_store.findCourseUser({ user_id: parseNonNegInt(user.user_id ?? 0) }); + $q.notify({ + message: `The user with username '${u.username ?? ''}' was added successfully.`, + color: 'green' + }); + if (close) { + emit('closeDialog'); + } else { + course_user.value = new CourseUser(); } }; const validateRole = (val: string | null) => { - if (val == undefined) { + if (val == undefined || val == 'UNKNOWN') { return 'You must select a role'; } return true; diff --git a/src/stores/users.ts b/src/stores/users.ts index 651d067e..f568543f 100644 --- a/src/stores/users.ts +++ b/src/stores/users.ts @@ -88,7 +88,7 @@ export const useUserStore = defineStore('user', { * Fetch all global users in all courses and store the results. */ async fetchUsers(): Promise { - const response = await api.get('users'); + const response = await api.get('users'); if (response.status === 200) { const users_to_parse = response.data as Array; this.users = users_to_parse.map(user => new User(user)); @@ -99,13 +99,29 @@ export const useUserStore = defineStore('user', { } }, + /** + * Fetch a single global user and add to the store. + */ + async fetchUser(user_id: number): Promise { + const session_store = useSessionStore(); + const course_id = session_store.course.course_id; + const response = await api.get(`courses/${course_id}/global-users/${user_id}`); + if (response.status === 200) { + this.users.push(new User(response.data as ParseableUser)); + } else { + const error = response.data as ResponseError; + logger.error(`${error.exception}: ${error.message}`); + throw new Error(error.message); + } + }, + /** * Fetch the global users for a given course and store the results. */ // the users are stored in the same users field as the all global users // Perhaps this is a problem. async fetchGlobalCourseUsers(course_id: number): Promise { - const response = await api.get(`courses/${course_id}/global-users`); + const response = await api.get(`courses/${course_id}/global-courseusers`); if (response.status === 200) { const users = response.data as ParseableUser[]; this.users = users.map(user => new User(user)); @@ -121,7 +137,9 @@ export const useUserStore = defineStore('user', { async updateUser(user: User): Promise { if (!user.isValid()) return invalidError(user, 'The updated user is invalid'); - const response = await api.put(`users/${user.user_id}`, user.toObject()); + const session_store = useSessionStore(); + const course_id = session_store.course.course_id; + const response = await api.put(`courses/${course_id}/global-users/${user.user_id}`, user.toObject()); if (response.status === 200) { const updated_user = new User(response.data as ParseableUser); const index = this.users.findIndex(user => user.user_id === updated_user.user_id); @@ -138,7 +156,9 @@ export const useUserStore = defineStore('user', { * Deletes the given User in the database and in the store. */ async deleteUser(user: User): Promise { - const response = await api.delete(`/users/${user.user_id ?? 0}`); + const session_store = useSessionStore(); + const course_id = session_store.course.course_id; + const response = await api.delete(`courses/${course_id}/global-users/${user.user_id}`); if (response.status === 200) { const index = this.users.findIndex((u) => u.user_id === user.user_id); // splice is used so vue3 reacts to changes. @@ -153,7 +173,9 @@ export const useUserStore = defineStore('user', { async addUser(user: User): Promise { if (!user.isValid()) return invalidError(user, 'The added user is invalid.'); - const response = await api.post('users', user.toObject()); + const session_store = useSessionStore(); + const course_id = session_store.course.course_id; + const response = await api.post(`courses/${course_id}/global-users`, user.toObject()); if (response.status === 200) { const new_user = new User(response.data as ParseableUser); this.users.push(new_user); @@ -247,8 +269,9 @@ export const useUserStore = defineStore('user', { const response = await api.delete(`courses/${course_user.course_id}/users/${course_user.user_id}`); if (response.status === 200) { const index = this.db_course_users.findIndex((u) => u.course_user_id === course_user.course_user_id); + // splice is used so vue3 reacts to changes. - this.course_users.splice(index, 1); + this.db_course_users.splice(index, 1); const deleted_course_user = new DBCourseUser(response.data as ParseableCourseUser); const user = this.users.find(u => u.user_id === deleted_course_user.user_id); return new CourseUser(Object.assign({}, user?.toObject(), deleted_course_user.toObject())); From 9750fa04e206bb805cffea955b87c60b2a3a47b6 Mon Sep 17 00:00:00 2001 From: Peter Staab Date: Fri, 15 Jul 2022 15:14:28 -0400 Subject: [PATCH 30/34] FIX: linting error. --- src/stores/users.ts | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/src/stores/users.ts b/src/stores/users.ts index f568543f..020dd994 100644 --- a/src/stores/users.ts +++ b/src/stores/users.ts @@ -88,7 +88,7 @@ export const useUserStore = defineStore('user', { * Fetch all global users in all courses and store the results. */ async fetchUsers(): Promise { - const response = await api.get('users'); + const response = await api.get('users'); if (response.status === 200) { const users_to_parse = response.data as Array; this.users = users_to_parse.map(user => new User(user)); @@ -102,10 +102,10 @@ export const useUserStore = defineStore('user', { /** * Fetch a single global user and add to the store. */ - async fetchUser(user_id: number): Promise { + async fetchUser(user_id: number): Promise { const session_store = useSessionStore(); - const course_id = session_store.course.course_id; - const response = await api.get(`courses/${course_id}/global-users/${user_id}`); + const course_id = session_store.course.course_id; + const response = await api.get(`courses/${course_id}/global-users/${user_id}`); if (response.status === 200) { this.users.push(new User(response.data as ParseableUser)); } else { @@ -138,8 +138,8 @@ export const useUserStore = defineStore('user', { if (!user.isValid()) return invalidError(user, 'The updated user is invalid'); const session_store = useSessionStore(); - const course_id = session_store.course.course_id; - const response = await api.put(`courses/${course_id}/global-users/${user.user_id}`, user.toObject()); + const course_id = session_store.course.course_id; + const response = await api.put(`courses/${course_id}/global-users/${user.user_id}`, user.toObject()); if (response.status === 200) { const updated_user = new User(response.data as ParseableUser); const index = this.users.findIndex(user => user.user_id === updated_user.user_id); @@ -157,8 +157,8 @@ export const useUserStore = defineStore('user', { */ async deleteUser(user: User): Promise { const session_store = useSessionStore(); - const course_id = session_store.course.course_id; - const response = await api.delete(`courses/${course_id}/global-users/${user.user_id}`); + const course_id = session_store.course.course_id; + const response = await api.delete(`courses/${course_id}/global-users/${user.user_id}`); if (response.status === 200) { const index = this.users.findIndex((u) => u.user_id === user.user_id); // splice is used so vue3 reacts to changes. @@ -174,8 +174,8 @@ export const useUserStore = defineStore('user', { if (!user.isValid()) return invalidError(user, 'The added user is invalid.'); const session_store = useSessionStore(); - const course_id = session_store.course.course_id; - const response = await api.post(`courses/${course_id}/global-users`, user.toObject()); + const course_id = session_store.course.course_id; + const response = await api.post(`courses/${course_id}/global-users`, user.toObject()); if (response.status === 200) { const new_user = new User(response.data as ParseableUser); this.users.push(new_user); From 89c5842a8c6c769c19273bc1dc3c5d173a774570 Mon Sep 17 00:00:00 2001 From: Peter Staab Date: Fri, 15 Jul 2022 15:20:14 -0400 Subject: [PATCH 31/34] FIX: make a server call only if the username is valid --- .../AddUsersManually.vue | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/src/components/instructor/ClasslistManagerComponents/AddUsersManually.vue b/src/components/instructor/ClasslistManagerComponents/AddUsersManually.vue index a5770152..15fcb18f 100644 --- a/src/components/instructor/ClasslistManagerComponents/AddUsersManually.vue +++ b/src/components/instructor/ClasslistManagerComponents/AddUsersManually.vue @@ -100,14 +100,17 @@ const settings = useSettingsStore(); // see if the user exists already and fill in the known fields const checkUser = async () => { - const existing_user = await checkIfUserExists(session.course.course_id, course_user.value.username); - if (existing_user.username) { - course_user.value.set(existing_user); - user_exists.value = true; - } else { - // make sure the other fields are emptied. This can happen if they were prefilled. - course_user.value = new CourseUser({ username: course_user.value.username }); - user_exists.value = false; + // check first if the username is valid. + if (validateUsername(course_user.value.username) === true) { + const existing_user = await checkIfUserExists(session.course.course_id, course_user.value.username); + if (existing_user.username) { + course_user.value.set(existing_user); + user_exists.value = true; + } else { + // make sure the other fields are emptied. This can happen if they were prefilled. + course_user.value = new CourseUser({ username: course_user.value.username }); + user_exists.value = false; + } } }; From 5ce8918ce8bd20b79382f75f4f3c8c7c932882da Mon Sep 17 00:00:00 2001 From: Peter Staab Date: Fri, 22 Jul 2022 09:48:22 -0400 Subject: [PATCH 32/34] FIX: duplicated rule --- .eslintrc.js | 1 - 1 file changed, 1 deletion(-) diff --git a/.eslintrc.js b/.eslintrc.js index f856be86..e9741088 100644 --- a/.eslintrc.js +++ b/.eslintrc.js @@ -26,7 +26,6 @@ const baseRules = { 'keyword-spacing': ['error'], 'space-before-blocks': ['error', 'always'], 'arrow-spacing': ['error'], - 'template-curly-spacing': ['error', 'never'], // allow console and debugger during development only 'no-console': process.env.NODE_ENV === 'development' ? 'off' : 'error', From 7731de857fd6a303a6de7b402aaac2938ef1309d Mon Sep 17 00:00:00 2001 From: Peter Staab Date: Fri, 22 Jul 2022 14:07:59 -0400 Subject: [PATCH 33/34] FIX: move TestUtils.pm to t/lib and remove unnecessary $set_type variable in UserSet.pm --- lib/DB/Schema/Result/UserSet.pm | 7 ------- lib/DB/Schema/ResultSet/Course.pm | 2 +- t/db/001_courses.t | 3 ++- t/db/002_course_settings.t | 3 ++- t/db/003_users.t | 3 ++- t/db/004_course_users.t | 3 ++- t/db/005_hwsets.t | 3 ++- t/db/006_quizzes.t | 3 ++- t/db/007_user_set.t | 3 ++- t/db/008_problem_pools.t | 3 ++- t/db/009_problems.t | 3 ++- t/db/010_user_problems.t | 3 ++- t/db/011_attempts.t | 3 ++- t/db/012_set_versions.t | 3 ++- t/db/013_problem_versions.t | 3 ++- t/db/build_db.pl | 2 +- t/db/test_course_user.pl | 2 +- {lib/DB => t/lib}/TestUtils.pm | 2 +- t/mojolicious/005_problem_sets.t | 3 ++- t/mojolicious/006_quizzes.t | 3 ++- t/mojolicious/007_review_sets.t | 3 ++- 21 files changed, 36 insertions(+), 27 deletions(-) rename {lib/DB => t/lib}/TestUtils.pm (99%) diff --git a/lib/DB/Schema/Result/UserSet.pm b/lib/DB/Schema/Result/UserSet.pm index efb82b24..49969aa6 100644 --- a/lib/DB/Schema/Result/UserSet.pm +++ b/lib/DB/Schema/Result/UserSet.pm @@ -155,13 +155,6 @@ __PACKAGE__->typecast_map( } ); -our $set_type = { - 1 => 'DB::Schema::Result::UserSet::HWSet', - 2 => 'DB::Schema::Result::UserSet::Quiz', - 3 => 'DB::Schema::Result::UserSet::JITAR', - 4 => 'DB::Schema::Result::UserSet::ReviewSet' -}; - sub set_type ($) { my %set_type_rev = reverse %{$DB::Schema::ResultSet::ProblemSet::SET_TYPES}; return $set_type_rev{ shift->type }; diff --git a/lib/DB/Schema/ResultSet/Course.pm b/lib/DB/Schema/ResultSet/Course.pm index f7921a3b..a4cdbb2a 100644 --- a/lib/DB/Schema/ResultSet/Course.pm +++ b/lib/DB/Schema/ResultSet/Course.pm @@ -12,7 +12,7 @@ use DB::Utils qw/getCourseInfo getUserInfo/; use DB::Exception; use Exception::Class ('DB::Exception::CourseNotFound', 'DB::Exception::CourseExists'); -#use DB::TestUtils qw/removeIDs/; +#use TestUtils qw/removeIDs/; use WeBWorK3::Utils::Settings qw/getDefaultCourseSettings mergeCourseSettings getDefaultCourseValues validateCourseSettings/; diff --git a/t/db/001_courses.t b/t/db/001_courses.t index 5878b64e..46994789 100644 --- a/t/db/001_courses.t +++ b/t/db/001_courses.t @@ -12,6 +12,7 @@ BEGIN { } use lib "$main::ww3_dir/lib"; +use lib "$main::ww3_dir/t/lib"; use List::MoreUtils qw(uniq); @@ -24,7 +25,7 @@ use DB::WithParams; use DB::WithDates; use DB::Schema; -use DB::TestUtils qw/loadCSV removeIDs loadSchema/; +use TestUtils qw/loadCSV removeIDs loadSchema/; # Load the database my $config_file = "$main::ww3_dir/conf/ww3-dev.yml"; diff --git a/t/db/002_course_settings.t b/t/db/002_course_settings.t index 845ca077..64af43d8 100644 --- a/t/db/002_course_settings.t +++ b/t/db/002_course_settings.t @@ -12,6 +12,7 @@ BEGIN { } use lib "$main::ww3_dir/lib"; +use lib "$main::ww3_dir/t/lib"; use Test::More; use Test::Exception; @@ -23,7 +24,7 @@ use WeBWorK3::Utils::Settings qw/getDefaultCourseSettings getDefaultCourseValues validateSettingsConfFile validateSingleCourseSetting validateSettingConfig isInteger isTimeString isTimeDuration isDecimal mergeCourseSettings/; -use DB::TestUtils qw/removeIDs loadSchema/; +use TestUtils qw/removeIDs loadSchema/; # Load the database my $config_file = "$main::ww3_dir/conf/ww3-dev.yml"; diff --git a/t/db/003_users.t b/t/db/003_users.t index ce41df37..3704a353 100644 --- a/t/db/003_users.t +++ b/t/db/003_users.t @@ -12,6 +12,7 @@ BEGIN { } use lib "$main::ww3_dir/lib"; +use lib "$main::ww3_dir/t/lib"; use Test::More; use Test::Exception; @@ -22,7 +23,7 @@ use DateTime::Format::Strptime; use Mojo::JSON qw/true false/; use DB::Schema; -use DB::TestUtils qw/loadCSV removeIDs/; +use TestUtils qw/loadCSV removeIDs/; # Load the database my $config_file = "$main::ww3_dir/conf/ww3-dev.yml"; diff --git a/t/db/004_course_users.t b/t/db/004_course_users.t index 41cd11c9..d1d706a2 100644 --- a/t/db/004_course_users.t +++ b/t/db/004_course_users.t @@ -12,6 +12,7 @@ BEGIN { } use lib "$main::ww3_dir/lib"; +use lib "$main::ww3_dir/t/lib"; use Test::More; use Test::Exception; @@ -20,7 +21,7 @@ use Clone qw/clone/; use Mojo::JSON qw/true false/; use DB::Schema; -use DB::TestUtils qw/loadCSV removeIDs/; +use TestUtils qw/loadCSV removeIDs/; use DB::Utils qw/removeLoginParams/; # Load the database diff --git a/t/db/005_hwsets.t b/t/db/005_hwsets.t index c436efb7..6f240f61 100644 --- a/t/db/005_hwsets.t +++ b/t/db/005_hwsets.t @@ -12,6 +12,7 @@ BEGIN { } use lib "$main::ww3_dir/lib"; +use lib "$main::ww3_dir/t/lib"; use Test::More; use Test::Exception; @@ -21,7 +22,7 @@ use DateTime::Format::Strptime; use Mojo::JSON qw/true false/; use DB::Schema; -use DB::TestUtils qw/loadCSV removeIDs filterBySetType/; +use TestUtils qw/loadCSV removeIDs filterBySetType/; # Load the database my $config_file = "$main::ww3_dir/conf/ww3-dev.yml"; diff --git a/t/db/006_quizzes.t b/t/db/006_quizzes.t index 18692319..7d157512 100644 --- a/t/db/006_quizzes.t +++ b/t/db/006_quizzes.t @@ -12,6 +12,7 @@ BEGIN { } use lib "$main::ww3_dir/lib"; +use lib "$main::ww3_dir/t/lib"; use Test::More; use Test::Exception; @@ -21,7 +22,7 @@ use DateTime::Format::Strptime; use Mojo::JSON qw/true false/; use DB::Schema; -use DB::TestUtils qw/loadCSV removeIDs filterBySetType/; +use TestUtils qw/loadCSV removeIDs filterBySetType/; # Load the database my $config_file = "$main::ww3_dir/conf/ww3-dev.yml"; diff --git a/t/db/007_user_set.t b/t/db/007_user_set.t index 06200497..6ee5c2d5 100644 --- a/t/db/007_user_set.t +++ b/t/db/007_user_set.t @@ -13,6 +13,7 @@ BEGIN { } use lib "$main::ww3_dir/lib"; +use lib "$main::ww3_dir/t/lib"; use DateTime::Format::Strptime; use Test::More; @@ -23,7 +24,7 @@ use YAML::XS qw/LoadFile/; use Mojo::JSON qw/true false/; use DB::Schema; -use DB::TestUtils qw/loadCSV removeIDs/; +use TestUtils qw/loadCSV removeIDs/; # Load the configuration files my $config_file = "$main::ww3_dir/conf/ww3-dev.yml"; diff --git a/t/db/008_problem_pools.t b/t/db/008_problem_pools.t index 114e35c5..292f2d10 100644 --- a/t/db/008_problem_pools.t +++ b/t/db/008_problem_pools.t @@ -12,13 +12,14 @@ BEGIN { } use lib "$main::ww3_dir/lib"; +use lib "$main::ww3_dir/t/lib"; use Test::More; use Test::Exception; use YAML::XS qw/LoadFile/; use DB::Schema; -use DB::TestUtils qw/loadCSV removeIDs/; +use TestUtils qw/loadCSV removeIDs/; # Load the database my $config_file = "$main::ww3_dir/conf/ww3-dev.yml"; diff --git a/t/db/009_problems.t b/t/db/009_problems.t index 3582949e..911f809c 100644 --- a/t/db/009_problems.t +++ b/t/db/009_problems.t @@ -12,6 +12,7 @@ BEGIN { } use lib "$main::ww3_dir/lib"; +use lib "$main::ww3_dir/t/lib"; use Test::More; use Test::Exception; @@ -19,7 +20,7 @@ use Clone qw/clone/; use YAML::XS qw/LoadFile/; use DB::Schema; -use DB::TestUtils qw/loadCSV removeIDs/; +use TestUtils qw/loadCSV removeIDs/; use DB::Utils qw/updateAllFields/; # Load the database diff --git a/t/db/010_user_problems.t b/t/db/010_user_problems.t index b47c0f61..3d533040 100644 --- a/t/db/010_user_problems.t +++ b/t/db/010_user_problems.t @@ -12,6 +12,7 @@ BEGIN { } use lib "$main::ww3_dir/lib"; +use lib "$main::ww3_dir/t/lib"; use Test::More; use Test::Exception; @@ -22,7 +23,7 @@ use List::MoreUtils qw/firstval/; use YAML::XS qw/LoadFile/; use DB::Schema; -use DB::TestUtils qw/loadCSV removeIDs loadSchema/; +use TestUtils qw/loadCSV removeIDs loadSchema/; use DB::Utils qw/updateAllFields/; # Set up the database. diff --git a/t/db/011_attempts.t b/t/db/011_attempts.t index 00bb2d84..7b6ce9a0 100644 --- a/t/db/011_attempts.t +++ b/t/db/011_attempts.t @@ -12,6 +12,7 @@ BEGIN { } use lib "$main::ww3_dir/lib"; +use lib "$main::ww3_dir/t/lib"; use Test::More; use Test::Exception; @@ -22,7 +23,7 @@ use Try::Tiny; use YAML::XS qw/LoadFile/; use DB::Schema; -use DB::TestUtils qw/loadCSV removeIDs loadSchema/; +use TestUtils qw/loadCSV removeIDs loadSchema/; use DB::Utils qw/updateAllFields/; # Set up the database. diff --git a/t/db/012_set_versions.t b/t/db/012_set_versions.t index 99b94077..f480c3ee 100644 --- a/t/db/012_set_versions.t +++ b/t/db/012_set_versions.t @@ -12,6 +12,7 @@ BEGIN { } use lib "$main::ww3_dir/lib"; +use lib "$main::ww3_dir/t/lib"; use Test::More; use Test::Exception; @@ -20,7 +21,7 @@ use List::MoreUtils qw/firstval/; use Clone qw/clone/; use DB::Schema; -use DB::TestUtils qw/loadCSV removeIDs/; +use TestUtils qw/loadCSV removeIDs/; # Load the database my $config_file = "$main::ww3_dir/conf/ww3-dev.yml"; diff --git a/t/db/013_problem_versions.t b/t/db/013_problem_versions.t index 43b9bf01..5feeeab9 100644 --- a/t/db/013_problem_versions.t +++ b/t/db/013_problem_versions.t @@ -12,6 +12,7 @@ BEGIN { } use lib "$main::ww3_dir/lib"; +use lib "$main::ww3_dir/t/lib"; use Test::More; use Test::Exception; @@ -20,7 +21,7 @@ use List::MoreUtils qw/firstval/; use Clone qw/clone/; use DB::Schema; -use DB::TestUtils qw/loadCSV removeIDs/; +use TestUtils qw/loadCSV removeIDs/; # Load the database my $config_file = "$main::ww3_dir/conf/ww3-dev.yml"; diff --git a/t/db/build_db.pl b/t/db/build_db.pl index 06ea4450..6c56d4b3 100755 --- a/t/db/build_db.pl +++ b/t/db/build_db.pl @@ -22,7 +22,7 @@ BEGIN use Mojo::JSON qw/true false/; use DB::Schema; -use DB::TestUtils qw/loadCSV/; +use TestUtils qw/loadCSV/; my $verbose = 1; diff --git a/t/db/test_course_user.pl b/t/db/test_course_user.pl index 9f1bb177..bd9c2fd6 100755 --- a/t/db/test_course_user.pl +++ b/t/db/test_course_user.pl @@ -21,7 +21,7 @@ BEGIN use DB::WithParams; use DB::WithDates; use DB::Schema; -use DB::TestUtils qw/loadCSV removeIDs/; +use TestUtils qw/loadCSV removeIDs/; # load the database my $db_file = "$main::test_dir/sample_db.sqlite"; diff --git a/lib/DB/TestUtils.pm b/t/lib/TestUtils.pm similarity index 99% rename from lib/DB/TestUtils.pm rename to t/lib/TestUtils.pm index 4dd6b11b..56678b17 100644 --- a/lib/DB/TestUtils.pm +++ b/t/lib/TestUtils.pm @@ -1,4 +1,4 @@ -package DB::TestUtils; +package TestUtils; use warnings; use strict; diff --git a/t/mojolicious/005_problem_sets.t b/t/mojolicious/005_problem_sets.t index 99336ba6..92fe410e 100644 --- a/t/mojolicious/005_problem_sets.t +++ b/t/mojolicious/005_problem_sets.t @@ -15,11 +15,12 @@ BEGIN { } use lib "$main::ww3_dir/lib"; +use lib "$main::ww3_dir/t/lib"; use DB::Schema; use Clone qw/clone/; use YAML::XS qw/LoadFile/; -use DB::TestUtils qw/loadCSV/; +use TestUtils qw/loadCSV/; # Test the api with common 'courses/sets' routes. diff --git a/t/mojolicious/006_quizzes.t b/t/mojolicious/006_quizzes.t index 660d3990..ac25401c 100644 --- a/t/mojolicious/006_quizzes.t +++ b/t/mojolicious/006_quizzes.t @@ -15,11 +15,12 @@ BEGIN { } use lib "$main::ww3_dir/lib"; +use lib "$main::ww3_dir/t/lib"; use DB::Schema; use Clone qw/clone/; use YAML::XS qw/LoadFile/; -use DB::TestUtils qw/loadCSV/; +use TestUtils qw/loadCSV/; use List::MoreUtils qw/firstval/; # Test the api with common 'courses/sets' routes for quizzes. diff --git a/t/mojolicious/007_review_sets.t b/t/mojolicious/007_review_sets.t index f3994bec..e890c06d 100644 --- a/t/mojolicious/007_review_sets.t +++ b/t/mojolicious/007_review_sets.t @@ -13,13 +13,14 @@ BEGIN { } use lib "$main::ww3_dir/lib"; +use lib "$main::ww3_dir/t/lib"; use DB::Schema; use Clone qw/clone/; use YAML::XS qw/LoadFile/; use DateTime::Format::Strptime; use List::MoreUtils qw/firstval/; -use DB::TestUtils qw/loadCSV/; +use TestUtils qw/loadCSV/; my $strp = DateTime::Format::Strptime->new(pattern => '%FT%T', on_error => 'croak'); # Test the api with common "users" routes. From a365b0aece5bf28c3657257f3d5bb561985d17c1 Mon Sep 17 00:00:00 2001 From: Peter Staab Date: Fri, 22 Jul 2022 14:11:28 -0400 Subject: [PATCH 34/34] FIX: updated lib in build_db.pl script. --- t/db/build_db.pl | 1 + 1 file changed, 1 insertion(+) diff --git a/t/db/build_db.pl b/t/db/build_db.pl index 6c56d4b3..816c66dd 100755 --- a/t/db/build_db.pl +++ b/t/db/build_db.pl @@ -12,6 +12,7 @@ BEGIN } use lib "$main::ww3_dir/lib"; +use lib "$main::ww3_dir/t/lib"; use Carp; use feature 'say';