-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
fix: blocks v2 will filter discussion xblocks #31820
Conversation
30fcc78
to
8456500
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should add a waffle flag for this, and unit tests for the change would be nice to have.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a nit. Looks good!
I think waffle flag is not needed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR looks good to me
can we add some unit test cases here?
if provider == Provider.OPEN_EDX and request_from_mobile: | ||
blocks = response.data | ||
filtered_blocks = {} | ||
for key, value in blocks.get('blocks', {}).items(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just a nit.
response.data = {key: value for key, value in blocks.get('blocks', {}).items() if value.get('type') != 'discussion'}
This is the temporary fix to stop mobile application from crashing. Is test case required for this? |
8456500
to
09e896a
Compare
""" | ||
configuration = DiscussionsConfiguration.get(context_key=course_key) | ||
provider = configuration.provider_type | ||
if provider == Provider.OPEN_EDX and request_from_mobile: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we do not need this request_from_mobile condition. If the provider is OPEN_EDX then we do not want to return the discussion xblocks in any case because they are no longer relevant
7a41b67
to
00eb281
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of nits. Rest looks good
False, | ||
True, | ||
) | ||
def test_filter_discussion_xblocks_for_mobile(self, change_provider): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need to add "for_mobile" here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
change_provider can be changed to "is_open_edx_provider" for clarity
00eb281
to
8e8883f
Compare
EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production. |
EdX Release Notice: This PR has been deployed to the production environment. |
1 similar comment
EdX Release Notice: This PR has been deployed to the production environment. |
Mobile app was crashing when discussion xblock was clicked for openedx provider. This change will filter discussion xblocks from blocks v2 api when provider is openedx and request is sent from mobile