Skip to content
This repository has been archived by the owner on Feb 1, 2024. It is now read-only.

Handle None and string values in Excel percent column #1822

Merged
merged 1 commit into from Apr 27, 2022

Conversation

jwalgran
Copy link
Contributor

@jwalgran jwalgran commented Apr 27, 2022

Overview

This change attempts to prevent exceptions when there are non-numeric values in an Excel column that mostly contains numbers formatted as percentages.

In addition we fix the handling of empty cells so that a None value is not
transformed into the string "None".

Connects https://github.com/open-apparel-registry/open-apparel-registry-clients/issues/85

Testing Instructions

  • Check out develop
  • Run ./scripts/resetdb
  • Log in as c2@example.com and attempt to submit. percent-test-with-empty-and-string.xlsx
    Verify that an exception is raised
  • Check out this branch bugfix/jcw/handle-none-and-string-in-percent-col
  • Try submitting the workbook again and verify that it succeeds
  • Use ./tools/batch_process {id} to process the list. Verify that there are 2 expected parse failures.
  • Browse http://localhost:6543/settings select Embed and enable the percent_female_workers field and set the embed size
  • Use the embed test at https://open-apparel-registry.github.io/embeded-map-test-full-width.html?host=localhost:6543&protocol=http&contributors=1 and verify that the facilities have correct values
    • KH EXPORTS has a product type but non of the other examples here has a product type of "None"
    • KH EXPORTS INDIA PRIVATE LTD shows 60% for percent_female_workers
    • SAGE CREATIONS shows fifteen percent for percent_female_workers
    • GAURAV does not have percent_female_workers value
    • CENTURY OVERSEAS has a parse error due to missing address
    • JIVA DESIGNS does not have a number of workers value but does have 18% for percent_female_workers

Checklist

  • fixup! commits have been squashed
  • CI passes after rebase
  • CHANGELOG.md updated with summary of features or fixes, following Keep a Changelog guidelines

@jwalgran jwalgran force-pushed the bugfix/jcw/handle-none-and-string-in-percent-col branch from b3542d8 to 988e69f Compare April 27, 2022 19:33
@jwalgran jwalgran requested a review from TaiWilkin April 27, 2022 19:49
@jwalgran jwalgran marked this pull request as ready for review April 27, 2022 19:50
@jwalgran jwalgran requested review from emilyhu0106 and removed request for TaiWilkin April 27, 2022 20:18
@jwalgran jwalgran assigned emilyhu0106 and unassigned TaiWilkin Apr 27, 2022
Copy link
Contributor

@emilyhu0106 emilyhu0106 left a comment

Choose a reason for hiding this comment

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

All the tests pass well! I also tried a few other wrong formats and they all work as expected. Nice work!

@@ -53,6 +53,8 @@ def get_xlsx_sheet(file, request):


def format_percent(value):
if value is None or isinstance(value, str):
Copy link
Contributor

Choose a reason for hiding this comment

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

Good call for considering string here

@emilyhu0106 emilyhu0106 assigned jwalgran and unassigned emilyhu0106 Apr 27, 2022
This change attempts to prevent exceptions when there are non-numeric values in
an Excel column that mostly contains numbers formatted as percentages.

In addition we fix the handling of empty cells so that a None value is not
transformed into the string `"None"`.
@jwalgran jwalgran force-pushed the bugfix/jcw/handle-none-and-string-in-percent-col branch from 988e69f to 8adad6e Compare April 27, 2022 21:00
@jwalgran
Copy link
Contributor Author

Thanks for the review.

@jwalgran jwalgran merged commit f269055 into develop Apr 27, 2022
@jwalgran jwalgran deleted the bugfix/jcw/handle-none-and-string-in-percent-col branch April 29, 2022 19:48
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants