Skip to content

[OldMongo FC-0004] Tests for removing support for children in Old Mongo - part 1 - #31422

Merged
ormsbee merged 1 commit into
openedx:masterfrom
raccoongang:sagirov/EDXOLDMNG-192-part1
Dec 21, 2022
Merged

[OldMongo FC-0004] Tests for removing support for children in Old Mongo - part 1#31422
ormsbee merged 1 commit into
openedx:masterfrom
raccoongang:sagirov/EDXOLDMNG-192-part1

Conversation

@UvgenGen

@UvgenGen UvgenGen commented Dec 8, 2022

Copy link
Copy Markdown
Contributor

Description

First part of preparing tests for Remove support for children in Old Mongo task (#31134).
Fixed problem with .get_children() in tests for split modulestore.
Problem: If we create some CourseItem with some parent, we must get the updated parent item before using that parent item.

Useful information to include:
Remove support for children in Old Mongo PR: #31134
openedx/public-engineering#80

@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Dec 8, 2022
@openedx-webhooks

Copy link
Copy Markdown

Thanks for the pull request, @UvgenGen! Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

  • supporting documentation
  • Open edX discussion forum threads
  • timeline information ("this must be merged by XX date", and why that is)
  • partner information ("this is a course on edx.org")
  • any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

@ormsbee ormsbee left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

A couple of minor questions. Thank you1

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Isn't the purpose of the test to see if it saved properly and could be read back?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

updated

Comment thread xmodule/modulestore/tests/utils.py Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you please explain in the commit message why this was change was necessary?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

When using parent_location instead of parent I get an error related with children/parent problem for split modulestore. Looking at other tests using split modulestore - parent argument is used only. I'll add it to commit message later.

@mphilbrick211 mphilbrick211 added the FC Relates to an Axim Funded Contribution project label Dec 16, 2022

@ormsbee ormsbee left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please squash changes and add context to the commit message. Thank you.

* fixed problem with creating some CourseItem with some parent:
we must get the updated parent item before using that parent item
* replaced parent_location with parent argument in ItemFactory due to
error children/parent relation for split modulestore. In all tests with
split modulestore parent argument used
@UvgenGen
UvgenGen force-pushed the sagirov/EDXOLDMNG-192-part1 branch from 7676560 to 8ad9ac6 Compare December 21, 2022 10:17
@UvgenGen

Copy link
Copy Markdown
Contributor Author

@ormsbee PR squashed and rebased, commit message updated)

@openedx-webhooks

Copy link
Copy Markdown

@UvgenGen 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

@edx-pipeline-bot

Copy link
Copy Markdown
Contributor

EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production.

@edx-pipeline-bot

Copy link
Copy Markdown
Contributor

EdX Release Notice: This PR has been deployed to the production environment.

1 similar comment
@edx-pipeline-bot

Copy link
Copy Markdown
Contributor

EdX Release Notice: This PR has been deployed to the production environment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

FC Relates to an Axim Funded Contribution project open-source-contribution PR author is not from Axim or 2U

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

5 participants