feat: Add manufacture_data django command #369

marlonkeating · 2023-12-11T17:46:02Z

Description:

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.

JIRA

ENT-7636

Installation/Testing instructions:

In Devstack, prior to being pushed to PyPi

Copy to IDA VM's python path
Example: docker cp ~/edx-repos/edx-django-utils/. {container uuid}:/lib/
Where container uuid can be found by running docker ps
Create management command that inherits from manufacture_data.command and imports model factories (Example: feat: Add manufacture_data django command enterprise-catalog#734)
Run manufacture_data management command
Example:
python ./manage.py manufacture_data --model enterprise_catalog.apps.catalog.models.EnterpriseCatalog --title "Test Catalog"

Merge checklist:

Post merge:

Create a tag
Check new version is pushed to PyPi after tag-triggered build is
finished. (https://github.com/openedx/edx-django-utils/releases/tag/v5.10.1)
Delete working branch (if not needed anymore)

.venv-3.8/pyvenv.cfg

robrap · 2023-12-11T21:20:13Z

Hello @marlonkeating. I know this is just a draft. I'm going to check in with arch-bom about the planned how and where for developer data. See edx/edx-arch-experiments#138

edx_django_utils/data_generation/management/commands/manufacture_data.py

alex-sheehan-edx

Small comments/question mostly about documentation. Otherwise looks great and I'm excited to get this released!

edx_django_utils/data_generation/README.rst

alex-sheehan-edx · 2024-01-12T19:20:19Z

edx_django_utils/data_generation/README.rst

+
+Unsupported Cases
+-----------------
+One limitation of this script is that it can only fetch or customize, you cannot customize a specified, existing FK:


you cannot customize a specified, existing FK:

or customizations to any subclasses/FK's to the specified to an existing object

Edited for more clarity

edx_django_utils/data_generation/README.rst

edx_django_utils/data_generation/tests/apps.py

edx_django_utils/data_generation/tests/test_management.py

alex-sheehan-edx · 2024-01-12T19:25:35Z

edx_django_utils/data_generation/tests/test_management.py

+    defaults = dict(
+        defaults._get_kwargs(), **arg_options
+    )
+    # Commented out section allows for unknown options to be passed to the command


I feel like this should go to the top of the method, explaining why we have to redefine the core django function in the test.

Went ahead and removed the commented out code, and explained the disabled arg checking at the top. I figure if we ever want that code back, we know where we originally got it.

rgraber

Mostly nits and requests for clarification. Also, I'm surprised that this passed the doc linter. That's not on you, but I think something is up with the way the linter is configured. It should complain when docstrings are not something like

def my_func(arg1):
    Does an important thing.

   More information about important thing.
   Arguments:
       arg1: Is a thing

CHANGELOG.rst

rgraber · 2024-01-11T14:09:26Z

README.rst

@@ -23,13 +23,12 @@ This repository includes shared utilities for:

 * `Logging Utilities`_: Includes log filters and an encrypted logging helper.

-* `Monitoring Utilities`_: Includes Middleware and utilities for enhanced monitoring.


Why did you remove this?

Good catch, that wasn't my intention. It may have been a merge issue that I missed.

rgraber · 2024-01-11T17:49:39Z

edx_django_utils/data_generation/README.rst

+
+Unsupported Cases
+-----------------
+One limitation of this script is that it can only fetch or customize, you cannot customize a specified, existing FK:


I don't understand this sentence. Can you elaborate more on what we are fetching and customizing? And do you mean we cannot customize an existing object? There's not really a notion of customizing keys.
I also think modify makes more sense than customize but that's minor.

Updated based on @alex-sheehan-edx's comments

rgraber · 2024-01-11T17:50:19Z

edx_django_utils/data_generation/README.rst

+`./manage.py lms manufacture_data --model enterprise.models.EnterpriseCustomerUser --enterprise_customer <invalid uuid>`
+
+we'd get:
+`CommandError: Provided FK value: <SOMETHING BAD> does not exist on EnterpriseCustomer`


Suggested change

`CommandError: Provided FK value: <SOMETHING BAD> does not exist on EnterpriseCustomer`

`CommandError: Provided FK value: <invalid uuid> does not exist on EnterpriseCustomer`

?

Yes, made that correction based on @alex-sheehan-edx 's comment

edx_django_utils/data_generation/management/commands/manufacture_data.py

rgraber · 2024-01-11T17:54:23Z

edx_django_utils/data_generation/management/commands/manufacture_data.py

+
+def is_not_pascal(string):
+    """
+    helper method to detect if strings are not Pascal case.


An example of Pascal case would be useful here (I'd never heard the term before)

CapWords is how it's named in the python documentation, added note in the comments.

rgraber · 2024-01-11T18:03:33Z

edx_django_utils/data_generation/management/commands/manufacture_data.py

+
+    def __str__(self, level=0):
+        """
+        Overridden str method to allow for proper tree printing


An example of the expected output would be helpful

I think the best example is in the unit tests. I don't know that there's a more compact way to illustrate the output than by reading the code here, and referencing the unit test.

edx_django_utils/data_generation/README.rst

rgraber

One incredibly obnoxious nit, otherwise looks great! Thanks for all your hard work on this.

rgraber · 2024-01-16T20:53:47Z

edx_django_utils/data_generation/management/commands/manufacture_data.py

+
+def build_tree_from_field_list(list_of_fields, provided_factory, base_node, customization_value):
+    """
+    Builds a non-binary tree of nodes based on a list of children nodes, using a base node and it's associated data


[nit]

Suggested change

Builds a non-binary tree of nodes based on a list of children nodes, using a base node and it's associated data

Builds a non-binary tree of nodes based on a list of children nodes, using a base node and its associated data

test: Fix test configuration test: improve test coverage test: case for nonstandard model casing test: more coverage docs: Add documentation for manufacture_data docs: Setup documentation for manufacture_data style: Fix code quality errors style: Fix pycodestyle errors style: Fix isort errors test: remove unneeded test code test: error test coverage fix: support field names that don't match snake-cased model names test: remove unused code branches fix: Handle abstract classes during factory discovery fix: Clean up node tree logging docs: Update documentation docs: Update documentation based on comments chore: fix trailing whitespace chore: fix typo

johnnagro marked this pull request as ready for review December 11, 2023 19:32

johnnagro marked this pull request as draft December 11, 2023 19:32

marlonkeating force-pushed the mkeating/provisioning-scripts branch from 2eccbaa to 28c7b29 Compare December 11, 2023 19:35

marlonkeating commented Dec 11, 2023

View reviewed changes

.venv-3.8/pyvenv.cfg Outdated Show resolved Hide resolved

marlonkeating force-pushed the mkeating/provisioning-scripts branch from 28c7b29 to f21002f Compare December 11, 2023 20:43

robrap mentioned this pull request Dec 13, 2023

feat: Add manufacture_data django command edx/edx-arch-experiments#426

Closed

8 tasks

marlonkeating force-pushed the mkeating/provisioning-scripts branch 4 times, most recently from 24304f1 to 0c6b27f Compare December 19, 2023 20:15

marlonkeating marked this pull request as ready for review December 19, 2023 20:16

marlonkeating mentioned this pull request Dec 20, 2023

feat: Add manufacture_data django command openedx/enterprise-catalog#734

Merged

6 tasks

marlonkeating force-pushed the mkeating/provisioning-scripts branch from 0c6b27f to 73698ff Compare December 21, 2023 02:09

marlonkeating force-pushed the mkeating/provisioning-scripts branch 6 times, most recently from e026502 to 3ff5f28 Compare January 5, 2024 13:40

alex-sheehan-edx reviewed Jan 5, 2024

View reviewed changes

edx_django_utils/data_generation/management/commands/manufacture_data.py Show resolved Hide resolved

alex-sheehan-edx reviewed Jan 5, 2024

View reviewed changes

edx_django_utils/data_generation/management/commands/manufacture_data.py Outdated Show resolved Hide resolved

alex-sheehan-edx reviewed Jan 5, 2024

View reviewed changes

edx_django_utils/data_generation/management/commands/manufacture_data.py Outdated Show resolved Hide resolved

marlonkeating force-pushed the mkeating/provisioning-scripts branch 4 times, most recently from 5b8f80c to 6321589 Compare January 12, 2024 16:03

alex-sheehan-edx reviewed Jan 12, 2024

View reviewed changes

marlonkeating force-pushed the mkeating/provisioning-scripts branch from 6321589 to 5531640 Compare January 13, 2024 00:18

rgraber reviewed Jan 16, 2024

View reviewed changes

marlonkeating force-pushed the mkeating/provisioning-scripts branch 2 times, most recently from 6deb17f to aeb961d Compare January 16, 2024 19:52

rgraber approved these changes Jan 16, 2024

View reviewed changes

marlonkeating force-pushed the mkeating/provisioning-scripts branch from aeb961d to e562a1d Compare January 16, 2024 23:41

marlonkeating force-pushed the mkeating/provisioning-scripts branch from e562a1d to 0f1ad56 Compare January 16, 2024 23:45

marlonkeating merged commit c194e42 into master Jan 16, 2024
8 checks passed

marlonkeating deleted the mkeating/provisioning-scripts branch January 16, 2024 23:53

This was referenced Jan 17, 2024

build: bump version #376

Merged

build: bump version to 5.10.1 #377

Merged

feat: Add manufacture_data django command openedx/enterprise-subsidy#196

Merged

feat: Add manufacture_data django command openedx/license-manager#564

Merged

marlonkeating mentioned this pull request Feb 6, 2024

feat: Add manufacture_data django command openedx/enterprise-access#400

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add manufacture_data django command #369

feat: Add manufacture_data django command #369

marlonkeating commented Dec 11, 2023 •

edited

Loading

robrap commented Dec 11, 2023 •

edited

Loading

alex-sheehan-edx left a comment

alex-sheehan-edx Jan 12, 2024

marlonkeating Jan 13, 2024

alex-sheehan-edx Jan 12, 2024

marlonkeating Jan 13, 2024

rgraber left a comment •

edited

Loading

rgraber Jan 11, 2024

marlonkeating Jan 16, 2024

rgraber Jan 11, 2024

marlonkeating Jan 16, 2024

rgraber Jan 11, 2024

marlonkeating Jan 16, 2024

rgraber Jan 11, 2024

marlonkeating Jan 16, 2024

rgraber Jan 11, 2024

marlonkeating Jan 16, 2024

rgraber left a comment

rgraber Jan 16, 2024

		@@ -23,13 +23,12 @@ This repository includes shared utilities for:

		* `Logging Utilities`_: Includes log filters and an encrypted logging helper.

		* `Monitoring Utilities`_: Includes Middleware and utilities for enhanced monitoring.

	`CommandError: Provided FK value: <SOMETHING BAD> does not exist on EnterpriseCustomer`
	`CommandError: Provided FK value: <invalid uuid> does not exist on EnterpriseCustomer`

	Builds a non-binary tree of nodes based on a list of children nodes, using a base node and it's associated data
	Builds a non-binary tree of nodes based on a list of children nodes, using a base node and its associated data

feat: Add manufacture_data django command #369

feat: Add manufacture_data django command #369

Conversation

marlonkeating commented Dec 11, 2023 • edited Loading

robrap commented Dec 11, 2023 • edited Loading

alex-sheehan-edx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rgraber left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rgraber left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marlonkeating commented Dec 11, 2023 •

edited

Loading

robrap commented Dec 11, 2023 •

edited

Loading

rgraber left a comment •

edited

Loading