Skip to content
This repository was archived by the owner on Mar 13, 2025. It is now read-only.

Add metadata from review summation #3

Merged
merged 1 commit into from
Nov 13, 2018
Merged

Add metadata from review summation #3

merged 1 commit into from
Nov 13, 2018

Conversation

callmekatootie
Copy link
Collaborator

No description provided.

@cwdcwd cwdcwd self-requested a review November 12, 2018 17:21
Copy link
Contributor

@cwdcwd cwdcwd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need a couple tweaks for safety. I'm fine with accepting this as is and then putting those in

function getTestsPassed (metadata = {}) {
const tests = metadata.tests || {}

let testsPassed = tests.total - tests.pending - tests.failed
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

won't this error when tests is just {} ?

}
}
)
} else {
logger.debug(`Record with ID # ${message.payload.id} does not exists in database. Creating the record`)
// const submission = await helper.reqToAPI(`${config.SUBMISSION_API_URL}/${message.payload.submissionId}`)

const challengeDetail = await helper.reqToAPI(`${config.CHALLENGE_API_URL}?filter=id=${submission.body.challengeId}`)

if (!helper.isGroupIdValid(challengeDetail.body.result.content[0].groupIds)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this too. Shouldn't we do this in a safer manner with _.get() with defaults?

@@ -10,7 +10,8 @@ const LeaderboardSchema = new Schema({
challengeId: { type: String },
memberId: { type: String },
handle: { type: String },
aggregateScore: { type: Number }
aggregateScore: { type: Number },
testsPassed: { type: Number }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we also post the total number of tests?

@cwdcwd cwdcwd merged commit d14d50d into develop Nov 13, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants