From 4b92bf51bd5de15dce6bc1e7754804999d04045f Mon Sep 17 00:00:00 2001 From: Thomas Kranitsas Date: Tue, 12 Apr 2022 14:26:33 +0300 Subject: [PATCH] Revert "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, 348 insertions(+), 55 deletions(-) create mode 100644 local/docker-compose.yml diff --git a/README.md b/README.md index defa7137..95b2a22c 100755 --- a/README.md +++ b/README.md @@ -115,6 +115,84 @@ 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 19a09284..920f7634 100755 --- a/app.js +++ b/app.js @@ -127,6 +127,19 @@ _.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 8188fd76..2b8b17b6 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,5 +42,15 @@ 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 || [] + 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 + } } diff --git a/docs/swagger.yaml b/docs/swagger.yaml index e1d576f6..523c9b9b 100755 --- a/docs/swagger.yaml +++ b/docs/swagger.yaml @@ -1757,7 +1757,9 @@ components: description: The scoreCardId filter of the reviews associated with the submission. required: false schema: - type: integer + oneOf: + - type: integer + - type: string filterSubmissionReviewSubmissionId: in: query name: review.submissionId @@ -1871,7 +1873,9 @@ components: description: The score card id filter for reviews. required: false schema: - type: integer + oneOf: + - type: integer + - type: string filterReviewSubmissionId: in: query name: submissionId @@ -2147,7 +2151,9 @@ components: example: a12bc280-65ab-42ec-a945-5fd21dec1567 description: The review reviewer id. scoreCardId: - type: integer + oneOf: + - type: integer + - type: string description: The review score card id. example: 123456789 submissionId: diff --git a/local/docker-compose.yml b/local/docker-compose.yml new file mode 100644 index 00000000..ccc97dc4 --- /dev/null +++ b/local/docker-compose.yml @@ -0,0 +1,8 @@ +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 83865da5..4c64ed5c 100755 --- a/package.json +++ b/package.json @@ -19,7 +19,11 @@ "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" + "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" }, "dependencies": { "amazon-s3-uri": "0.0.3", @@ -30,6 +34,7 @@ "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 f050634c..4f2986e2 100755 --- a/src/common/helper.js +++ b/src/common/helper.js @@ -313,6 +313,10 @@ 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 @@ -375,12 +379,15 @@ 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 @@ -445,7 +452,18 @@ function * checkCreateAccess (authUser, subEntity) { // Get phases and winner detail from challengeDetails const phases = challengeDetails.body.phases - const winner = challengeDetails.body.winners + + // 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`) + } // Check if the User is assigned as the reviewer for the contest const reviewers = _.filter(currUserRoles, { role: 'Reviewer' }) @@ -468,14 +486,22 @@ function * checkCreateAccess (authUser, subEntity) { const submissionPhaseId = yield getSubmissionPhaseId(subEntity.challengeId) if (submissionPhaseId == null) { - throw new errors.HttpStatusError(403, 'You are not allowed to submit when submission phase is not open') + throw new errors.HttpStatusError(403, 'You cannot create a submission in the current phase') } const currPhase = _.filter(phases, { phaseId: submissionPhaseId }) - 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') + 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') } } } else { @@ -579,7 +605,7 @@ function * checkGetAccess (authUser, submission) { const appealsResponseStatus = getPhaseStatus('Appeals Response', challengeDetails.body) // Appeals Response is not closed yet - if (appealsResponseStatus !== 'Closed') { + if (appealsResponseStatus !== 'Closed' && appealsResponseStatus !== 'Invalid') { throw new errors.HttpStatusError(403, 'You cannot access other submissions before the end of Appeals Response phase') } else { const userSubmission = yield fetchFromES({ @@ -615,10 +641,21 @@ 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}`) @@ -629,9 +666,32 @@ function * checkReviewGetAccess (authUser, submission) { return false } - if (challengeDetails) { + // 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] + }) + 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') @@ -646,6 +706,10 @@ 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}`) } } @@ -690,6 +754,7 @@ 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()) @@ -815,6 +880,21 @@ 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, @@ -833,5 +913,6 @@ module.exports = { getRoleIdToRoleNameMap, getV5ChallengeId, adjustSubmissionChallengeId, - getLatestChallenges + getLatestChallenges, + getLegacyScoreCardId } diff --git a/src/routes/ReviewRoutes.js b/src/routes/ReviewRoutes.js index 4e7d4975..a027beb6 100644 --- a/src/routes/ReviewRoutes.js +++ b/src/routes/ReviewRoutes.js @@ -9,14 +9,16 @@ module.exports = { method: 'createReview', auth: 'jwt', access: ['Administrator', 'Copilot'], - scopes: ['create:review', 'all:review'] + scopes: ['create:review', 'all:review'], + blockByIp: true }, get: { controller: 'ReviewController', method: 'listReviews', auth: 'jwt', access: ['Administrator', 'Copilot'], - scopes: ['read:review', 'all:review'] + scopes: ['read:review', 'all:review'], + blockByIp: true } }, '/reviews/:reviewId': { @@ -24,29 +26,33 @@ module.exports = { controller: 'ReviewController', method: 'getReview', auth: 'jwt', - access: ['Administrator', 'Copilot'], - scopes: ['read:review', 'all:review'] + access: ['Administrator', 'Copilot', 'Topcoder User'], + scopes: ['read:review', 'all:review'], + blockByIp: true }, put: { controller: 'ReviewController', method: 'updateReview', auth: 'jwt', access: ['Administrator'], - scopes: ['update:review', 'all:review'] + scopes: ['update:review', 'all:review'], + blockByIp: true }, patch: { controller: 'ReviewController', method: 'patchReview', auth: 'jwt', access: ['Administrator'], - scopes: ['update:review', 'all:review'] + scopes: ['update:review', 'all:review'], + blockByIp: true }, delete: { controller: 'ReviewController', method: 'deleteReview', auth: 'jwt', access: ['Administrator'], - scopes: ['delete:review', 'all:review'] + scopes: ['delete:review', 'all:review'], + blockByIp: true } } } diff --git a/src/routes/ReviewSummationRoutes.js b/src/routes/ReviewSummationRoutes.js index 2e1cd558..a5144821 100644 --- a/src/routes/ReviewSummationRoutes.js +++ b/src/routes/ReviewSummationRoutes.js @@ -9,14 +9,16 @@ module.exports = { method: 'createReviewSummation', auth: 'jwt', access: ['Administrator', 'Copilot'], - scopes: ['create:review_summation', 'all:review_summation'] + scopes: ['create:review_summation', 'all:review_summation'], + blockByIp: true }, get: { controller: 'ReviewSummationController', method: 'listReviewSummations', auth: 'jwt', access: ['Administrator', 'Copilot'], - scopes: ['read:review_summation', 'all:review_summation'] + scopes: ['read:review_summation', 'all:review_summation'], + blockByIp: true } }, '/reviewSummations/:reviewSummationId': { @@ -25,28 +27,32 @@ module.exports = { method: 'getReviewSummation', auth: 'jwt', access: ['Administrator', 'Copilot'], - scopes: ['read:review_summation', 'all:review_summation'] + scopes: ['read:review_summation', 'all:review_summation'], + blockByIp: true }, put: { controller: 'ReviewSummationController', method: 'updateReviewSummation', auth: 'jwt', access: ['Administrator'], - scopes: ['update:review_summation', 'all:review_summation'] + scopes: ['update:review_summation', 'all:review_summation'], + blockByIp: true }, patch: { controller: 'ReviewSummationController', method: 'patchReviewSummation', auth: 'jwt', access: ['Administrator'], - scopes: ['update:review_summation', 'all:review_summation'] + scopes: ['update:review_summation', 'all:review_summation'], + blockByIp: true }, delete: { controller: 'ReviewSummationController', method: 'deleteReviewSummation', auth: 'jwt', access: ['Administrator'], - scopes: ['delete:review_summation', 'all:review_summation'] + scopes: ['delete:review_summation', 'all:review_summation'], + blockByIp: true } } } diff --git a/src/routes/SubmissionRoutes.js b/src/routes/SubmissionRoutes.js index 7593f5fd..7144c9fe 100755 --- a/src/routes/SubmissionRoutes.js +++ b/src/routes/SubmissionRoutes.js @@ -9,14 +9,16 @@ module.exports = { method: 'createSubmission', auth: 'jwt', access: ['Topcoder User', 'Administrator', 'Copilot'], - scopes: ['create:submission', 'all:submission'] + scopes: ['create:submission', 'all:submission'], + blockByIp: true }, get: { controller: 'SubmissionController', method: 'listSubmissions', auth: 'jwt', access: ['Topcoder User', 'Administrator', 'Copilot'], - scopes: ['read:submission', 'all:submission'] + scopes: ['read:submission', 'all:submission'], + blockByIp: true } }, '/submissions/:submissionId': { @@ -25,28 +27,32 @@ module.exports = { method: 'getSubmission', auth: 'jwt', access: ['Topcoder User', 'Administrator', 'Copilot'], - scopes: ['read:submission', 'all:submission'] + scopes: ['read:submission', 'all:submission'], + blockByIp: true }, put: { controller: 'SubmissionController', method: 'updateSubmission', auth: 'jwt', access: ['Administrator'], - scopes: ['update:submission', 'all:submission'] + scopes: ['update:submission', 'all:submission'], + blockByIp: true }, patch: { controller: 'SubmissionController', method: 'patchSubmission', auth: 'jwt', access: ['Administrator'], - scopes: ['update:submission', 'all:submission'] + scopes: ['update:submission', 'all:submission'], + blockByIp: true }, delete: { controller: 'SubmissionController', method: 'deleteSubmission', auth: 'jwt', access: ['Administrator'], - scopes: ['delete:submission', 'all:submission'] + scopes: ['delete:submission', 'all:submission'], + blockByIp: true } }, '/submissions/:submissionId/download': { @@ -55,7 +61,8 @@ module.exports = { method: 'downloadSubmission', auth: 'jwt', access: ['Topcoder User', 'Administrator', 'Copilot'], - scopes: ['read:submission', 'all:submission'] + scopes: ['read:submission', 'all:submission'], + blockByIp: true } }, '/submissions/:submissionId/artifacts': { @@ -64,14 +71,16 @@ module.exports = { method: 'createArtifact', auth: 'jwt', access: ['Topcoder User', 'Administrator', 'Copilot'], - scopes: ['create:submission', 'all:submission'] + scopes: ['create:submission', 'all:submission'], + blockByIp: true }, get: { controller: 'ArtifactController', method: 'listArtifacts', auth: 'jwt', access: ['Topcoder User', 'Administrator', 'Copilot'], - scopes: ['read:submission', 'all:submission'] + scopes: ['read:submission', 'all:submission'], + blockByIp: true } }, '/submissions/:submissionId/artifacts/:file': { @@ -80,7 +89,8 @@ module.exports = { method: 'deleteArtifact', auth: 'jwt', access: ['Administrator'], - scopes: ['delete:submission', 'all:submission'] + scopes: ['delete:submission', 'all:submission'], + blockByIp: true } }, '/submissions/:submissionId/artifacts/:file/download': { @@ -89,7 +99,8 @@ module.exports = { method: 'downloadArtifact', auth: 'jwt', access: ['Topcoder User', 'Administrator', 'Copilot'], - scopes: ['read:submission', 'all:submission'] + scopes: ['read:submission', 'all:submission'], + blockByIp: true } } } diff --git a/src/services/ReviewService.js b/src/services/ReviewService.js index ffd565d0..dfc54414 100644 --- a/src/services/ReviewService.js +++ b/src/services/ReviewService.js @@ -70,7 +70,9 @@ function * getReview (authUser, reviewId) { false ) logger.info('Check User access before returning the review') - yield helper.checkReviewGetAccess(authUser, submission) + if (_.intersection(authUser.roles, ['Administrator', 'administrator']).length === 0 && !authUser.scopes) { + yield helper.checkReviewGetAccess(authUser, submission) + } // Return the review return review } @@ -89,6 +91,15 @@ 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)) } @@ -96,7 +107,7 @@ const listReviewsQuerySchema = { score: joi.score(), typeId: joi.string().uuid(), reviewerId: joi.alternatives().try(joi.id(), joi.string().uuid()), - scoreCardId: joi.id(), + scoreCardId: joi.alternatives().try(joi.id(), joi.string().uuid()), submissionId: joi.string().uuid(), status: joi.reviewStatus(), page: joi.id(), @@ -132,6 +143,20 @@ 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(), @@ -204,7 +229,7 @@ createReview.schema = { .error(errors => ({ message: '"reviewerId" must be a number or a string' })), - scoreCardId: joi.id().required(), + scoreCardId: joi.alternatives().try(joi.id().required(), joi.string().uuid().required()), submissionId: joi .string() .uuid() @@ -238,6 +263,28 @@ 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, @@ -246,17 +293,19 @@ 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`, + updated = :ua, updatedBy = :ub, reviewedDate = :rd + ${v5ScoreCardId ? ', v5ScoreCardId = :v5s' : ''}`, ExpressionAttributeValues: { ':s': entity.score || exist.score, - ':sc': entity.scoreCardId || exist.scoreCardId, + ':sc': 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 + ':rd': entity.reviewedDate || exist.reviewedDate || exist.created, + ...(v5ScoreCardId ? { ':v5s': v5ScoreCardId } : {}) }, ExpressionAttributeNames: { '#st': 'status' @@ -291,7 +340,11 @@ function * _updateReview (authUser, reviewId, entity) { updatedBy: authUser.handle || authUser.sub, reviewedDate: entity.reviewedDate || exist.reviewedDate || exist.created }, - entity + entity, + { + scoreCardId, + v5ScoreCardId + } ) } @@ -303,7 +356,9 @@ function * _updateReview (authUser, reviewId, entity) { return _.extend(exist, entity, { updated: currDate, updatedBy: authUser.handle || authUser.sub, - reviewedDate: entity.reviewedDate || exist.reviewedDate || exist.created + reviewedDate: entity.reviewedDate || exist.reviewedDate || exist.created, + scoreCardId, + v5ScoreCardId }) } @@ -336,7 +391,7 @@ updateReview.schema = { .alternatives() .try(joi.id(), joi.string().uuid()) .required(), - scoreCardId: joi.id().required(), + scoreCardId: joi.alternatives().try(joi.id().required(), joi.string().uuid().required()), submissionId: joi .string() .uuid() @@ -369,7 +424,7 @@ patchReview.schema = { score: joi.score(), typeId: joi.string().uuid(), reviewerId: joi.alternatives().try(joi.id(), joi.string().uuid()), - scoreCardId: joi.id(), + scoreCardId: joi.alternatives().try(joi.id(), joi.string().uuid()), submissionId: joi.string().uuid(), status: joi.reviewStatus(), metadata: joi.object(), diff --git a/src/services/ReviewSummationService.js b/src/services/ReviewSummationService.js index 69af4aa2..c14c0bf3 100644 --- a/src/services/ReviewSummationService.js +++ b/src/services/ReviewSummationService.js @@ -59,7 +59,7 @@ function * listReviewSummations (query) { } const listReviewSummationsQuerySchema = { - scoreCardId: joi.id(), + scoreCardId: joi.alternatives().try(joi.id(), joi.string().uuid()), 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.id().required(), + scoreCardId: joi.alternatives().try(joi.id().required(), joi.string().uuid().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.id().required(), + scoreCardId: joi.alternatives().try(joi.id().required(), joi.string().uuid().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.id(), + scoreCardId: joi.alternatives().try(joi.id(), joi.string().uuid()), submissionId: joi.string().uuid(), aggregateScore: joi.score(), isPassing: joi.boolean(), diff --git a/src/services/SubmissionService.js b/src/services/SubmissionService.js index 336049e3..1e3e0847 100755 --- a/src/services/SubmissionService.js +++ b/src/services/SubmissionService.js @@ -188,7 +188,17 @@ 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) => { @@ -215,7 +225,7 @@ const listSubmissionsQuerySchema = { 'review.score': joi.score(), 'review.typeId': joi.string().uuid(), 'review.reviewerId': joi.string().uuid(), - 'review.scoreCardId': joi.id(), + 'review.scoreCardId': joi.alternatives().try(joi.id(), joi.string().uuid()), 'review.submissionId': joi.string().uuid(), 'review.status': joi.reviewStatus(), 'reviewSummation.scoreCardId': joi.id(), @@ -423,12 +433,15 @@ function * _updateSubmission (authUser, submissionId, entity) { id: submissionId }, UpdateExpression: `set #type = :t, #url = :u, memberId = :m, challengeId = :c, - updated = :ua, updatedBy = :ub, submittedDate = :sb`, + ${legacyChallengeId ? 'legacyChallengeId = :lc,' : ''} 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 @@ -483,7 +496,8 @@ function * _updateSubmission (authUser, submissionId, entity) { memberId: updatedSub.memberId, submissionPhaseId: updatedSub.submissionPhaseId, type: updatedSub.type, - submittedDate: updatedSub.submittedDate + submittedDate: updatedSub.submittedDate, + legacyChallengeId: updatedSub.legacyChallengeId }, entity) }