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.

Sign up for GitHub

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

Jump to bottom

OpenConceptLab/ocl_issues#22 | exclude retired concepts and/or mappings from exports #14

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

OpenConceptLab/ocl_issues#22 | exclude retired concepts and/or mappings from exports #14

wants to merge 1 commit into from

Conversation

PatrickCmd
Copy link
Contributor

Retired concepts and/or mappings can now be excluded from concepts and or mappings.

@PatrickCmd PatrickCmd requested a review from snyaggarwal August 3, 2021 08:56
@PatrickCmd PatrickCmd mentioned this pull request Aug 3, 2021
Closed
snyaggarwal
snyaggarwal reviewed Aug 3, 2021
View reviewed changes
core/common/tasks.py Outdated
@@ -56,7 +56,7 @@ def export_source(self, version_id):
version.add_processing(self.request.id)
try:
logger.info('Found source version %s. Beginning export...', version.version)
write_export_file(version, 'source', 'core.sources.serializers.SourceVersionExportSerializer', logger)
write_export_file(self.request, version, 'source', 'core.sources.serializers.SourceVersionExportSerializer', logger)
Copy link
Contributor

Choose a reason for hiding this comment

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

@PatrickCmd these methods should not be dependent on the request object.
passing include_retired=True in export_source directly as a parameter keeps things flexible and the same can be passed to write_export_file. The view should control request query params.
The reason I don't like using request object in write_export_file is it's a very low-level method and if I want to call it anywhere else I need to have a full request object, which is an overkill in my opinion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@snyaggarwal I have worked on this. please do review again.

@PatrickCmd PatrickCmd force-pushed the exclude-retired-concepts-and-mappings branch 2 times, most recently from 11e3fa5 to 2c6ad01 Compare August 9, 2021 13:50
@snyaggarwal
Copy link
Contributor

snyaggarwal commented Aug 10, 2021 •
edited
Loading

@PatrickCmd The missing thing is a test to exclude retired concepts

@PatrickCmd PatrickCmd force-pushed the exclude-retired-concepts-and-mappings branch 2 times, most recently from ea03273 to 481d93a Compare August 30, 2021 09:23
snyaggarwal
snyaggarwal reviewed Aug 30, 2021
View reviewed changes
core/common/models.py Outdated
@cached_property
def exclude_retired_export_path(self):
last_update = self.last_child_update.strftime('%Y%m%d%H%M%S')
return self.generic_export_path(suffix="{}_non_retired.zip".format(last_update))
Copy link
Contributor

Choose a reason for hiding this comment

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

non_retired -> unretired

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

core/common/utils.py Outdated
@@ -330,6 +336,8 @@ def write_export_file(
logger.info('Done compressing. Uploading...')

s3_key = version.export_path
if not include_retired:
Copy link
Contributor

Choose a reason for hiding this comment

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

export_path is an expensive call, since its calculates on runtime so use it only if needed
s3_key = version.export_path if include_retired else version.exclude_retired_export_path

Copy link
Contributor Author

Choose a reason for hiding this comment

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

addressed

@snyaggarwal
Copy link
Contributor

@PatrickCmd This looks good. Few things that are still left:

  1. GET /export/?includeRetired=true should work as is (export_path) but GET /export/?includeRetired=false should use new exclude_retired_export_path
  2. An integration test for ?includeRetired=false, you can use the existing integration test ExportSourceTaskTest.test_export_source

@PatrickCmd PatrickCmd force-pushed the exclude-retired-concepts-and-mappings branch from 481d93a to 7a97a85 Compare August 31, 2021 11:05
@PatrickCmd PatrickCmd force-pushed the exclude-retired-concepts-and-mappings branch from 7a97a85 to e552dc8 Compare September 1, 2021 20:31
@PatrickCmd PatrickCmd force-pushed the exclude-retired-concepts-and-mappings branch from e552dc8 to fd2297d Compare September 13, 2021 20:01
@PatrickCmd PatrickCmd force-pushed the exclude-retired-concepts-and-mappings branch 2 times, most recently from 0fc07a3 to cf7a6aa Compare October 1, 2021 10:53
@PatrickCmd PatrickCmd force-pushed the exclude-retired-concepts-and-mappings branch from cf7a6aa to 02f9138 Compare October 12, 2021 15:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@snyaggarwal snyaggarwal snyaggarwal left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@PatrickCmd @snyaggarwal