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

Adding in support for missing answer text in CAPA problem reports #239

Closed
wants to merge 17 commits into from

Conversation

blarghmatey
Copy link
Member

In the current state if it is unable to find the answer text for a given response (maybe because it was skipped?) then the report will fail to generate. Adding in a conditional with placeholder text allows the report to generate rather than completely failing to provide this data to the course instructor.

asadiqbal and others added 17 commits January 25, 2021 17:29
* Sysadmin: Enhance couses page performance in system admin page

Sysadmin: Only show gitlogs for non superuser

* Fixing disconnect call for mongoengine

Remove problematic disconnect call for mongoengine in git import module

Fixing bug in call to query timer when structure object is empty

* Hack to avoid looking up course git info for sysadmin courses

The courses page of the sysadmin dashboard times out due to the amount of work being done. The git info isn't critical to that view so overriding it with empty values.

Co-authored-by: Amir Qayyum Khan <amir-qayyum-khan@users.noreply.github.com>
Co-authored-by: Tobias Macey <tmacey@mit.edu>
* Added a configuration flag to force third party auth

This adds a toggle to allow operators to prevent user registration and login via username/password authentication, forcing the platform to only support login and registration using third-party auth such as SAML.

* address tobias feedback
* Sync canvas enrollments

* Added instructor dashboard button to push edX grades to Canvas

* Fixed canvas grade syncing JS

* Changed 'edX' reference to 'MITx'

* Use EDIT_COURSE_ACCESS permission instead of is_staff

* Use OVERRIDE_GRADES rule instead

* Use instructor_tasks for canvas work (#183)

* Add per_page to paginated requests

* Static method

* Fix email lowercase mismatch

* Use instructor_tasks to handle canvas work

* Fix transaction error

* Decorator needs to be at the top

* Attempt to fix polling of tasks

* More task bug fixes

* Fix typo

* Fix course key bug

* Update message for push edx grades command

* Use course_key so sync_canvas_enrollments uses the same message

* Format the submitted time

* Don't return output to prevent task output max size error (#189)

Co-authored-by: George Schneeloch <gschneel@mit.edu>
Co-authored-by: Gavin Sidebottom <gsidebo@mit.edu>
This is aggregating a collection of changes for adding a tab to the instructor dashboard for reporting on submissions to rapid response problems.

(cherry picked from commit 817b74e)

Replacing deprecated permissions check for rapid response instructor view

In Juniper the `require_level` decorator is removed in favor of the `require_course_permissions` decorator with more granular permissions definitions. Updated to use the new syntax in the rapid response instructor dashboard and remote gradebook modules.

Removing explicit unicode handling in rapid response report

The rapid response code was explicitly encoding report data unicode which is no longer necessary in Python 3

Fix filename for rapid response CSV exports

The file name for the Rapid Response CSV exports is being rendered as `b'rapid_response_submissions.csv'` due to extraneous encoding. Removed the call to encoding as we are now running in Python 3 so the name is UTF-8 by default.

Use VIEW_DASHBOARD permission for rapid_response view (#187)

removing unicodes from remote gradebook 659af00

Fixing section iteration in remote gradebook 9b03ccf

Fixed 2to3 issues with remote gradebook and json response from gradebook 9819a24

Fixing settings type for remote gradebook plugin app

Converting remote gradebooks to be python 3 compatible 7b9f46d

changing remote_gradebook and canvas_integration modules importing w.r.t koa (https://discuss.openedx.org/t/koa-will-change-how-edx-platform-code-is-imported/3610)
* Adding enrollment restriction for users based on social auth providers

In order to prevent users outside of MIT creating a collaborator account and self-enrolling in courses we need to be able to restrict self-enrollment to users who authenticate using the MIT Kerberos SAML backend. This adds filtering of self-enrollment based on a feature flag that sets the allowed provider ids.

(cherry picked from commit a4c53a1)

* Bugfix for collaborator account access check

Co-authored-by: Tobias Macey <tmacey@mit.edu>
edX requires the full namespace for imports so importing e.g. `remote_gradebook.tasks` is no longer valid. Instead it
requires `from lms.djangoapps.remote_gradebook import tasks`.
- Change submission type to "None"
- Change published state to "False"
In order to allow for preventing users accidentally enrolling in a course we are adding a flag to toggle whether courses in a given installation of edx-platform are invite only, rather than relying on setting it at the per-course level.

(cherry picked from commit 1be29e2)
(cherry picked from commit 2581b511ceb44c6f6677ece4bf833c556fe766f0)
(cherry picked from commit ccce559)
Posting to the remote gradebook via lmod proxy was failing due to a key error when parsing the CSV content. This was due to erroneously encoding a string object. This commit adds a check for the type of the header value to prevent double encoding it.
In the current state if it is unable to find the answer text for a given response (maybe because it was skipped?) then the report will fail to generate. Adding in a conditional with placeholder text allows the report to generate rather than completely failing to provide this data to the course instructor.
@blarghmatey blarghmatey self-assigned this Feb 3, 2021
@blarghmatey
Copy link
Member Author

The relevant stack trace for this error condition is:

    2021-02-03 20:19:00,628 ERROR 2490256 [celery.app.trace] [user None] [ip None] trace.py:255 - Task lms.djangoapps.instructor_task.tasks.calculate_problem_responses_csv.v2[6d4897e6-e11e-4be2-98e1-956f7c4ae05b] raised unexpected: TypeError('sequence item 1: expected str instance, NoneType found')
    Traceback (most recent call last):
      File "/edx/app/edxapp/venvs/edxapp/lib/python3.8/site-packages/celery/app/trace.py", line 412, in trace_task
        R = retval = fun(*args, **kwargs)
      File "/edx/app/edxapp/venvs/edxapp/lib/python3.8/site-packages/newrelic/hooks/application_celery.py", line 99, in wrapper
        return wrapped(*args, **kwargs)
      File "/edx/app/edxapp/venvs/edxapp/lib/python3.8/site-packages/celery/app/trace.py", line 704, in __protected_call__
        return self.run(*args, **kwargs)
      File "/edx/app/edxapp/edx-platform/lms/djangoapps/instructor_task/tasks.py", line 175, in calculate_problem_responses_csv
        return run_main_task(entry_id, task_fn, action_name)
      File "/edx/app/edxapp/edx-platform/lms/djangoapps/instructor_task/tasks_helper/runner.py", line 120, in run_main_task
        task_progress = task_fcn(entry_id, course_id, task_input, action_name)
      File "/edx/app/edxapp/edx-platform/lms/djangoapps/instructor_task/tasks_helper/grades.py", line 990, in generate
        student_data, student_data_keys = cls._build_student_data(
      File "/edx/app/edxapp/edx-platform/lms/djangoapps/instructor_task/tasks_helper/grades.py", line 925, in _build_student_data
        for username, state in block.generate_report_data(user_state_iterator, max_count):
      File "/edx/app/edxapp/edx-platform/common/lib/xmodule/xmodule/capa_module.py", line 481, in generate_report_data
        answer_text = lcp.find_answer_text(answer_id, current_answer=orig_answers)
      File "/edx/app/edxapp/edx-platform/common/lib/capa/capa/capa_problem.py", line 659, in find_answer_text
        answer_text = ", ".join(
    TypeError: sequence item 1: expected str instance, NoneType found

@pdpinch
Copy link
Member

pdpinch commented Feb 4, 2021

We're not sure how to reproduce this issue, although it seems like it might be something like:

  1. Author a multiple choice problem in studio
  2. Submit an answer (in Studio, I think, although maybe it has to be in LMS)
  3. Edit the problem to remove the answer that you had chosen
  4. Create a problem report from the student admin section of the instructor dashboard

@pdpinch
Copy link
Member

pdpinch commented Feb 4, 2021

This PR needs tests

@asadiqbal08 asadiqbal08 self-assigned this Feb 4, 2021
@pdpinch
Copy link
Member

pdpinch commented Feb 5, 2021

Seems like this block needs some error handling to avoid find_answer_text returning None:

elif isinstance(current_answer, six.string_types) and current_answer.startswith('choice_'):
# Many problem (e.g. checkbox) report "choice_0" "choice_1" etc.
# Here we transform it
elems = self.tree.xpath('//*[@id="{answer_id}"]//*[@name="{choice_number}"]'.format(
answer_id=answer_id,
choice_number=current_answer
))
assert len(elems) == 1
choicegroup = elems[0].getparent()
input_cls = inputtypes.registry.get_class_for_tag(choicegroup.tag)
choices_map = dict(input_cls.extract_choices(choicegroup, self.capa_system.i18n, text_only=True))
answer_text = choices_map[current_answer]

@pdpinch
Copy link
Member

pdpinch commented Feb 5, 2021

So, in looking into the most recent case of this, I'm finding that the problems are failing aren't multiple choice which seems at odds with the docstrings on the code. I think we need to figure out the steps to reproduce here.

@asadiqbal08
Copy link

@blarghmatey I am supposed to add test in your PR, would you like to look around the @pdpinch comments over your PR here?

@pdpinch
Copy link
Member

pdpinch commented Feb 10, 2021

@asadiqbal08 can you take this over from Tobias? I'm not sure we're fixing the right problem here, and he doesn't have time to investigate any further.

@asadiqbal08
Copy link

Had a discussion with @HamzaIbnFarooq . He will join here for it on priority.

@HamzaIbnFarooq
Copy link

According to my research, the issue arises in the following situations:

  1. The question has multi-selectable choices (for example in the case of checkboxes problems)
  2. One or More of the selected answers do not have any text in them (e.g You put [ ] in question markup but forgot to place any text in front of it)

Finding:
It's not exactly an error, it's more kind of a Course-Authoring fault.
Ideally, when a Course Author inputs something wrong in the question markup, this error should be pointed out but I believe it requires some validations in edX's Markup rendering module (which I haven't explored yet and might be a third-party library).

So the question is "Are we willing to take this issue further?" @pdpinch @asadiqbal08

Steps to reproduce:

  1. Create a Checkbox Problem on studio
  2. Don't put any text against one of the options
    image
  3. Through LMS select that option with no text
  4. Go to Data Download under Instructor tab
  5. Under the Reports section, select the Problem and press Create a report of problem responses
    The error will be generated.

@pdpinch
Copy link
Member

pdpinch commented Feb 11, 2021

Thanks @HamzaIbnFarooq for the steps to reproduce. I'm going to consult with the MITx team on next steps.

@pdpinch
Copy link
Member

pdpinch commented Feb 12, 2021

I was hoping to get some input from the MITx team about what seems the best approach here. I agree that the root cause is an authoring problem, but there isn't any validation that prevents the error and once the course is in the error state, there doesn't seem to be a way to correct it. I'm going to discuss some options with the team and make a decision about how to proceed. If you have any thoughts or suggestions, please let me know.

@HamzaIbnFarooq
Copy link

@pdpinch I found that the markdown gets rendered by edx's own code, so while debugging I devised a solution and created a PR (#240). Kindly let me know about your thoughts on this approach.

@pdpinch
Copy link
Member

pdpinch commented Apr 28, 2021

@HamzaIbnFarooq do you think we should close this PR in favor of your #240?

@HamzaIbnFarooq
Copy link

HamzaIbnFarooq commented Apr 28, 2021

@HamzaIbnFarooq do you think we should close this PR in favor of your #240?

Yes, I believe #240 shows a huge potential for solving this issue.

Edit: #240 is contaminated with some unwanted commits, the cap issue fix is done in the following commit e2b0866

@HamzaIbnFarooq
Copy link

Summarizing discussions done regarding CAPA reports generation issue

Main issue

When an author creates a multichoice question problem (like checkboxes) through the studio, without putting any text value for an answer checkbox, then the system crashes while generating problem responses report. (A detailed comment is here)

Most probable solutions

  1. While generating problem responses report set a default value instead of giving error (as proposed by tobias here)

  2. Add validation to the multichoice question problem module to notify the author in case he misses putting any text value (as proposed here)

  3. Both points, 1 & 2, should be implemented to fix the old data and to ensure validation of newly created problems.

Questions

  1. Which of the above approaches should be used?
  2. Are we focusing this fix to be an upstream PR or a specific branch?

cc: @pdpinch

@pdpinch
Copy link
Member

pdpinch commented May 27, 2021

Thanks for the analysis.

I'd like to pursue #1 for koa (mitx/koa), unless it takes more than a month, in which case we'll need it for lilac (mitx/lilac). I'm hoping this will help us fix #262 as well.

I'd like to pursue #2 as well, but that could be upstream for edx-platform/master.

@pdpinch
Copy link
Member

pdpinch commented Jun 10, 2021

Does this still need to be merged, or was it replaced by #264?

@HamzaIbnFarooq
Copy link

HamzaIbnFarooq commented Jun 10, 2021

Does this still need to be merged, or was it replaced by #264?

It was replaced by #264

@pdpinch pdpinch closed this Jun 10, 2021
arslanashraf7 pushed a commit that referenced this pull request Mar 14, 2022
fix: escape vulnerable enrollment mode
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.

7 participants