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

Boolean flags mismanaged during assignment creation and update #64

Closed
liblit opened this issue Aug 31, 2017 · 9 comments
Closed

Boolean flags mismanaged during assignment creation and update #64

liblit opened this issue Aug 31, 2017 · 9 comments

Comments

@liblit
Copy link
Contributor

liblit commented Aug 31, 2017

Working with canvasapi 0.6.0, I recently used Course.create_assignment to create a large number of assignments. Each assignment had both peer_reviews and omit_from_final_grade explicitly set to False. (Granted, these are the defaults, but for unrelated reasons it was useful to be explicit.) I was surprised to find that the newly-generated courses had each of these settings turned on, rather than off.

I then used Assignment.edit to correct the bad settings, again with both peer_reviews and omit_from_final_grade explicitly set to False. The calls reported no errors, but these two settings remained True on subsequent queries or when viewed in the standard web interface.

Next I used Assignment.edit to correct the bad settings again, but this time with both peer_reviews and omit_from_final_grade explicitly set to 'false' as a lower-case string. The calls reported no errors, and the two settings were indeed changed as desired.

So it seems that False is either ignored or misinterpreted as true, whereas 'false' is interpreted as false. I can pass the 'false' string for now as a workaround, but surely it would be better for False to have the expected interpretation. When getting results back from the server, Boolean settings come back as True and False, not 'true' and 'false'. Passing settings up to the server in creation and modification requests should behave the same.

@Thetwam
Copy link
Member

Thetwam commented Aug 31, 2017

We've had issues in the past with Canvas being weird about what it accepts for boolean values. See this issue for example.

It could be related. Or, it could be on our end. Will investigate.

@Thetwam
Copy link
Member

Thetwam commented Sep 22, 2017

When attempting to replicate this issue, I ran into some odd behavior. For each test, I kept everything the same except peer_reviews, which I tried different values for.

First, I tried False:

course = canvas.get_course(COURSE_ID)
assignment_template = {
    'name': 'CanvasAPI Assignment',
    'peer_reviews': False
}
assignment = course.create_assignment(assignment_template)
print(assignment.peer_reviews)

@liblit: based on your report, I expected it would incorrectly set peer_reviews to True, but it correctly returned as False.

When I tried True, it still returned (incorrectly) as False. I then tried a few other values:

peer_reviews Response Expected?
True False ❌ no
False False ✅ yes
'true' True ✅ yes
'false' False ✅ yes
1 True ✅ yes
0 False ✅ yes

I was suspicious that boolean values were simply being ignored, so I created assignments that had peer_reviews set to True (and confirmed with an assert), then used edit to modify them:

assignment_template = {
    'name': 'CanvasAPI Assignment',
    'peer_reviews': 'true'
}
assignment = course.create_assignment(assignment_template)

# ensure peer_reviews starts as True
assert assignment.peer_reviews is True

# edit assignment to set it to False
assignment.edit(assignment={'peer_reviews': False})

# ensure it's not just the local object that's been changed
confirm_assignment = course.get_assignment(assignment.id)
assert confirm_assignment.peer_reviews == assignment.peer_reviews

print(assignment.peer_reviews)

This correctly changed peer_reviews from True to False. So, I tried a few more things:

peer_reviews Response Expected?
True False ❌ no
False False ✅ yes
'true' True ✅ yes
'false' False ✅ yes
1 True ✅ yes
0 False ✅ yes

These results are identical to the creation results, which was very surprising to me. If booleans were being ignored/dropped, both True and False should have returned True. However, in BOTH cases, they return False.

So... I'm not entirely certain what's going on, especially since this seems to be opposite to behavior that you reported. Do you have example code that replicates the behavior you reported?

@jessemcbride
Copy link
Member

From my own testing, I discovered that the string True and the string False are not evaluated correctly by Canvas (in this case).

The requests library sends 'True' and 'False' when it encounters a boolean, so this would explain the behavior. We can hotfix this issue by converting boolean values to strings ourselves before we fire off a request, but I think it's worth starting a dialogue with Canvas about the inconsistency we've observed.

@Thetwam Thetwam added the canvas label Sep 22, 2017
@liblit liblit changed the title Boolean flags mismanaged during course creation and update Boolean flags mismanaged during assignment creation and update Sep 22, 2017
@liblit
Copy link
Contributor Author

liblit commented Sep 22, 2017

@Thetwam asked:

Do you have example code that replicates the behavior you reported?

I do. In fact, the code you offered shows the bug when I run it:

course = canvas.get_course(COURSE_ID)
assignment_template = {
    'name': 'CanvasAPI Assignment',
    'peer_reviews': False
}
assignment = course.create_assignment(assignment_template)
print(assignment.peer_reviews)

You say that this prints False. For me, it prints True.

Extending this to include omit_from_final_grade:

assignment_template = {
    'name': 'CanvasAPI Assignment',
    'peer_reviews': False,
    'omit_from_final_grade': False,
}
assignment = course.create_assignment(assignment_template)
print(assignment.peer_reviews, assignment.omit_from_final_grade)

This prints True True, consistent with my original report. I would have expected False False if canvasapi were working correctly.


Could the inconsistency between my and @Thetwam’s output be caused by a difference in some underlying supporting package? Here is my complete dependency stack as printed by pipdeptree -p canvasapi:

canvasapi==0.6.0
  - requests [required: Any, installed: 2.18.4]
    - certifi [required: >=2017.4.17, installed: 2017.7.27.1]
    - chardet [required: >=3.0.2,<3.1.0, installed: 3.0.4]
    - idna [required: >=2.5,<2.7, installed: 2.6]
    - urllib3 [required: <1.23,>=1.21.1, installed: 1.22]
  - six [required: Any, installed: 1.11.0]

@liblit
Copy link
Contributor Author

liblit commented Sep 22, 2017

I speculated:

Could the inconsistency between my and @Thetwam’s output be caused by a difference in some underlying supporting package?

Or maybe @Thetwam and I are interacting with different installations of Canvas itself? I do not know how to ask a Canvas server for its version number, or whether that is even a meaningful thing. My Canvas server is canvas.wisc.edu, if that helps any.

@jessemcbride
Copy link
Member

@liblit We've figured that out. It looks like you can append /health_check.json to the root of a public Canvas instance to get that data back.

Our universities are (at least now) running the same version of Canvas. Can you try running this again to see if it's still acting up?

@liblit
Copy link
Contributor Author

liblit commented Oct 26, 2017

I reran the following bug-demonstrating code from my earlier comment:

assignment_template = {
    'name': 'CanvasAPI Assignment',
    'peer_reviews': False,
    'omit_from_final_grade': False,
}
assignment = course.create_assignment(assignment_template)
print(assignment.peer_reviews, assignment.omit_from_final_grade)

This code prints True True instead of the expected False False. So yes, the problem is still occurring as I originally reported. Drat. 😞

@jessemcbride jessemcbride added this to the CanvasAPI v0.9.0 milestone Feb 8, 2018
@jessemcbride
Copy link
Member

I've reported this issue to Canvas: instructure/canvas-lms#1232

@jessemcbride
Copy link
Member

jessemcbride commented Feb 17, 2018

We’ve patched this in develop following the approach I outlined in my comment last September. See commit 65b7549.

@Thetwam Thetwam moved this from To-Do to In Develop in CanvasAPI v0.9.0 Feb 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
CanvasAPI v0.9.0
  
In Develop
Development

No branches or pull requests

3 participants