refactor: create problem feedback runtime service

[Cup0fCoffee]

Description

This PR resolves #33325.

The original PR - #33424 - contains most of the context and discussion. We had to duplicate the PR, as the original one was open from a fork that OpenCraft doesn't have necessary permissions to be able to squash and rebase commits.

Supporting information

Link to other information about the change, such as Jira issues, GitHub issues, or Discourse discussions.
Be sure to check they are publicly readable, or if not, repeat the information here.

Testing instructions

• Checkout to the current branch and build
• Browse the demo course to Content > Outline and search the Homework task
• Pick any of the activities in the live and studio version and use the show answer button

Deadline

"None"

[openedx-webhooks]

Thanks for the pull request, @Cup0fCoffee!

What's next?

Please work through the following steps to get your changes ready for engineering review:

🔘 Get product approval

If you haven't already, check this list to see if your contribution needs to go through the product review process.

• If it does, you'll need to submit a product proposal for your contribution, and have it reviewed by the Product Working Group.
  • This process (including the steps you'll need to take) is documented here.
• If it doesn't, simply proceed with the next step.

🔘 Provide context

To help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:

• Dependencies

  This PR must be merged before / after / at the same time as ...

• Blockers

  This PR is waiting for OEP-1234 to be accepted.

• Timeline information

  This PR must be merged by XX date because ...

• Partner information

  This is for a course on edx.org.

• Supporting documentation
• Relevant Open edX discussion forum threads

🔘 Get a green build

If one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green.

🔘 Let us know that your PR is ready for review:

Who will review my changes?

This repository is currently maintained by @openedx/wg-maintenance-edx-platform. Tag them in a comment and let them know that your changes are ready for review.

Where can I find more information?

If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources:

• Overview of Review Process for Community Contributions
• Pull Request Status Guide
• Making changes to your pull request

When can I expect my changes to be merged?

Our goal is to get community contributions seen and reviewed as efficiently as possible.

However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:

• The size and impact of the changes that it introduces
• The need for product review
• Maintenance status of the parent repository

💡 As a result it may take up to several weeks or months to complete a review and merge your PR.

[Cup0fCoffee]

@Agrendalath When I saw the tests failing, I realized that there some bugs and tried fixing them first. But then I decided to check the original PR, and found that Stephannie brought this up already, and David has replied that since this is a refactoring, the tests should not be changed, even though it does look like a bug. So I made changes to the implementation in the latest commit (didn't squash yet, so it's easy to see what changes were made) to make sure that the tests are passing. The tests that I did modify, are the ones where we actually changed the implementation of the functionality that it's touching.

Feel free to squash and merge, if you're ok with the changes.

[Agrendalath]

Context: we changed this condition from the following:

return (self.close_date is not None and
        datetime.datetime.now(utc) > self.close_date)

Why do we want to return True when there is no close_date or due set? Won't it affect the following code?

edx-platform/openedx/features/personalized_learner_schedules/call_to_action.py

Lines 80 to 84 in b6634a8

	if callable(xblock.is_past_due):
	is_past_due = xblock.is_past_due()
	else:
	PersonalizedLearnerScheduleCallToAction._log_past_due_warning(type(xblock).__name__)
	is_past_due = xblock.is_past_due

Besides, is there a reason to avoid using the methods and properties already provided by the InheritanceMixin?

edx-platform/xmodule/modulestore/inheritance.py

Lines 250 to 269 in f544a48

	@property
	def close_date(self):
	"""
	Return the date submissions should be closed from.
	If graceperiod is present for the course, all the submissions
	can be submitted till due date and the graceperiod. If no
	graceperiod, then the close date is same as the due date.
	"""
	due_date = self.due
	if self.graceperiod is not None and due_date:
	return due_date + self.graceperiod
	return due_date
	def is_past_due(self):
	"""
	Returns the boolean identifying if the submission due date has passed.
	"""
	return self.close_date is not None and timezone.now() > self.close_date

[Cup0fCoffee]

This is related to the comment I left above.

One on hand, we want it to be false when there is no due date set (I'll ignore close_date, because it's due + graceperiod, so if due is None, close_date is also None), for example when we want the learner to be able to complete problems in the self paced course where there are no due dates. On another hand, there is logic related to displaying the answer and it's correctness, that depends on the due date, and in the scenario when there is no due date, we want the learner to be able to see the answer right after they've completed the problem, and in that case we want to it to return true. So these to interpretations of the is_past_due are conflicting. We could keep this method internal to this service.

Re InheritanceMixin, I tried removing the is_past_due method of the capa XBlock, and the tests started failing, so I decided to play it save, and keep it there, as I was trying to make the least number of changes to an already approved PR.

[Cup0fCoffee]

Also, the reason I decided to check for both close_date and due properties of the XBlock, is because on the code block referenced below, XBlocks that are passed to the service seem to not have an implementation for close_date, because the tests start failing. That's why I added the fallback to the due property.

https://github.com/openedx/edx-platform/blob/5907b7e4f5ffa18dba4598871816b9fa353f8dcf/lms/djangoapps/grades/subsection_grade.py#L60-L64

I'm not sure why the don't inherit from the InheritanceMixin, but if they were, they would have the close_date property. It seems like the BlockFactory creates XBlocks that don't inherit from this mixin.

You can see failed test here.

[Cup0fCoffee]

I tried removing the is_past_due method of the ProblemXBlock, located the tests that are failing (bellow for reference), and found that the in the factory that creates the xblocks for the tests the line that should add these mixins is commented out. Do you know why it was commented out? And whether we can uncomment them

================================================================================================================================= short test summary info ==================================================================================================================================
FAILED xmodule/tests/test_capa_block.py::ProblemBlockTest::test_submit_problem_closed - AttributeError: 'ProblemBlock' object has no attribute 'is_past_due'
FAILED xmodule/tests/test_delay_between_attempts.py::XModuleQuizAttemptsDelayTest::test_still_cannot_submit_after_max_attempts - AttributeError: 'ProblemBlock' object has no attribute 'is_past_due'

https://github.com/openedx/edx-platform/blob/5907b7e4f5ffa18dba4598871816b9fa353f8dcf/xmodule/tests/__init__.py#L239-L243

Though uncommenting these lines doesn't fix the issue.

[Agrendalath]

@Cup0fCoffee, I pushed 184516500f3a56860b4b26c98ecfc379e91eb025, which initializes XBlock mixins in CAPA tests.

Short explanation:

1. CapaFactory creates a new ProblemBlock. As the first step, it initializes the runtime here:
   https://github.com/openedx/edx-platform/blob/184516500f3a56860b4b26c98ecfc379e91eb025/xmodule/tests/test_capa_block.py#L121-L125
   (Note: I renamed "system" to "runtime" because the "system" is a deprecated term.)
2. The get_test_system function runs get_test_descriptor_system to create a new TestDescriptorSystem. As we can see below, this class already contains the InheritanceMixin during initialization:
   https://github.com/openedx/edx-platform/blob/184516500f3a56860b4b26c98ecfc379e91eb025/xmodule/tests/__init__.py#L259
3. The mixins arg passed there is then used during the initialization of the Runtime class. It creates a new Mixologist instance.
4. However, the mixing (mixologist.mix(cls)(...)) needs to be explicitly executed. As it's implicitly executed in the construct_xblock_from_class method, I decided to use it instead of initializing the block directly from the ProblemBlock class.

[Cup0fCoffee]

@Agrendalath Nice! Thank you for the explanation. I found another place in xmodule tests that needs this change, and applied it.

I've also removed the close_date and is_past_due methods from the ProblemXBlock.

[Agrendalath]

@Cup0fCoffee, why did we change this one? The idea is to import things from other apps through the api module.

[Cup0fCoffee]

@Agrendalath I didn't dig too much into it (I'm going to dig a bit more after I post this comment), but I think it's because due to some import changes, a circular imports occur. I'm getting a following error when I change from signals to api:

Traceback (most recent call last):
  File "manage.py", line 103, in <module>
    startup.run()
  File "/edx/app/edxapp/edx-platform/lms/startup.py", line 20, in run
    django.setup()
  File "/edx/app/edxapp/venvs/edxapp/lib/python3.8/site-packages/django/__init__.py", line 24, in setup
    apps.populate(settings.INSTALLED_APPS)
  File "/edx/app/edxapp/venvs/edxapp/lib/python3.8/site-packages/django/apps/registry.py", line 116, in populate
    app_config.import_models()
  File "/edx/app/edxapp/venvs/edxapp/lib/python3.8/site-packages/django/apps/config.py", line 269, in import_models
    self.models_module = import_module(models_module_name)
  File "/usr/lib/python3.8/importlib/__init__.py", line 127, in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
  File "<frozen importlib._bootstrap>", line 1014, in _gcd_import
  File "<frozen importlib._bootstrap>", line 991, in _find_and_load
  File "<frozen importlib._bootstrap>", line 975, in _find_and_load_unlocked
  File "<frozen importlib._bootstrap>", line 671, in _load_unlocked
  File "<frozen importlib._bootstrap_external>", line 848, in exec_module
  File "<frozen importlib._bootstrap>", line 219, in _call_with_frames_removed
  File "/edx/app/edxapp/edx-platform/lms/djangoapps/bulk_email/models.py", line 22, in <module>
    from openedx.core.djangoapps.course_groups.cohorts import get_cohort_by_name
  File "/edx/app/edxapp/edx-platform/openedx/core/djangoapps/course_groups/cohorts.py", line 21, in <module>
    from lms.djangoapps.courseware import courses
  File "/edx/app/edxapp/edx-platform/lms/djangoapps/courseware/courses.py", line 39, in <module>
    from lms.djangoapps.courseware.date_summary import (
  File "/edx/app/edxapp/edx-platform/lms/djangoapps/courseware/date_summary.py", line 21, in <module>
    from lms.djangoapps.certificates.api import get_active_web_certificate, can_show_certificate_available_date_field
  File "/edx/app/edxapp/edx-platform/lms/djangoapps/certificates/api.py", line 27, in <module>
    from lms.djangoapps.certificates.generation_handler import (
  File "/edx/app/edxapp/edx-platform/lms/djangoapps/certificates/generation_handler.py", line 24, in <module>
    from lms.djangoapps.grades.api import CourseGradeFactory
  File "/edx/app/edxapp/edx-platform/lms/djangoapps/grades/api.py", line 15, in <module>
    from lms.djangoapps.grades import constants, context, course_data, events
  File "/edx/app/edxapp/edx-platform/lms/djangoapps/grades/context.py", line 10, in <module>
    from .course_grade import CourseGrade
  File "/edx/app/edxapp/edx-platform/lms/djangoapps/grades/course_grade.py", line 17, in <module>
    from .subsection_grade import ZeroSubsectionGrade
  File "/edx/app/edxapp/edx-platform/lms/djangoapps/grades/subsection_grade.py", line 16, in <module>
    from xmodule.services import ProblemFeedbackService
  File "/edx/app/edxapp/edx-platform/xmodule/services.py", line 30, in <module>
    from lms.djangoapps.grades.api import signals as grades_signals
ImportError: cannot import name 'signals' from partially initialized module 'lms.djangoapps.grades.api' (most likely due to a circular import) (/edx/app/edxapp/edx-platform/lms/djangoapps/grades/api.py)

[Cup0fCoffee]

From the stack trace above, it can be observed that when importing from grades.api, it imports other modules from grades. The subsection_grade module tries to import from xmodule.services, which in turn tries to import from grades.api, which forms the circular dependency.

[Cup0fCoffee]

In the subsection_grade the ProblemFeedbackService, which is imported from the xmodule.services is used only once in one of the methods. We can move the import from the top of the file into the method body, to avoid the circular dependency. That way the xmodule.services will be able to import from the grades.api. I'll push the commit now, but please let me know if you think that's not a good approach.

[Agrendalath]

@Cup0fCoffee, I need some help understanding the implications of this change.

@bradenmacdonald, @ormsbee, @kdmccormick, do you know why the InheritanceMixin was not explicitly listed in the mixins for the new runtime?

[bradenmacdonald]

do you know why the InheritanceMixin was not explicitly listed in the mixins for the new runtime?

The new runtime is only used for content libraries and LabXchange, both of which have very flat content trees, so I think that we didn't have any need for it.

[mphilbrick211]

Hey @bradenmacdonald @Cup0fCoffee @Agrendalath! Just checking in on this.

[kdmccormick]

There is a long-term interest in flattening out XBlocks. All XBlocks would be leaf nodes, and structure/dynamicism would be handled by separate subsystem(s) in defined in openedx-learning. Leaving InheritanceMixin out of the new runtime was a nice nod towards this. This ADR describes the rationale, although it only goes as far as saying that the Unit should be the highest-level xblock, rather than getting rid of inheritance entirely.

I know there are still several blocks today that rely on parent-child relationships: chapter/sequence (obviously), library_content, split_test, conditional, problem_builder. So, it might be inevitable that we add InhertianceMixin the new runtime. @Cup0fCoffee can you help me understand why this PR in particular needs to add the mixin?

[Cup0fCoffee]

@kdmccormick The reasoning behind it is that @Agrendalath noticed that ProblemBlock implements several methods that are also implemented in the InheritanceMixin exactly the same way. Since there was an assumption that XBlocks inherit from the InheritanceMixin, the methods that already implemented by the InheritanceMixin can be removed from the ProblemBlock. So in this PR I attempted to remove the close_date and is_past_due methods.

Once I removed the methods, some tests started failing with errors like AttributeError: 'ProblemBlock' object has no attribute 'is_past_due', because runtimes in those tests were not adding InheritanceMixin to the list of mixins, so the XBlocks that were being created were not inheriting from it. So for each failing test, I started tracking down the runtimes that were being used, and adding the mixin to the list until all the tests with this specific error started passing.

You can find more details in the following thread - #34320 (comment).

[kdmccormick]

Thanks for you explanation @Cup0fCoffee , and sorry it took so long for me to get back to you.

I was confused for a while because I had forgotten that InheritanceMixin doesn't actually add inheritance capabilities... rather, it just defines fields which the modulestore runtime treats as inheritable.

Given that, I am OK with adding the mixin to Learning Core runtime, but with the knowledge that no field inheritance will actually happen in that runtime. Could you add a note to InheritanceMixin explaining this? Even better, if you have time, I think it would be good to rename the mixin to InheritableFieldsMixin.

That's my sense at least. @bradenmacdonald , does that sound right to you too?

[bradenmacdonald]

That sounds reasonable @kdmccormick.

[mphilbrick211]

Hi @Cup0fCoffee @kdmccormick @bradenmacdonald is this still in progress?

[mphilbrick211]

Hi @Cup0fCoffee @kdmccormick @bradenmacdonald is this still in progress?

Friendly ping on this @Cup0fCoffee @kdmccormick @bradenmacdonald

[bradenmacdonald]

@Cup0fCoffee @Agrendalath Can you advise - is this something you want to get merged?

[Cup0fCoffee]

@bradenmacdonald I guess, it would make sense to finish it, assuming we are ok spending more time on this (iirc, there isn't much left to do here). I'm not the assignee on the internal ticket, but I can assign it to myself, and put it in the next sprint.