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

Correct flow submissions #758

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

sahilkamboj3
Copy link

  • Fixed bug to show 100% when student gets all answers correct
  • Also, modified test_points_percent_full() and test_unreachable_points_percent_full() tests

Comment on lines 39 to 48
# DATABASES = {
# "default": {
# "ENGINE": "django.db.backends.postgresql",
# "NAME": "relate",
# "USER": "sahilkamboj",
# "PASSWORD": 'lovebread3',
# "HOST": '127.0.0.1',
# "PORT": '5432',
# }
# }
Copy link
Owner

Choose a reason for hiding this comment

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

?

course/flow.py Outdated
Comment on lines 706 to 707
# Rounding to larger than 100% will break the percent bars on the
# flow results page.
Copy link
Owner

Choose a reason for hiding this comment

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

This comment can probably be removed.

course/flow.py Outdated
Comment on lines 706 to 707
# Rounding to larger than 100% will break the percent bars on the
# flow results page.
Copy link
Owner

Choose a reason for hiding this comment

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

Did you take any measures to prevent the percent bars being fed numbers larger than 100 (e.g. due to rounding)?

import jsonfield.fields


class Migration(migrations.Migration):
Copy link
Owner

Choose a reason for hiding this comment

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

Where did this migration come from?

@inducer
Copy link
Owner

inducer commented Jan 3, 2021

Thanks for working on this! I would argue that your changes don't really address the issue, as the rounded values could very well still add up to a value greater than 100.

Observe:

  • 0.6
  • 0.6
  • 0.6

Each round to 1. Suppose 2 (instead of 100) is the upper limit. Individually, the sum is 1.8. Rounded, they sum to 3.

Copy link
Owner

@inducer inducer left a comment

Choose a reason for hiding this comment

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

Thanks! (And sorry again about the epic delay... life got busy in a hurry.) This looks great. My only request is to add comments explaining why we're doing the dodgy-looking multiplication, so that folks who read this code in the future will have an easier time understanding. After that, it's ready to land.

@@ -42,12 +42,18 @@ <h1>{% trans "Results" %}: {{flow_desc.title}}</h1>
{% if grade_info.total_points_percent < 100.001 %}
{# otherwise we'll have trouble drawing the bar #}
<div class="progress">
{% with points_percent=grade_info.points_percent|mul:0.9999 %}
Copy link
Owner

Choose a reason for hiding this comment

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

Add a template {# ... #} comment on why that's here.

@@ -64,14 +70,22 @@ <h1>{% trans "Results" %}: {{flow_desc.title}}</h1>
{% endif %}
</p>
<div class="progress">
{% with fully_correct_percent=grade_info.fully_correct_percent|mul:0.9999 %}
Copy link
Owner

Choose a reason for hiding this comment

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

Add a template {# ... #} comment on why that's here.

@@ -89,14 +103,22 @@ <h1>{% trans "Results" %}: {{flow_desc.title}}</h1>
{% endif %}
</p>
<div class="progress">
{% with optional_fully_correct_percent=grade_info.optional_fully_correct_percent|mul:0.9999 %}
Copy link
Owner

Choose a reason for hiding this comment

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

Add a template {# ... #} comment on why that's here.

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.

2 participants