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

feat: Add manufacture_data django command #426

Closed
wants to merge 7 commits into from

Conversation

marlonkeating
Copy link

@marlonkeating marlonkeating commented Aug 30, 2023

Jira Ticket

This change migrates the manufacture_data django command introduced in openedx/edx-enterprise#1826 to where it can be used outside of the enterprise ecosystem.

Merge checklist:
Check off if complete or not applicable:

  • Version bumped
  • Changelog record added
  • Documentation updated (not only docstrings)
  • Fixup commits are squashed away
  • Unit tests added/updated
  • Manual testing instructions provided
  • Noted any: Concerns, dependencies, migration issues, deadlines, tickets

Remaining Issues

  • ManufactureDataCommand is not being exported

@marlonkeating marlonkeating marked this pull request as ready for review September 12, 2023 20:38
[2.1.0] - 2023-06-01
~~~~~~~~~~~~~~~~~~~~

* Adds test factory
Copy link
Contributor

Choose a reason for hiding this comment

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

Two high-level comments:

  1. This functionality seems low on docs about the new functionality (README or longer docstrings, etc.), including this changelog entry.
  2. I don't think this repo is the right place for this, but we should discuss. See https://github.com/edx/edx-arch-experiments/blob/main/docs/decisions/0001-purpose-of-this-repo.rst. I'm not sure that this aligns. That said, the missing docs makes it hard for me to be sure.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree. I think this might make more sense in devstack

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree. I think this might make more sense in devstack

Do we have any history of the decision to use edx-django-utils that we can provide here or are on the new PR?

@@ -0,0 +1,314 @@
"""
Management command for making things with test factories
Copy link
Contributor

Choose a reason for hiding this comment

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

An example of how this might be called would be useful

@rgraber
Copy link
Contributor

rgraber commented Dec 13, 2023

@marlonkeating can I close this now that the work has been moved to django-utils?

@robrap
Copy link
Contributor

robrap commented Dec 13, 2023

can I close this now that the work has been moved to django-utils?

[inform] Here is a link to the PR with the new code: openedx/edx-django-utils#369

@marlonkeating
Copy link
Author

Closed due to work being moved to openedx/edx-django-utils#369

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.

4 participants