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

Bugfix: Format data values for multi-choice fields #48

Merged
merged 3 commits into from Mar 7, 2023

Conversation

nbarnabee
Copy link
Collaborator

This addresses https://phabricator.wikimedia.org/T331262

As I wasn't having any luck creating a custom schema that would correctly handle both strings and lists, I opted to accept a string and convert it into a list.

Unfortunately, Flask did not do a good job of converting the resulting data to a valid JSON, so I had to do it manually.

@nbarnabee
Copy link
Collaborator Author

The latest changes to main appear to have broken this. I'll try to figure out what's gone wrong.

@nbarnabee nbarnabee marked this pull request as draft March 6, 2023 11:14
@nbarnabee nbarnabee marked this pull request as ready for review March 6, 2023 11:20
This addresses https://phabricator.wikimedia.org/T331262

As I wasn't having any luck creating a custom schema that would
correctly handle both strings and lists, I opted to accept a string
and convert it into a list.

Unfortunately, Flask did not do a good job of converting the resulting
data to a valid JSON, so I had to do it manually.
Copy link
Member

@blancadesal blancadesal left a comment

Choose a reason for hiding this comment

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

LGTM. Tiny nit inline, unrelated to the changes in this patch.

api/utils.py Outdated
@@ -8,9 +9,13 @@


def build_request(form_data):
Copy link
Member

Choose a reason for hiding this comment

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

nit: I'd rename this function to build_put_request for more clarity

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have sullied this PR with an unrelated change. I hope you're happy!! 🤣

This change is completely unrelated to this PR.
@nbarnabee nbarnabee merged commit a8bac20 into main Mar 7, 2023
@nbarnabee nbarnabee deleted the convert-data-to-list branch March 7, 2023 14:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants