Skip to content

Conversation

@bradenmacdonald
Copy link
Member

@bradenmacdonald bradenmacdonald commented Jul 11, 2016

This fixes https://openedx.atlassian.net/browse/TNL-4901 by backporting the already-existing fix that was on the master branch.

The problem was that mentoring_view was inadvertently modifying its context parameter.

Note 1: @adampalay tested and confirmed this fixes the TNL-4901 issue.

Note 2: on this branch, CircleCI is expected to fail (it's only configured on master), but TravisCI is required to pass - so the tests are fine.

@adampalay
Copy link

@bradenmacdonald 👍

""" Render this XBlock within a mentoring block. """
context = context or {}
context = context.copy() if context else {}
context['self'] = self

Choose a reason for hiding this comment

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

I trust you that this fixes the issue, but I'm confused by why having "self" as a parameter in context doesn't mess up mako's render function

Copy link
Member Author

Choose a reason for hiding this comment

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

This XBlock uses Django to render its templates, not mako. The problem only occurs if this block unintentionally modifies the context dict that was passed as an argument, so that the 'self' gets added to a different scope/context entirely (e.g. it gets added to the context of the parent block).

Copy link
Member Author

Choose a reason for hiding this comment

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

Example:

>>> from django.template import Context, Template
>>> Template(u"hello").render(Context({'self': 'present'}))
u'hello'

@haikuginger
Copy link

haikuginger commented Jul 13, 2016

@bradenmacdonald 👍 Verified that with this change pulled in, the edx-release branch seems to run fine; I was able to create and work on a problem using this XBlock. Code looks fine to me.

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