Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix missing participants #1

Merged
merged 6 commits into from
Mar 28, 2020

Conversation

Bobface
Copy link
Contributor

@Bobface Bobface commented Mar 21, 2020

Fix missing participants

Adds checks for missing participants when calling calculating finalize maps.

The method calculateFinalizeMaps has a new parameter overrideMissingValue which defaults to false.

  • If overrideMissingValue == false the method throws an error when it detects a missing participant in the participants array.
  • If overrideMissingValue == true the method inserts a dummy participant with status = PARTICIPANT_STATUS.REGISTERED when it detects a missing participant

@@ -211,6 +211,30 @@ describe('.calculateFinalizeMaps', () => {

expect(calculateFinalizeMaps(ps)).toEqual(maps)
})

it.only('p5 is missing, override', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please take out only to make sure that all the other tests pass

ps.splice(5, 1)

expect(calculateFinalizeMaps(ps, true)).toEqual(maps)
})
Copy link
Contributor

@makoto makoto Mar 21, 2020

Choose a reason for hiding this comment

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

please add another test case which throws the Invalid overrideMissingValue error

@@ -20,10 +20,36 @@ export const calculateNumAttended = participants => participants.reduce((m, v) =
return m + (attended ? 1 : 0)
}, 0)

export const calculateFinalizeMaps = participants => {
export const calculateFinalizeMaps = (participants, overrideMissingValue = false) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking about the following 3 options

  • calculateFinalizeMaps(participants) = Default, raises error if there is a missing value
  • calculateFinalizeMaps(participants, overrideMissingValue=1) = set missing value as 1
  • calculateFinalizeMaps(participants, overrideMissingValue=0)= set missing value as 0
  • calculateFinalizeMaps(participants, overrideMissingValue=12345)= invalid value, throw error

Copy link
Contributor

Choose a reason for hiding this comment

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

Would be better to have the caller pass in the PARTICIPANT_STATUS.<value> as the overrideMissingValue.

Copy link
Contributor

@makoto makoto Mar 23, 2020

Choose a reason for hiding this comment

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

@hiddentao not sure about that. PARTICIPANT_STATUS has 5 valid statuses but this option has 3 (1, 0, null=throw error) and they don't map equally

Copy link
Contributor

Choose a reason for hiding this comment

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

Just disallow the ones that don't map then.

Copy link
Contributor

Choose a reason for hiding this comment

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

Otherwise, create a new enum to pass in as that would still be better than passing in magic numbers.

@makoto makoto merged commit 8dd7102 into wearekickback:master Mar 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants