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

Use xlsx file object instead of absolute path #2257

Merged
merged 2 commits into from
Jun 10, 2022
Merged

Conversation

KipSigei
Copy link
Contributor

@KipSigei KipSigei commented Jun 10, 2022

Signed-off-by: Kipchirchir Sigei [email protected]

Changes / Features implemented

  • Upadate to use file object intsead of absolute file path when loading excel workbook

Steps taken to verify this change does what is intended

  • The following exception is being thrown when uploading a form with external choices
File "/srv/onadata/./onadata/apps/viewer/models/data_dictionary.py", line 246, in set_object_permissions
    f = sheet_to_csv(instance.xls, "external_choices")
  File "/srv/onadata/./onadata/apps/viewer/models/data_dictionary.py", line 76, in sheet_to_csv
    workbook = openpyxl.load_workbook(xls_content.path)
  File "/usr/local/lib/python3.9/dist-packages/django/db/models/fields/files.py", line 59, in path
    return self.storage.path(self.name)
  File "/usr/local/lib/python3.9/dist-packages/django/core/files/storage.py", line 128, in path
    raise NotImplementedError("This backend doesn't support absolute paths.")

Side effects of implementing this change

Before submitting this PR for review, please make sure you have:

  • Included tests
  • Updated documentation

Closes #2258

@KipSigei KipSigei changed the title Use xls file object instead of absolute path Use xlsx file object instead of absolute path Jun 10, 2022
Copy link
Member

@ukanga ukanga left a comment

Choose a reason for hiding this comment

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

Why is the change necessary? Do we have a test that should have captured this issue? Maybe we are missing a test.

@KipSigei
Copy link
Contributor Author

@ukanga i was unable to test this locally, there already exists a test case for external choices form and it seems to be passing

Signed-off-by: Kipchirchir Sigei <[email protected]>
@KipSigei KipSigei merged commit d811017 into main Jun 10, 2022
@KipSigei KipSigei deleted the fix-external-selects-bug branch June 10, 2022 15:05
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.

Error: NotImplementedError("This backend doesn't support absolute paths.")
2 participants