From bef7c8a5755cc43e8ba314ba024647f94f5bd1c6 Mon Sep 17 00:00:00 2001 From: Thomas Kranitsas Date: Tue, 12 Apr 2022 13:33:47 +0300 Subject: [PATCH] Revert "Issue Reviewer Role" --- README.md | 78 --------------------- app.js | 13 ---- config/default.js | 14 +--- docs/swagger.yaml | 12 +--- local/docker-compose.yml | 8 --- package.json | 7 +- src/common/helper.js | 97 +++----------------------- src/routes/ReviewRoutes.js | 20 ++---- src/routes/ReviewSummationRoutes.js | 18 ++--- src/routes/SubmissionRoutes.js | 33 +++------ src/services/ReviewService.js | 75 +++----------------- src/services/ReviewSummationService.js | 8 +-- src/services/SubmissionService.js | 20 +----- 13 files changed, 55 insertions(+), 348 deletions(-) delete mode 100644 local/docker-compose.yml diff --git a/README.md b/README.md index 95b2a22c..defa7137 100755 --- a/README.md +++ b/README.md @@ -115,84 +115,6 @@ This script will load the data from `scripts/data` directory into ES npm run start ``` -### Local Deployment -0. Make sure to use Node v10+ by command `node -v`. We recommend using [NVM](https://github.com/nvm-sh/nvm) to quickly switch to the right version: - - ```bash - nvm use - ``` - -1. 📦 Install npm dependencies - - ```bash - npm install - ``` - -2. ⚙ Local config - In the `submissions-api` root directory create `.env` file with the next environment variables. Values for **Auth0 config** should be shared with you on the forum.
- ```bash - # AWS related config - AWS_ACCESS_KEY_ID= - AWS_SECRET_ACCESS_KEY= - AWS_REGION= - S3_BUCKET= - ARTIFACT_BUCKET= - - # Auth0 config - AUTH0_URL= - AUTH0_PROXY_SERVER_URL= - TOKEN_CACHE_TIME= - AUTH0_AUDIENCE= - AUTH0_CLIENT_ID= - AUTH0_CLIENT_SECRET= - - # Locally deployed services (via docker-compose) - ES_HOST=localhost:9200 - ``` - - - Values from this file would be automatically used by many `npm` commands. - - ⚠️ Never commit this file or its copy to the repository! - -3. 🚢 Start docker-compose with services which are required to start Topcoder Submissions API locally - - ```bash - npm run services:up - ``` - - `npm run services:down` can be used to shutdown the docker services - - `npm run services:logs` can be used to view the logs from the docker services - -4. ♻ Create tables. - - ```bash - npm run create-tables - ``` -5. ♻ Create ES index. - - ```bash - npm run create-index - ``` - -6. ♻ Init DB, ES - - ```bash - npm run local:init - ``` - - This command will do 2 things: - - Import the data to the database and index it to ElasticSearch - - Note, to migrate the existing data from DynamoDB to ES, run the following script - ``` - npm run db-to-es - ``` - -7. 🚀 Start Topcoder Submissions API - - ```bash - npm run start - ``` - The Topcoder Submissions API will be served on `http://localhost:3000` - - #### Linting JS files ``` diff --git a/app.js b/app.js index 920f7634..19a09284 100755 --- a/app.js +++ b/app.js @@ -127,19 +127,6 @@ _.each(routes, (verbs, url) => { }) } - if (def.blockByIp) { - actions.push((req, res, next) => { - req.authUser.blockIP = _.find(req.authUser, (value, key) => { - return (key.indexOf('blockIP') !== -1) - }) - if (req.authUser.blockIP) { - throw new errors.HttpStatusError(403, 'Access denied') - } else { - next() - } - }) - } - actions.push(method) winston.info(`API : ${verb.toLocaleUpperCase()} ${config.API_VERSION}${url}`) apiRouter[verb](`${config.API_VERSION}${url}`, helper.autoWrapExpress(actions)) diff --git a/config/default.js b/config/default.js index 2b8b17b6..8188fd76 100755 --- a/config/default.js +++ b/config/default.js @@ -1,7 +1,7 @@ /** * Default configuration file */ -require('dotenv').config() + module.exports = { DISABLE_LOGGING: process.env.DISABLE_LOGGING || false, // If true, logging will be disabled LOG_LEVEL: process.env.LOG_LEVEL || 'debug', @@ -42,15 +42,5 @@ module.exports = { AUTH0_PROXY_SERVER_URL: process.env.AUTH0_PROXY_SERVER_URL, FETCH_CREATED_DATE_START: process.env.FETCH_CREATED_DATE_START || '2021-01-01', FETCH_PAGE_SIZE: process.env.FETCH_PAGE_SIZE || 500, - MIGRATE_CHALLENGES: process.env.MIGRATE_CHALLENGES || [], - - V5TOLEGACYSCORECARDMAPPING: { - 'c56a4180-65aa-42ec-a945-5fd21dec0501': 30001363, - 'c56a4180-65aa-42ec-a945-5fd21dec0502': 123456789, - 'c56a4180-65aa-42ec-a945-5fd21dec0503': 30001031, - 'c56a4180-65aa-42ec-a945-5fd21dec0504': 987654321, - 'c56a4180-65aa-42ec-a945-5fd21dec0505': 987123456, - '9ecc88e5-a4ee-44a4-8ec1-70bd98022510': 123789456, - 'd6d31f34-8ee5-4589-ae65-45652fcc01a6': 30000720 - } + MIGRATE_CHALLENGES: process.env.MIGRATE_CHALLENGES || [] } diff --git a/docs/swagger.yaml b/docs/swagger.yaml index 523c9b9b..e1d576f6 100755 --- a/docs/swagger.yaml +++ b/docs/swagger.yaml @@ -1757,9 +1757,7 @@ components: description: The scoreCardId filter of the reviews associated with the submission. required: false schema: - oneOf: - - type: integer - - type: string + type: integer filterSubmissionReviewSubmissionId: in: query name: review.submissionId @@ -1873,9 +1871,7 @@ components: description: The score card id filter for reviews. required: false schema: - oneOf: - - type: integer - - type: string + type: integer filterReviewSubmissionId: in: query name: submissionId @@ -2151,9 +2147,7 @@ components: example: a12bc280-65ab-42ec-a945-5fd21dec1567 description: The review reviewer id. scoreCardId: - oneOf: - - type: integer - - type: string + type: integer description: The review score card id. example: 123456789 submissionId: diff --git a/local/docker-compose.yml b/local/docker-compose.yml deleted file mode 100644 index ccc97dc4..00000000 --- a/local/docker-compose.yml +++ /dev/null @@ -1,8 +0,0 @@ -version: '3' -services: - esearch: - image: "docker.elastic.co/elasticsearch/elasticsearch:6.8.0" - ports: - - "9200:9200" - environment: - - "discovery.type=single-node" diff --git a/package.json b/package.json index 4c64ed5c..83865da5 100755 --- a/package.json +++ b/package.json @@ -19,11 +19,7 @@ "test": "mocha test/unit/*.test.js --require test/unit/prepare.js --exit", "e2e": "mocha test/e2e/*.test.js --require test/e2e/prepare.js --exit", "cov": "nyc --reporter=html --reporter=text mocha test/unit/*.test.js --require test/unit/prepare.js --exit", - "cov-e2e": "nyc --reporter=html --reporter=text mocha test/e2e/*.test.js --require test/e2e/prepare.js --exit", - "services:up": "docker-compose -f ./local/docker-compose.yml up -d", - "services:down": "docker-compose -f ./local/docker-compose.yml down", - "services:logs": "docker-compose -f ./local/docker-compose.yml logs", - "local:init": "npm run init-db && npm run init-es" + "cov-e2e": "nyc --reporter=html --reporter=text mocha test/e2e/*.test.js --require test/e2e/prepare.js --exit" }, "dependencies": { "amazon-s3-uri": "0.0.3", @@ -34,7 +30,6 @@ "common-errors": "^1.0.4", "config": "^1.26.2", "cors": "^2.8.4", - "dotenv": "^8.2.0", "elasticsearch": "^15.1.1", "express": "^4.15.4", "express-fileupload": "^0.4.0", diff --git a/src/common/helper.js b/src/common/helper.js index 4f2986e2..f050634c 100755 --- a/src/common/helper.js +++ b/src/common/helper.js @@ -313,10 +313,6 @@ function * getLegacyChallengeId (challengeId) { const response = yield request.get(`${config.CHALLENGEAPI_V5_URL}/${challengeId}`) .set('Authorization', `Bearer ${token}`) .set('Content-Type', 'application/json') - if (_.get(response.body, 'legacy.pureV5')) { - // pure V5 challenges don't have a legacy ID - return null - } const legacyId = parseInt(response.body.legacyId, 10) logger.debug(`Legacy challenge id is ${legacyId} for v5 challenge id ${challengeId}`) return legacyId @@ -379,15 +375,12 @@ function * getSubmissionPhaseId (challengeId) { const checkPoint = _.filter(phases, { name: 'Checkpoint Submission', isOpen: true }) const submissionPh = _.filter(phases, { name: 'Submission', isOpen: true }) const finalFixPh = _.filter(phases, { name: 'Final Fix', isOpen: true }) - const approvalPh = _.filter(phases, { name: 'Approval', isOpen: true }) if (checkPoint.length !== 0) { phaseId = checkPoint[0].phaseId } else if (submissionPh.length !== 0) { phaseId = submissionPh[0].phaseId } else if (finalFixPh.length !== 0) { phaseId = finalFixPh[0].phaseId - } else if (approvalPh.length !== 0) { - phaseId = approvalPh[0].phaseId } } return phaseId @@ -452,18 +445,7 @@ function * checkCreateAccess (authUser, subEntity) { // Get phases and winner detail from challengeDetails const phases = challengeDetails.body.phases - - // Check if the User is assigned as the reviewer for the contest - const reviewers = _.filter(currUserRoles, { role: 'Reviewer' }) - if (reviewers.length !== 0) { - throw new errors.HttpStatusError(400, `You cannot create a submission for a challenge while you are a reviewer`) - } - - // Check if the User is assigned as the iterative reviewer for the contest - const iterativeReviewers = _.filter(currUserRoles, { role: 'Iterative Reviewer' }) - if (iterativeReviewers.length !== 0) { - throw new errors.HttpStatusError(400, `You cannot create a submission for a challenge while you are an iterative reviewer`) - } + const winner = challengeDetails.body.winners // Check if the User is assigned as the reviewer for the contest const reviewers = _.filter(currUserRoles, { role: 'Reviewer' }) @@ -486,22 +468,14 @@ function * checkCreateAccess (authUser, subEntity) { const submissionPhaseId = yield getSubmissionPhaseId(subEntity.challengeId) if (submissionPhaseId == null) { - throw new errors.HttpStatusError(403, 'You cannot create a submission in the current phase') + throw new errors.HttpStatusError(403, 'You are not allowed to submit when submission phase is not open') } const currPhase = _.filter(phases, { phaseId: submissionPhaseId }) - if (currPhase[0].name === 'Final Fix' || currPhase[0].name === 'Approval') { - // Check if the user created a submission in the Submission phase - only such users - // will be allowed to submit during final phase - const userSubmission = yield fetchFromES({ - challengeId, - memberId: authUser.userId - }, camelize('Submission')) - - // User requesting submission haven't made any submission - prevent them for creating one - if (userSubmission.total === 0) { - throw new errors.HttpStatusError(403, 'You are not expected to create a submission in the current phase') + if (currPhase[0].name === 'Final Fix') { + if (!authUser.handle.equals(winner[0].handle)) { + throw new errors.HttpStatusError(403, 'Only winner is allowed to submit during Final Fix phase') } } } else { @@ -605,7 +579,7 @@ function * checkGetAccess (authUser, submission) { const appealsResponseStatus = getPhaseStatus('Appeals Response', challengeDetails.body) // Appeals Response is not closed yet - if (appealsResponseStatus !== 'Closed' && appealsResponseStatus !== 'Invalid') { + if (appealsResponseStatus !== 'Closed') { throw new errors.HttpStatusError(403, 'You cannot access other submissions before the end of Appeals Response phase') } else { const userSubmission = yield fetchFromES({ @@ -641,21 +615,10 @@ function * checkGetAccess (authUser, submission) { * @returns {Promise} */ function * checkReviewGetAccess (authUser, submission) { - let resources let challengeDetails const token = yield getM2Mtoken() const challengeId = yield getV5ChallengeId(submission.challengeId) - try { - resources = yield request.get(`${config.RESOURCEAPI_V5_BASE_URL}/resources?challengeId=${challengeId}`) - .set('Authorization', `Bearer ${token}`) - .set('Content-Type', 'application/json') - } catch (ex) { - logger.error(`Error while accessing ${config.RESOURCEAPI_V5_BASE_URL}/resources?challengeId=${challengeId}`) - logger.error(ex) - throw new errors.HttpStatusError(503, `Could not determine the user's role in the challenge with id ${challengeId}`) - } - try { challengeDetails = yield request.get(`${config.CHALLENGEAPI_V5_URL}/${challengeId}`) .set('Authorization', `Bearer ${token}`) @@ -666,32 +629,9 @@ function * checkReviewGetAccess (authUser, submission) { return false } - // Get map of role id to role name - const resourceRolesMap = yield getRoleIdToRoleNameMap() - - // Check if role id to role name mapping is available. If not user's role cannot be determined. - if (resourceRolesMap == null || _.size(resourceRolesMap) === 0) { - throw new errors.HttpStatusError(503, `Could not determine the user's role in the challenge with id ${challengeId}`) - } - - if (resources && challengeDetails) { - // Fetch all roles of the User pertaining to the current challenge - const currUserRoles = _.filter(resources.body, { memberHandle: authUser.handle }) - - // Populate the role names for the current user role ids - _.forEach(currUserRoles, currentUserRole => { - currentUserRole.role = resourceRolesMap[currentUserRole.roleId] - }) - + if (challengeDetails) { const subTrack = challengeDetails.body.legacy.subTrack - // Check if the User is a Copilot, Manager or Observer for that contest - const validRoles = ['Copilot', 'Manager', 'Observer'] - const passedRoles = currUserRoles.filter(a => validRoles.includes(a.role)) - if (passedRoles.length !== 0) { - return true - } - // For Marathon Match, everyone can access review result if (subTrack === 'DEVELOP_MARATHON_MATCH') { logger.info('No access check for Marathon match') @@ -706,10 +646,6 @@ function * checkReviewGetAccess (authUser, submission) { return true } - } else { - // We don't have enough details to validate the access - logger.debug('No enough details to validate the Permissions') - throw new errors.HttpStatusError(503, `Not all information could be fetched about challenge with id ${submission.challengeId}`) } } @@ -754,7 +690,6 @@ function * postToBusApi (payload) { function cleanseReviews (reviews, authUser) { // Not a machine user if (!authUser.scopes) { - logger.info('Not a machine user. Filtering reviews...') const admin = _.filter(authUser.roles, role => role.toLowerCase() === 'Administrator'.toLowerCase()) const copilot = _.filter(authUser.roles, role => role.toLowerCase() === 'Copilot'.toLowerCase()) @@ -880,21 +815,6 @@ function * getLatestChallenges (page) { } } -/** - * Get legacy scorecard id if the scorecard id is uuid form - * @param {String} scoreCardId Scorecard ID - * @returns {String} Legacy scorecard ID of the given challengeId - */ -function getLegacyScoreCardId (scoreCardId) { - if (/^[0-9a-f]{8}-[0-9a-f]{4}-[0-5][0-9a-f]{3}-[089ab][0-9a-f]{3}-[0-9a-f]{12}$/i.test(scoreCardId)) { - logger.debug(`${scoreCardId} detected as uuid. Converting to legacy scorecard id`) - - return config.get('V5TOLEGACYSCORECARDMAPPING')[scoreCardId] - } - - return scoreCardId -} - module.exports = { wrapExpress, autoWrapExpress, @@ -913,6 +833,5 @@ module.exports = { getRoleIdToRoleNameMap, getV5ChallengeId, adjustSubmissionChallengeId, - getLatestChallenges, - getLegacyScoreCardId + getLatestChallenges } diff --git a/src/routes/ReviewRoutes.js b/src/routes/ReviewRoutes.js index a027beb6..4e7d4975 100644 --- a/src/routes/ReviewRoutes.js +++ b/src/routes/ReviewRoutes.js @@ -9,16 +9,14 @@ module.exports = { method: 'createReview', auth: 'jwt', access: ['Administrator', 'Copilot'], - scopes: ['create:review', 'all:review'], - blockByIp: true + scopes: ['create:review', 'all:review'] }, get: { controller: 'ReviewController', method: 'listReviews', auth: 'jwt', access: ['Administrator', 'Copilot'], - scopes: ['read:review', 'all:review'], - blockByIp: true + scopes: ['read:review', 'all:review'] } }, '/reviews/:reviewId': { @@ -26,33 +24,29 @@ module.exports = { controller: 'ReviewController', method: 'getReview', auth: 'jwt', - access: ['Administrator', 'Copilot', 'Topcoder User'], - scopes: ['read:review', 'all:review'], - blockByIp: true + access: ['Administrator', 'Copilot'], + scopes: ['read:review', 'all:review'] }, put: { controller: 'ReviewController', method: 'updateReview', auth: 'jwt', access: ['Administrator'], - scopes: ['update:review', 'all:review'], - blockByIp: true + scopes: ['update:review', 'all:review'] }, patch: { controller: 'ReviewController', method: 'patchReview', auth: 'jwt', access: ['Administrator'], - scopes: ['update:review', 'all:review'], - blockByIp: true + scopes: ['update:review', 'all:review'] }, delete: { controller: 'ReviewController', method: 'deleteReview', auth: 'jwt', access: ['Administrator'], - scopes: ['delete:review', 'all:review'], - blockByIp: true + scopes: ['delete:review', 'all:review'] } } } diff --git a/src/routes/ReviewSummationRoutes.js b/src/routes/ReviewSummationRoutes.js index a5144821..2e1cd558 100644 --- a/src/routes/ReviewSummationRoutes.js +++ b/src/routes/ReviewSummationRoutes.js @@ -9,16 +9,14 @@ module.exports = { method: 'createReviewSummation', auth: 'jwt', access: ['Administrator', 'Copilot'], - scopes: ['create:review_summation', 'all:review_summation'], - blockByIp: true + scopes: ['create:review_summation', 'all:review_summation'] }, get: { controller: 'ReviewSummationController', method: 'listReviewSummations', auth: 'jwt', access: ['Administrator', 'Copilot'], - scopes: ['read:review_summation', 'all:review_summation'], - blockByIp: true + scopes: ['read:review_summation', 'all:review_summation'] } }, '/reviewSummations/:reviewSummationId': { @@ -27,32 +25,28 @@ module.exports = { method: 'getReviewSummation', auth: 'jwt', access: ['Administrator', 'Copilot'], - scopes: ['read:review_summation', 'all:review_summation'], - blockByIp: true + scopes: ['read:review_summation', 'all:review_summation'] }, put: { controller: 'ReviewSummationController', method: 'updateReviewSummation', auth: 'jwt', access: ['Administrator'], - scopes: ['update:review_summation', 'all:review_summation'], - blockByIp: true + scopes: ['update:review_summation', 'all:review_summation'] }, patch: { controller: 'ReviewSummationController', method: 'patchReviewSummation', auth: 'jwt', access: ['Administrator'], - scopes: ['update:review_summation', 'all:review_summation'], - blockByIp: true + scopes: ['update:review_summation', 'all:review_summation'] }, delete: { controller: 'ReviewSummationController', method: 'deleteReviewSummation', auth: 'jwt', access: ['Administrator'], - scopes: ['delete:review_summation', 'all:review_summation'], - blockByIp: true + scopes: ['delete:review_summation', 'all:review_summation'] } } } diff --git a/src/routes/SubmissionRoutes.js b/src/routes/SubmissionRoutes.js index 7144c9fe..7593f5fd 100755 --- a/src/routes/SubmissionRoutes.js +++ b/src/routes/SubmissionRoutes.js @@ -9,16 +9,14 @@ module.exports = { method: 'createSubmission', auth: 'jwt', access: ['Topcoder User', 'Administrator', 'Copilot'], - scopes: ['create:submission', 'all:submission'], - blockByIp: true + scopes: ['create:submission', 'all:submission'] }, get: { controller: 'SubmissionController', method: 'listSubmissions', auth: 'jwt', access: ['Topcoder User', 'Administrator', 'Copilot'], - scopes: ['read:submission', 'all:submission'], - blockByIp: true + scopes: ['read:submission', 'all:submission'] } }, '/submissions/:submissionId': { @@ -27,32 +25,28 @@ module.exports = { method: 'getSubmission', auth: 'jwt', access: ['Topcoder User', 'Administrator', 'Copilot'], - scopes: ['read:submission', 'all:submission'], - blockByIp: true + scopes: ['read:submission', 'all:submission'] }, put: { controller: 'SubmissionController', method: 'updateSubmission', auth: 'jwt', access: ['Administrator'], - scopes: ['update:submission', 'all:submission'], - blockByIp: true + scopes: ['update:submission', 'all:submission'] }, patch: { controller: 'SubmissionController', method: 'patchSubmission', auth: 'jwt', access: ['Administrator'], - scopes: ['update:submission', 'all:submission'], - blockByIp: true + scopes: ['update:submission', 'all:submission'] }, delete: { controller: 'SubmissionController', method: 'deleteSubmission', auth: 'jwt', access: ['Administrator'], - scopes: ['delete:submission', 'all:submission'], - blockByIp: true + scopes: ['delete:submission', 'all:submission'] } }, '/submissions/:submissionId/download': { @@ -61,8 +55,7 @@ module.exports = { method: 'downloadSubmission', auth: 'jwt', access: ['Topcoder User', 'Administrator', 'Copilot'], - scopes: ['read:submission', 'all:submission'], - blockByIp: true + scopes: ['read:submission', 'all:submission'] } }, '/submissions/:submissionId/artifacts': { @@ -71,16 +64,14 @@ module.exports = { method: 'createArtifact', auth: 'jwt', access: ['Topcoder User', 'Administrator', 'Copilot'], - scopes: ['create:submission', 'all:submission'], - blockByIp: true + scopes: ['create:submission', 'all:submission'] }, get: { controller: 'ArtifactController', method: 'listArtifacts', auth: 'jwt', access: ['Topcoder User', 'Administrator', 'Copilot'], - scopes: ['read:submission', 'all:submission'], - blockByIp: true + scopes: ['read:submission', 'all:submission'] } }, '/submissions/:submissionId/artifacts/:file': { @@ -89,8 +80,7 @@ module.exports = { method: 'deleteArtifact', auth: 'jwt', access: ['Administrator'], - scopes: ['delete:submission', 'all:submission'], - blockByIp: true + scopes: ['delete:submission', 'all:submission'] } }, '/submissions/:submissionId/artifacts/:file/download': { @@ -99,8 +89,7 @@ module.exports = { method: 'downloadArtifact', auth: 'jwt', access: ['Topcoder User', 'Administrator', 'Copilot'], - scopes: ['read:submission', 'all:submission'], - blockByIp: true + scopes: ['read:submission', 'all:submission'] } } } diff --git a/src/services/ReviewService.js b/src/services/ReviewService.js index dfc54414..ffd565d0 100644 --- a/src/services/ReviewService.js +++ b/src/services/ReviewService.js @@ -70,9 +70,7 @@ function * getReview (authUser, reviewId) { false ) logger.info('Check User access before returning the review') - if (_.intersection(authUser.roles, ['Administrator', 'administrator']).length === 0 && !authUser.scopes) { - yield helper.checkReviewGetAccess(authUser, submission) - } + yield helper.checkReviewGetAccess(authUser, submission) // Return the review return review } @@ -91,15 +89,6 @@ getReview.schema = { * @return {Object} Data fetched from ES */ function * listReviews (query) { - if (query.scoreCardId) { - // Always use legacy scorecard id since v5 isn't stored in db - query.scoreCardId = helper.getLegacyScoreCardId(query.scoreCardId) - - if (!query.scoreCardId) { - throw new errors.HttpStatusError(400, 'Legacy scorecard id not found for the provided v5 scorecard id') - } - } - return yield helper.fetchFromES(query, helper.camelize(table)) } @@ -107,7 +96,7 @@ const listReviewsQuerySchema = { score: joi.score(), typeId: joi.string().uuid(), reviewerId: joi.alternatives().try(joi.id(), joi.string().uuid()), - scoreCardId: joi.alternatives().try(joi.id(), joi.string().uuid()), + scoreCardId: joi.id(), submissionId: joi.string().uuid(), status: joi.reviewStatus(), page: joi.id(), @@ -143,20 +132,6 @@ function * createReview (authUser, entity) { const currDate = new Date().toISOString() - const possibleV5ScoreCardId = entity.scoreCardId - - // Always use legacy id instead of v5 legacy id - entity.scoreCardId = helper.getLegacyScoreCardId(entity.scoreCardId) - - if (!entity.scoreCardId) { - throw new errors.HttpStatusError(400, 'Legacy scorecard id not found for the provided v5 scorecard id') - } - - if (entity.scoreCardId !== possibleV5ScoreCardId) { - // Remember the v5 score card id too that was passed - entity.v5ScoreCardId = possibleV5ScoreCardId - } - const item = _.extend( { id: uuid(), @@ -229,7 +204,7 @@ createReview.schema = { .error(errors => ({ message: '"reviewerId" must be a number or a string' })), - scoreCardId: joi.alternatives().try(joi.id().required(), joi.string().uuid().required()), + scoreCardId: joi.id().required(), submissionId: joi .string() .uuid() @@ -263,28 +238,6 @@ function * _updateReview (authUser, reviewId, entity) { const currDate = new Date().toISOString() - let scoreCardId = exist.scoreCardId - let v5ScoreCardId = exist.v5ScoreCardId - - if (entity.scoreCardId) { - scoreCardId = helper.getLegacyScoreCardId(entity.scoreCardId) - - if (!scoreCardId) { - throw new errors.HttpStatusError(400, 'Legacy scorecard id not found for the provided v5 scorecard id') - } - - if (entity.scoreCardId !== scoreCardId) { - v5ScoreCardId = entity.scoreCardId - } else if (v5ScoreCardId) { - // Since we currently have a static mapping b/w legacy and v5 scorecard ids, - // there is no way for us to fetch the v5 scorecard id for a legacy scorecard id - // In this scenario, the review already had a v5 scorecard id, so if it's legacy - // scorecard id gets updated, we would also need to update the v5 scorecard id - // which we cannot - hence the error. Ideally, in this case, a new review needs to be created - throw new errors.HttpStatusError(400, `Cannot update legacy scorecard id since review with id ${reviewId} already has a v5 scorecard id`) - } - } - // Record used for updating in Database const record = { TableName: table, @@ -293,19 +246,17 @@ function * _updateReview (authUser, reviewId, entity) { }, UpdateExpression: `set score = :s, scoreCardId = :sc, submissionId = :su, typeId = :t, reviewerId = :r, #st = :st, - updated = :ua, updatedBy = :ub, reviewedDate = :rd - ${v5ScoreCardId ? ', v5ScoreCardId = :v5s' : ''}`, + updated = :ua, updatedBy = :ub, reviewedDate = :rd`, ExpressionAttributeValues: { ':s': entity.score || exist.score, - ':sc': scoreCardId, + ':sc': entity.scoreCardId || exist.scoreCardId, ':su': entity.submissionId || exist.submissionId, ':t': entity.typeId || exist.typeId, ':r': entity.reviewerId || exist.reviewerId, ':st': entity.status || exist.status || 'completed', ':ua': currDate, ':ub': authUser.handle || authUser.sub, - ':rd': entity.reviewedDate || exist.reviewedDate || exist.created, - ...(v5ScoreCardId ? { ':v5s': v5ScoreCardId } : {}) + ':rd': entity.reviewedDate || exist.reviewedDate || exist.created }, ExpressionAttributeNames: { '#st': 'status' @@ -340,11 +291,7 @@ function * _updateReview (authUser, reviewId, entity) { updatedBy: authUser.handle || authUser.sub, reviewedDate: entity.reviewedDate || exist.reviewedDate || exist.created }, - entity, - { - scoreCardId, - v5ScoreCardId - } + entity ) } @@ -356,9 +303,7 @@ function * _updateReview (authUser, reviewId, entity) { return _.extend(exist, entity, { updated: currDate, updatedBy: authUser.handle || authUser.sub, - reviewedDate: entity.reviewedDate || exist.reviewedDate || exist.created, - scoreCardId, - v5ScoreCardId + reviewedDate: entity.reviewedDate || exist.reviewedDate || exist.created }) } @@ -391,7 +336,7 @@ updateReview.schema = { .alternatives() .try(joi.id(), joi.string().uuid()) .required(), - scoreCardId: joi.alternatives().try(joi.id().required(), joi.string().uuid().required()), + scoreCardId: joi.id().required(), submissionId: joi .string() .uuid() @@ -424,7 +369,7 @@ patchReview.schema = { score: joi.score(), typeId: joi.string().uuid(), reviewerId: joi.alternatives().try(joi.id(), joi.string().uuid()), - scoreCardId: joi.alternatives().try(joi.id(), joi.string().uuid()), + scoreCardId: joi.id(), submissionId: joi.string().uuid(), status: joi.reviewStatus(), metadata: joi.object(), diff --git a/src/services/ReviewSummationService.js b/src/services/ReviewSummationService.js index c14c0bf3..69af4aa2 100644 --- a/src/services/ReviewSummationService.js +++ b/src/services/ReviewSummationService.js @@ -59,7 +59,7 @@ function * listReviewSummations (query) { } const listReviewSummationsQuerySchema = { - scoreCardId: joi.alternatives().try(joi.id(), joi.string().uuid()), + scoreCardId: joi.id(), submissionId: joi.string().uuid(), aggregateScore: joi.score(), isPassing: joi.boolean(), @@ -138,7 +138,7 @@ function * createReviewSummation (authUser, entity) { createReviewSummation.schema = { authUser: joi.object().required(), entity: joi.object().keys({ - scoreCardId: joi.alternatives().try(joi.id().required(), joi.string().uuid().required()), + scoreCardId: joi.id().required(), submissionId: joi.string().uuid().required(), aggregateScore: joi.score().required(), isPassing: joi.boolean().required(), @@ -263,7 +263,7 @@ updateReviewSummation.schema = { authUser: joi.object().required(), reviewSummationId: joi.string().uuid().required(), entity: joi.object().keys({ - scoreCardId: joi.alternatives().try(joi.id().required(), joi.string().uuid().required()), + scoreCardId: joi.id().required(), submissionId: joi.string().uuid().required(), aggregateScore: joi.score().required(), isPassing: joi.boolean().required(), @@ -288,7 +288,7 @@ patchReviewSummation.schema = { authUser: joi.object().required(), reviewSummationId: joi.string().uuid().required(), entity: joi.object().keys({ - scoreCardId: joi.alternatives().try(joi.id(), joi.string().uuid()), + scoreCardId: joi.id(), submissionId: joi.string().uuid(), aggregateScore: joi.score(), isPassing: joi.boolean(), diff --git a/src/services/SubmissionService.js b/src/services/SubmissionService.js index 1e3e0847..336049e3 100755 --- a/src/services/SubmissionService.js +++ b/src/services/SubmissionService.js @@ -188,17 +188,7 @@ function * listSubmissions (authUser, query) { query.challengeId = yield helper.getV5ChallengeId(query.challengeId) } - // TODO - support v5 for review scorecardid - if (query['review.scoreCardId']) { - query['review.scoreCardId'] = helper.getLegacyScoreCardId(query['review.scoreCardId']) - - if (!query['review.scoreCardId']) { - throw new errors.HttpStatusError(400, 'Legacy scorecard id not found for the provided v5 scorecard id') - } - } - const data = yield helper.fetchFromES(query, helper.camelize(table)) - logger.info(`Data returned from ES: ${JSON.stringify(data, null, 4)}`) logger.info(`listSubmissions: returning ${data.length} submissions for query: ${JSON.stringify(query)}`) data.rows = _.map(data.rows, (submission) => { @@ -225,7 +215,7 @@ const listSubmissionsQuerySchema = { 'review.score': joi.score(), 'review.typeId': joi.string().uuid(), 'review.reviewerId': joi.string().uuid(), - 'review.scoreCardId': joi.alternatives().try(joi.id(), joi.string().uuid()), + 'review.scoreCardId': joi.id(), 'review.submissionId': joi.string().uuid(), 'review.status': joi.reviewStatus(), 'reviewSummation.scoreCardId': joi.id(), @@ -433,15 +423,12 @@ function * _updateSubmission (authUser, submissionId, entity) { id: submissionId }, UpdateExpression: `set #type = :t, #url = :u, memberId = :m, challengeId = :c, - ${legacyChallengeId ? 'legacyChallengeId = :lc,' : ''} updated = :ua, updatedBy = :ub, submittedDate = :sb`, + updated = :ua, updatedBy = :ub, submittedDate = :sb`, ExpressionAttributeValues: { ':t': entity.type || exist.type, ':u': entity.url || exist.url, ':m': entity.memberId || exist.memberId, ':c': challengeId, - ...(legacyChallengeId ? { - ':lc': legacyChallengeId - } : {}), ':ua': currDate, ':ub': authUser.handle || authUser.sub, ':sb': entity.submittedDate || exist.submittedDate || exist.created @@ -496,8 +483,7 @@ function * _updateSubmission (authUser, submissionId, entity) { memberId: updatedSub.memberId, submissionPhaseId: updatedSub.submissionPhaseId, type: updatedSub.type, - submittedDate: updatedSub.submittedDate, - legacyChallengeId: updatedSub.legacyChallengeId + submittedDate: updatedSub.submittedDate }, entity) }