Skip to content

EDUCATOR-1029 | More defensive version of ZeroSubsectionGrade.problem_scores - #15701

Merged
iloveagent57 merged 1 commit into
masterfrom
aed/EDUCATOR-1029
Jul 31, 2017
Merged

EDUCATOR-1029 | More defensive version of ZeroSubsectionGrade.problem_scores#15701
iloveagent57 merged 1 commit into
masterfrom
aed/EDUCATOR-1029

Conversation

@iloveagent57

@iloveagent57 iloveagent57 commented Jul 31, 2017

Copy link
Copy Markdown
Contributor

https://openedx.atlassian.net/browse/EDUCATOR-1029

Also added a unit test to surface the bug that this change fixes.

Reviewers

FYI
@nedbat

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If this is meant to guard against None, would if problem_score is not None: be better? Or do you also want to skip the next line if problem_score is zero?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

👍

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't disagree with the end result because it makes the case we're handling more explicit, but isn't problem_score going to end up as an object, not an integer, and thus truthy any time it isn't None?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good question! Objects can evaluate to False: https://docs.python.org/2/reference/datamodel.html#object.__nonzero__

@iloveagent57

Copy link
Copy Markdown
Contributor Author

jenkins run all

@sanfordstudent sanfordstudent left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🇰🇵

@iloveagent57
iloveagent57 merged commit cc6f8be into master Jul 31, 2017
@iloveagent57
iloveagent57 deleted the aed/EDUCATOR-1029 branch July 31, 2017 17:44
@nedbat

nedbat commented Jul 31, 2017

Copy link
Copy Markdown
Contributor

Thanks for the quick fix. Is this something that should go into Ginkgo?

@sanfordstudent

Copy link
Copy Markdown
Contributor

@nedbat this should probably go into Gingko, as should https://github.com/edx/edx-platform/pull/15627 . Apologies for forgetting to alert you to 15627 when I merged it.

@edx-pipeline-bot

Copy link
Copy Markdown
Contributor

EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production on Tuesday, August 01, 2017.

@nedbat

nedbat commented Aug 1, 2017

Copy link
Copy Markdown
Contributor

I've cherry-picked this onto Ginkgo (along with #15627)

@edx-pipeline-bot

Copy link
Copy Markdown
Contributor

EdX Release Notice: This PR has been deployed to the production environment.

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.

4 participants