-
Notifications
You must be signed in to change notification settings - Fork 2
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
update and rename edx_module_id #2109
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @abeglova, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
Summary of Changes
This pull request focuses on renaming edx_block_id
to edx_module_id
throughout the codebase. This includes changes in models, serializers, filters, constants, and related tests. Additionally, the logic for generating the edx module ID has been updated to handle 'static' folders differently, using 'asset-v1' as the key type instead of 'block-v1'.
Highlights
- Rename: Renamed
edx_block_id
toedx_module_id
across the project for consistency. - Logic Update: Updated the
get_edx_module_id
function to correctly generate IDs for content in 'static' folders. - Testing: Adjusted tests to reflect the changes in ID generation and the renaming of
edx_block_id
toedx_module_id
.
Changelog
Click here to see the changelog
- learning_resources/etl/utils.py
- Renamed
get_edx_block_id
toget_edx_module_id
(line 349). - Modified
get_edx_module_id
to handle 'static' folders, using 'asset-v1' as the key type (lines 360-372). - Replaced
edx_block_id
withedx_module_id
intransform_content_files
(lines 424 and 476).
- Renamed
- learning_resources/etl/utils_test.py
- Added a parameterize decorator for 'folder' to test both 'folder' and 'static' cases (line 175).
- Modified the source path in
test_transform_content_files
to include the folder parameter (line 210). - Added logic to assert the correct
edx_module_id
based on the folder type (lines 229-236). - Updated assertions to use
edx_module_id
instead ofedx_block_id
(lines 244 and 255).
- learning_resources/filters.py
- Renamed
edx_block_id
toedx_module_id
inContentFileFilter
(line 218). - Renamed
filter_edx_block_id
tofilter_edx_module_id
(line 259). - Updated the filter logic to use
edx_module_id
(line 261).
- Renamed
- learning_resources/filters_test.py
- Renamed
test_content_file_filter_edx_block_id
totest_content_file_filter_edx_module_id
(line 578). - Updated assertions to use
edx_module_id
instead ofedx_block_id
(lines 580-582, 586, 592).
- Renamed
- learning_resources/migrations/0084_rename_edx_block_id_contentfile_edx_module_id.py
- Created a migration to rename the
edx_block_id
field toedx_module_id
in theContentFile
model.
- Created a migration to rename the
- learning_resources/models.py
- Renamed
edx_block_id
toedx_module_id
in theContentFile
model (line 899).
- Renamed
- learning_resources/serializers.py
- Renamed
edx_block_id
toedx_module_id
in theContentFileSerializer
(line 823).
- Renamed
- learning_resources/serializers_test.py
- Replaced
edx_block_id
withedx_module_id
in the test data (line 478). - Updated assertions to use
edx_module_id
instead ofedx_block_id
(line 577).
- Replaced
- vector_search/constants.py
- Replaced
edx_block_id
withedx_module_id
in the constants (lines 17 and 74).
- Replaced
- vector_search/serializers.py
- Replaced
edx_block_id
withedx_module_id
inContentFileVectorSearchRequestSerializer
(lines 232 and 235).
- Replaced
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command>
or @gemini-code-assist <command>
. Below is a summary of the supported commands.
Feature | Command | Description |
---|---|---|
Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/
folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Trivia time!
What is the purpose of Django migrations?
Click here for the answer
Django migrations are used to propagate changes you make to your models (adding a field, deleting a model, etc.) into your database schema. They're Django's way of versioning your database schema.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
The pull request focuses on renaming edx_block_id
to edx_module_id
across the codebase, along with some related logic adjustments. The changes seem well-organized and address the intended purpose. However, a few points could be improved for clarity and consistency.
Merge Readiness
The pull request is mostly in good shape, but there are a few minor points that could be addressed before merging. I am unable to directly approve this pull request, and recommend that others review and approve this code before merging. Given the low severity of the comments, this pull request is close to being ready for merge.
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
The pull request renames edx_block_id
to edx_module_id
across the codebase. The changes appear to be consistently applied. No style guide was provided, so I have not made any comments related to style.
Merge Readiness
The pull request appears to consistently rename edx_block_id
to edx_module_id
. I am unable to directly approve this pull request, and recommend that other reviewers also examine the code before merging. Given that there are no comments, I believe that this is in good shape to be merged once other reviewers have had a chance to look at it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good 👍
What are the relevant tickets?
closes https://github.com/mitodl/hq/issues/6849
Description (What does it do?)
This pr updates the edx_block_id field to generate correct ids for objects in the static folder and renames the field to edx_module_id
How can this be tested?
Run
docker-compose run web ./manage.py backpopulate_xpro_files --overwrite
verify that http://api.open.odl.local:8063/api/v1/contentfiles/?edx_module_id=asset-v1%3AxPRO%2BLASERxE1%2BR15%2Btype%40asset%2Bblock%40426e3727-1e5f-4d60-a17a-d25bfc6d0b39-en
returns a content file