feat: add support for picking storage config from settings #344

iamsobanjaved · 2023-09-14T13:08:54Z

Adding support for picking storage class from the settings. If not defined, go with the system's default storage.

connorhaugh

Not a maintainer, but lgtm.

pdpinch

Can you explain a bit more about what this change does? I think you want to be able to configure a separate storage location for SGA submissions from other user submissions. Is that correct?

I also want to confirm that this new setting won't be required, and existing installations with no value for SGA_STORAGE_SETTINGS will continue to function.

I left a couple of comments about documentation, but I'll ask someone else from our team to review the code.

pdpinch · 2023-09-14T13:47:01Z

edx_sga/__init__.py

@@ -2,4 +2,4 @@
 Module for StaffGradedAssignmentXBlock.
 """

-__version__ = "0.22.0"
+__version__ = "0.23.0"


Please remove the version change. Our release bot will take care of this.

pdpinch · 2023-09-14T13:53:46Z

edx_sga/utils.py

 from edx_sga.constants import BLOCK_SIZE


+def get_default_storage():
+    sga_storage_settings = getattr(settings, "SGA_STORAGE_SETTINGS", None)


Is this introducing a new configuration setting to edx-platform for this xBlock?

Please follow the guidelines in OEP-17 for adding annotations for settings and flags.

In the annotation, please include an example of the data structure for SGA_STORAGE_SETTINGS.

Yes introducing new configuration, added the annotations

iamsobanjaved · 2023-09-14T14:43:27Z

Can you explain a bit more about what this change does? I think you want to be able to configure a separate storage location for SGA submissions from other user submissions. Is that correct?

Yes making the storage configurable for edX.

I also want to confirm that this new setting won't be required, and existing installations with no value for SGA_STORAGE_SETTINGS will continue to function.

Yes, if this setting isn't defined, it will use default_storage from django.core.storage.

codecov · 2023-09-15T09:37:39Z

Codecov Report

Merging #344 (ce26862) into master (907d670) will increase coverage by 0.47%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #344      +/-   ##
==========================================
+ Coverage   53.27%   53.75%   +0.47%     
==========================================
  Files          16       16              
  Lines        1663     1680      +17     
  Branches      114      115       +1     
==========================================
+ Hits          886      903      +17     
  Misses        764      764              
  Partials       13       13

Files Changed	Coverage Δ
edx_sga/sga.py	`64.62% <100.00%> (ø)`
edx_sga/tasks.py	`47.05% <100.00%> (ø)`
edx_sga/tests/test_utils.py	`100.00% <100.00%> (ø)`
edx_sga/utils.py	`94.73% <100.00%> (+0.98%)`	⬆️

arslanashraf7

Thanks for creating this PR @iamsobanjaved.

I just did the code review for now and suggested a couple of changes, Let me know what you think about them.

I'll test the PR in re-review.

arslanashraf7 · 2023-09-15T10:27:33Z

edx_sga/utils.py

+    sga_storage_settings = getattr(settings, "SGA_STORAGE_SETTINGS", None)
+
+    if sga_storage_settings:
+        return get_storage_class(


I have two things in mind regarding this:

nit: get_storage_class in Django automatically fallbacks to and returns default storage, e.g. FileStorage if the provided args are None or empty. We can use this and easily remove some condition statements and code lines.

Let me know, If you think that would be less readable.

nit: This can throw ImportError if the provided settings/class name isn't correct.

Now we can either 1) let the error bubble up in the logs or 2) Catch it and raise a properly formatted named exception such as ImproperlyConfigured from django.core.exceptions. I'm fine either way so this change is kind of optional, I just wanted to mention it anyway.

Yes, but we still have to call the constructor with the keyword arguments too. It would still require the conditional statement.

Yes it can throw an error, but there is a single source from which this value will come, that's why skipped throw catch in first place.

arslanashraf7 · 2023-09-15T10:28:37Z

edx_sga/utils.py

 from edx_sga.constants import BLOCK_SIZE


+def get_default_storage():


Can you write a simple test for this? That would go in test_utils.py.

arslanashraf7 · 2023-09-15T10:39:45Z

edx_sga/utils.py

 from edx_sga.constants import BLOCK_SIZE


+def get_default_storage():
+    """
+    Get config for storage from settings, use DJango's default_storage if no such settings defined


Suggested change

Get config for storage from settings, use DJango's default_storage if no such settings defined

Get config for storage from settings, use Django's default_storage if no such settings are defined

arslanashraf7 · 2023-09-15T10:51:58Z

edx_sga/utils.py

+    """
+    # .. setting_name: SGA_STORAGE_SETTINGS
+    # .. setting_default: {}
+    # .. setting_description: Specifies the storage class and keyword arguments to use in the constructor


I think this might be a better description.

Suggested change

# .. setting_description: Specifies the storage class and keyword arguments to use in the constructor

# .. setting_description: Specifies the storage class and keyword arguments to use in the constructor.

# .. Default Storage would be used if this setting is not specified.

iamsobanjaved · 2023-09-15T12:02:46Z

@arslanashraf7 Kindly review this now

arslanashraf7

Thanks for making all the changes. Looks great 👍 . Things worked while testing.

connorhaugh approved these changes Sep 14, 2023

View reviewed changes

pdpinch requested changes Sep 14, 2023

View reviewed changes

iamsobanjaved force-pushed the fix-config branch from 3604872 to e607305 Compare September 14, 2023 14:40

arslanashraf7 self-assigned this Sep 15, 2023

arslanashraf7 added the Needs Review label Sep 15, 2023

arslanashraf7 requested changes Sep 15, 2023

View reviewed changes

arslanashraf7 added waiting on author and removed Needs Review labels Sep 15, 2023

Muhammad Soban Javed added 2 commits September 15, 2023 17:00

feat: add support for picking storage config from settings

3ea008d

test: add test case for get_default_storage in utils.py

ce26862

iamsobanjaved force-pushed the fix-config branch from e607305 to ce26862 Compare September 15, 2023 12:00

iamsobanjaved requested a review from arslanashraf7 September 15, 2023 12:02

pdpinch approved these changes Sep 15, 2023

View reviewed changes

arslanashraf7 approved these changes Sep 15, 2023

View reviewed changes

arslanashraf7 merged commit 65d039f into mitodl:master Sep 15, 2023
3 checks passed

odlbot mentioned this pull request Sep 15, 2023

Release 0.23.0 #345

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add support for picking storage config from settings #344

feat: add support for picking storage config from settings #344

iamsobanjaved commented Sep 14, 2023

connorhaugh left a comment

pdpinch left a comment

pdpinch Sep 14, 2023

iamsobanjaved Sep 14, 2023

pdpinch Sep 14, 2023

iamsobanjaved Sep 14, 2023

iamsobanjaved commented Sep 14, 2023

codecov bot commented Sep 15, 2023 •

edited

arslanashraf7 left a comment

arslanashraf7 Sep 15, 2023

iamsobanjaved Sep 15, 2023

arslanashraf7 Sep 15, 2023

iamsobanjaved Sep 15, 2023

arslanashraf7 Sep 15, 2023

iamsobanjaved Sep 15, 2023

arslanashraf7 Sep 15, 2023

iamsobanjaved Sep 15, 2023

iamsobanjaved commented Sep 15, 2023

arslanashraf7 left a comment

		from edx_sga.constants import BLOCK_SIZE


		def get_default_storage():

	Get config for storage from settings, use DJango's default_storage if no such settings defined
	Get config for storage from settings, use Django's default_storage if no such settings are defined

	# .. setting_description: Specifies the storage class and keyword arguments to use in the constructor
	# .. setting_description: Specifies the storage class and keyword arguments to use in the constructor.
	# .. Default Storage would be used if this setting is not specified.

feat: add support for picking storage config from settings #344

feat: add support for picking storage config from settings #344

Conversation

iamsobanjaved commented Sep 14, 2023

connorhaugh left a comment

Choose a reason for hiding this comment

pdpinch 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

iamsobanjaved commented Sep 14, 2023

codecov bot commented Sep 15, 2023 • edited

Codecov Report

arslanashraf7 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

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

iamsobanjaved commented Sep 15, 2023

arslanashraf7 left a comment

Choose a reason for hiding this comment

codecov bot commented Sep 15, 2023 •

edited