Skip to content

Use EnumSupportSerializerMixin#850

Merged
GergiH merged 1 commit intodevelopfrom
feature/fix-enum-in-csv-response
Sep 9, 2020
Merged

Use EnumSupportSerializerMixin#850
GergiH merged 1 commit intodevelopfrom
feature/fix-enum-in-csv-response

Conversation

@thenav56
Copy link
Copy Markdown
Member

@thenav56 thenav56 commented Aug 21, 2020

Changes

  • Avoid different value types for EnumField among CSV and JSON Response.
  • EnumField in JSON/CSV respones will provide raw value (integer).
  • Add <enum-field-name>_display for each field for label value. (Except for Region, using region_name for EnumField name)

API Changes diff

Issues/PR related to:

@nanometrenat
Copy link
Copy Markdown
Contributor

What ticket does this change relate to? Be great to always have a ticket ref please :) thanks!

@thenav56
Copy link
Copy Markdown
Member Author

What ticket does this change relate to? Be great to always have a ticket ref please :) thanks!

Hi @nanometrenat, I am afraid I don't have the knowledge if we have a ticket for this. But for the rollback fix, we did in PR #794, this PR provides the necessary fix to the issue.

Explanation on the issue:
django-enumfields is used in go-api for static choice fields. Basically integer values are saved in the database, each integer value represents a string value (which is visible/understandable to users). The string values (labels) may change over time like MENA -> Middle East & North Africa but the original meaning will not. So, for logical uses users have to use integer value and use label value for display.

Unfortunately, the JSON and CSV responses have a different value for enum fields resulting in raw value (integer) in JSON response and label value (string) in the CSV response.

This PR fixes the issue by providing the raw value (integer) in CSV responses and additionally provides a label value in <enum-field-name>_display field in response to both JSON and CSV responses.

@thenav56
Copy link
Copy Markdown
Member Author

thenav56 commented Sep 2, 2020

@nanometrenat @LukeCaley
Here is the documentation providing which fields are added/modified in this PR. Could you guys let me know if this makes sense and where we should put this documentation?

@nanometrenat
Copy link
Copy Markdown
Contributor

@nanometrenat @LukeCaley
Here is the documentation providing which fields are added/modified in this PR. Could you guys let me know if this makes sense and where we should put this documentation?

@JonathanGarro tagging you in for info

@nanometrenat
Copy link
Copy Markdown
Contributor

@thenav56 just looking further through your document - can you please confirm, for the Countries API CSV output (https://goadmin.ifrc.org/api/v2/country/?record_type=1&limit=300&format=csv) will the Label of the Region be presented as part of these changes? (Region is in lots of endpoints, not just country). e.g. Current csv output has Region=0, it would be great to have Region=Africa (or RegionLabel, or whatever it is we want to call it) as that will make it a lot more usable. Thanks
cc @anamariaescobar @JonathanGarro

- Avoid different value type among CSV and JSON Response.
@thenav56 thenav56 force-pushed the feature/fix-enum-in-csv-response branch from 7cc28e2 to 40072c6 Compare September 9, 2020 05:39
@thenav56
Copy link
Copy Markdown
Member Author

thenav56 commented Sep 9, 2020

@nanometrenat Current PR scope is to change enum fields to raw+label (from the label) in the API. The region you mentioned is related to the database primary key rather then enum field (sorry for the confusion).
I would recommend doing that separately as it will add fields not related to enum fields.

Let's create a new issue regarding additional fields required in the APIs.
Let me know what you think.

@thenav56 thenav56 marked this pull request as ready for review September 9, 2020 05:42
@thenav56 thenav56 requested review from GergiH and batpad September 9, 2020 05:43
Copy link
Copy Markdown
Collaborator

@GergiH GergiH left a comment

Choose a reason for hiding this comment

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

I think we'll have to rigorously test the frontend to see if everything works as expected after this is deployed. The Tableau WDC might need some adjustments too but we'll see.

@GergiH GergiH merged commit 8fe77b5 into develop Sep 9, 2020
@thenav56 thenav56 deleted the feature/fix-enum-in-csv-response branch September 9, 2020 10:01
@GergiH
Copy link
Copy Markdown
Collaborator

GergiH commented Sep 9, 2020

@thenav56 This is deployed to staging.

@nanometrenat
Copy link
Copy Markdown
Contributor

@nanometrenat Current PR scope is to change enum fields to raw+label (from the label) in the API. The region you mentioned is related to the database primary key rather then enum field (sorry for the confusion).
I would recommend doing that separately as it will add fields not related to enum fields.

Understood - thanks for clarifying!

Let's create a new issue regarding additional fields required in the APIs.
Let me know what you think.

Don't want to add to complexity of this translations release, so agree that a separate ticket should be logged if desired to bring the primary key labels through. For several database items (e.g. regions) there is a separate endpoint that people can call to get the list/labels anyway so not urgent for those. Will leave to @JonathanGarro to confirm if a ticket needs to be raised for further future changes. Cheers

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.

4 participants