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: [AXM-749] Implement s3 storage supporting #2581

Merged

Conversation

KyryloKireiev
Copy link

Description

Adjust file upload engine to work with s3

  • The file generation works exactly the same for different media storage backends
  • The course blocks info endpoint provides a link to a file based on the used backend

We adjust developed code to work natively with both local media storage and s3 or other django storages backends.

More info there: https://raccoongang.atlassian.net/wiki/spaces/tech/pages/4236705816/Offline+mode+with+Minio+and+s3+bucket

YouTrack

https://youtrack.raccoongang.com/issue/AXM-749/FollowUp-Adjust-file-upload-engine-to-work-with-s3

openedx/features/offline_mode/tasks.py Outdated Show resolved Hide resolved
openedx/features/offline_mode/utils.py Outdated Show resolved Hide resolved
openedx/features/offline_mode/utils.py Outdated Show resolved Hide resolved
openedx/features/offline_mode/constants.py Outdated Show resolved Hide resolved
@@ -80,33 +81,20 @@ def create_subdirectories_for_asset(file_path):

def remove_old_files(xblock):
"""
Removes the 'assets' directory, 'index.html', and 'offline_content.zip' files
in the specified base path directory.
Removes the old 'offline_content.zip' files from media storage.
Copy link
Collaborator

Choose a reason for hiding this comment

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

The file isn't called offline_content anymore, please update docstring with describing this function

Copy link
Author

Choose a reason for hiding this comment

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

Fixed

@@ -18,85 +19,95 @@
log = logging.getLogger(__name__)


def generate_offline_content(xblock, html_data):
class SaveOfflineContentToStorage:
Copy link
Collaborator

@GlugovGrGlib GlugovGrGlib Jun 19, 2024

Choose a reason for hiding this comment

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

This class name should be changed to be in OOP paradigm. The name is stating an action, and suitable for function, but not for class.

Also the class should be moved outside of utils file into a file similar to class name

Copy link
Author

Choose a reason for hiding this comment

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

Yes, you are right. If you don't mind, I'll take that class name: OfflineContentStorageManager & storage_management.py file

Choose a reason for hiding this comment

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

I think it would be better to name the class something like OfflineContentGenerator instead of renaming the main method to generate.

@@ -80,33 +81,20 @@ def create_subdirectories_for_asset(file_path):

def remove_old_files(xblock):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please change this function name to clean_outdated_xblock_files

Copy link
Author

Choose a reason for hiding this comment

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

Changed

Copy link
Collaborator

Choose a reason for hiding this comment

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

I was actually asking to make Assets manager as class, and not just utils.py
Maybe it's worth to combine the class you've created and some functions from this file?

Copy link
Author

Choose a reason for hiding this comment

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

I'm going to push the latest changes. Based on this, it will be possible to decide what can be refactored

Copy link
Collaborator

@GlugovGrGlib GlugovGrGlib left a comment

Choose a reason for hiding this comment

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

Please address comments and change the files structure as suggested, the tests can be created in the follow up task

@KyryloKireiev KyryloKireiev force-pushed the kireiev/AXM-749/feat/Implement_s3_storage_supporting branch from ea68480 to c4b669a Compare June 20, 2024 07:42
@NiedielnitsevIvan NiedielnitsevIvan self-requested a review June 20, 2024 07:57
Copy link
Collaborator

@GlugovGrGlib GlugovGrGlib left a comment

Choose a reason for hiding this comment

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

LGTM!

@GlugovGrGlib GlugovGrGlib merged commit 04d79c8 into mob-develop Jun 20, 2024
43 checks passed
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.

3 participants