-
Notifications
You must be signed in to change notification settings - Fork 46
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
fix: exporter sanitizing content to delete #1796
fix: exporter sanitizing content to delete #1796
Conversation
9c73c48
to
3c49e07
Compare
integrated_channels/integrated_channel/exporters/content_metadata.py
Outdated
Show resolved
Hide resolved
3c49e07
to
7901820
Compare
51e9757
to
a920697
Compare
integrated_channels/integrated_channel/exporters/content_metadata.py
Outdated
Show resolved
Hide resolved
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.
LGTM! Thank you for walking through these changes with me.
bc76259
to
c2b498b
Compare
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.
the approach looks good. i would recommend a few tweaks. i'd love to know if we can factor out that need to hold back on the content_last_modified timestamp.
integrated_channels/integrated_channel/exporters/content_metadata.py
Outdated
Show resolved
Hide resolved
integrated_channels/integrated_channel/exporters/content_metadata.py
Outdated
Show resolved
Hide resolved
integrated_channels/sap_success_factors/exporters/content_metadata.py
Outdated
Show resolved
Hide resolved
integrated_channels/integrated_channel/exporters/content_metadata.py
Outdated
Show resolved
Hide resolved
) | ||
|
||
transformer_for_action = ( | ||
getattr(self, f'transform_for_action_{content_metadata_type}_{edx_data_schema_key}', None) |
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.
quick question: In what instance will this be used? Where is this attribute defined?
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.
The intention's to go into the channel specific content metadata exporter and use the defined mappings to tailor and sanitize the code (see https://github.com/openedx/edx-enterprise/blob/master/integrated_channels/sap_success_factors/exporters/content_metadata.py). There we have two different types of mapping methods, transform_{attribute}
and transform_{content_type}_{attribute}
because some external platforms care about and want to distinguish between metadata depending on what type of content it is, ie SAP wants different titles for course runs than for course and programs. In this case we're supporting the old implementation while also allowing for the caller to specify more detail/actions. transform_{content_type}_{attribute}
isn't currently being used by any channel yet but I wanted to add the functionality as we know channels do this content specific mapping already and we may want to support it in an upgrade in the future.
c2b498b
to
c88e335
Compare
integrated_channels/integrated_channel/exporters/content_metadata.py
Outdated
Show resolved
Hide resolved
integrated_channels/sap_success_factors/exporters/content_metadata.py
Outdated
Show resolved
Hide resolved
integrated_channels/integrated_channel/exporters/content_metadata.py
Outdated
Show resolved
Hide resolved
@@ -19,16 +19,6 @@ def __init__(self, enterprise_configuration, client=SAPSuccessFactorsAPIClient): | |||
client=client | |||
) | |||
|
|||
def _transmit_action(self, content_metadata_item_map, client_method, action_name): |
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.
🥇
5207bf9
to
9fc81cb
Compare
9fc81cb
to
ab55127
Compare
Merge checklist:
requirements/*.txt
files)base.in
if needed in production but edx-platform doesn't install ittest-master.in
if edx-platform pins it, with a matching versionmake upgrade && make requirements
have been run to regenerate requirementsmake static
has been run to update webpack bundling if any static content was updated./manage.py makemigrations
has been run./manage.py lms makemigrations
in the shell.Post merge:
(so basically once your build finishes, after maybe a minute you should see the new version in PyPi automatically (on refresh))
make upgrade
in edx-platform will look for the latest version in PyPi.