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

add ability to archive forms submitted by a user #24384

Merged
merged 3 commits into from
May 23, 2019

Conversation

mkangia
Copy link
Contributor

@mkangia mkangia commented May 22, 2019

need for this task but thought of adding it as a management command for reuse if needed

@mkangia mkangia requested a review from esoergel as a code owner May 22, 2019 10:50
@mkangia mkangia added the product/invisible Change has no end-user visible impact label May 22, 2019
@mkangia mkangia requested a review from millerdev May 22, 2019 10:52
def handle(self, domain, user_id, **options):
form_accessor = FormAccessors(domain)
form_ids = form_accessor.get_form_ids_for_user(user_id)
print("Found %s forms for user" % len(form_ids))

Choose a reason for hiding this comment

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

E1601 print statement used

Copy link
Contributor

@millerdev millerdev left a comment

Choose a reason for hiding this comment

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

👍 after from __future__ import print_function is added at the top of this file.

if response == 'yes':
with open("archived_forms_for_user_%s.txt" % user_id, 'wb') as log:
for _ids in chunked(with_progress_bar(form_ids), 100):
_ids = list([_f for _f in _ids if _f])
Copy link
Contributor

Choose a reason for hiding this comment

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

Curious why you're using underscore prefix on these variables? That's a convention used to mark a variable as private, but it's not necessary here since this is a local function scope and those names are not accessible outside of this function.

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.

Copy link
Contributor

@millerdev millerdev left a comment

Choose a reason for hiding this comment

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

I assume you're skipping tests with [skip ci] because there are no tests for the code you added? This is generally got a good practice because, even though there are no new tests, it's possible that a change in this PR could cause existing tests to fail for some reason. It's true that a test failure caused by this PR is unlikely, but it's safer to allow tests to run and catch the issue early rather than assuming all is good and only finding out when other people's PRs start failing after this one gets merged into master.

@mkangia
Copy link
Contributor Author

mkangia commented May 22, 2019

@millerdev ya this was an isolated management command, not changing anything in the code base so i thought having a test run was not necessary.

@mkangia mkangia merged commit f892b12 into master May 23, 2019
@mkangia mkangia deleted the mk-archive-form-by-user branch May 23, 2019 07:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
product/invisible Change has no end-user visible impact
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants