TNL-5721 – Fix check source module access before appending it to required modules #13868
Conversation
| if module: | ||
| result.append(module) | ||
|
|
||
| return result |
There was a problem hiding this comment.
what is result is empty?
Qubad786
changed the title
|
@Qubad786 can you give steps to create such a conditional module in a course? I'd like to be able to test this locally |
| <p class="conditional-message">${_message(reqm, message)}</p> | ||
| % else: | ||
| <p class="conditional-message"> | ||
| You do not have access to this dependency module. Please ask course staff to grant the access. |
There was a problem hiding this comment.
string should be i18n'd. Also, I'd lose the second sentence. Just do "You do not have access to this dependency module."
Qubad786
force-pushed
the
mrehan/tnl-5721-nonetype-no-attr-course-id
branch
from
438f444 to
4b67cb3
Compare
|
@adampalay , I wasn't able to create conditional module from the instructions in https://github.com/edx/edx-platform/pull/11710. I've imported on local. There are plently of conditionals in that course. I could build sandbox and setup a course for testing but http://jenkins.edx.org:8080/view/Ansible/job/ansible-provision/build?delay=0sec does not seem to be working for me. I am filing an IT support ticket. |
|
@Qubad786 Sandbox link is now working. If it's not working for you, I'd like to make a sandbox for your branch. |
|
@attiyaIshaque still no luck for me, it'd be great if you can build one for this branch. |
| # So just log and return false. | ||
| log.warn('Error in conditional module: \ | ||
| required module {module} has no {module_attr}'.format(module=module, module_attr=attr_name)) | ||
| if module is not None: |
There was a problem hiding this comment.
why could a module be None here?
There was a problem hiding this comment.
get_module has been used in required_modules which may return None if requester does not have access to that module – see more.
There was a problem hiding this comment.
can you just include a comment to that effect?
There was a problem hiding this comment.
Fair enough, will do that.
|
@Qubad786 Sandbox is ready. |
|
jenkins run js |
|
Thanks for the sandbox @attiyaIshaque. |
|
@adampalay, @attiyaIshaque just set up a conditional course, courseware link on the ticket. |
Qubad786
force-pushed
the
mrehan/tnl-5721-nonetype-no-attr-course-id
branch
2 times, most recently
from
cc8652b to
c7d2ce4
Compare
|
@Qubad786 Code looks perfect to me. I have also tested on the sandbox. |
Qubad786
force-pushed
the
mrehan/tnl-5721-nonetype-no-attr-course-id
branch
from
c7d2ce4 to
3bd4698
Compare
|
jenkins run bokchoy |
|
jenkins run lettuce |
adampalay
left a comment
adampalay
left a comment
There was a problem hiding this comment.
Minor comment, otherwise 👍
Qubad786
force-pushed
the
mrehan/tnl-5721-nonetype-no-attr-course-id
branch
from
0fff215 to
c7364a0
Compare
|
Yay! |
TNL-5721
Description
Conditional modules with staff only required module(s), when accessed as student, caused uncatched exception while rendering it in mako template. Now, we are checking for the
required_modulebefore using the data from it in string interpolation.Sandbox
Testing
Screenshot
Conditional with three required modules with one of them having staff access.
Reviewers
If you've been tagged for review, please check your corresponding box once you've given the 👍.
Post-review