Skip to content

Upgrade DRF#175

Merged
feanil merged 4 commits intomainfrom
farhan/upgrade-drf
Apr 5, 2024
Merged

Upgrade DRF#175
feanil merged 4 commits intomainfrom
farhan/upgrade-drf

Conversation

farhan added a commit that referenced this pull request Apr 1, 2024
@farhan farhan force-pushed the farhan/upgrade-drf branch from 125ff5f to a71b5ba Compare April 3, 2024 07:43
@farhan farhan force-pushed the farhan/upgrade-drf branch from 2f15a9e to 0fc4faa Compare April 4, 2024 08:59
@farhan farhan self-assigned this Apr 4, 2024
@farhan farhan requested review from awais786 and irtazaakram April 5, 2024 07:13
@feanil feanil requested a review from ormsbee April 5, 2024 13:16
Copy link
Contributor

@feanil feanil left a comment

Choose a reason for hiding this comment

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

I don't think we want to switch this from an enum to text choices, especially not translated, any translation should be happening upstream of the database. @ormsbee can you take a look?

@feanil
Copy link
Contributor

feanil commented Apr 5, 2024

I think instead you need to update the serializer here: https://github.com/openedx/openedx-learning/blob/1333cf6421e35b7e621fd5e10fc91c6a6f373634/openedx_tagging/core/tagging/rest_api/v1/serializers.py#L349

Currently it's trying to use the default serializer but that won't work with a tuple so you might need to make a custom serializer for just that field.

@ormsbee
Copy link
Contributor

ormsbee commented Apr 5, 2024

@ChrisChV, @pomegranited, @bradenmacdonald: Could one of you please take a look at this? I'm not clear on the implications.

@bradenmacdonald
Copy link
Contributor

@feanil

I don't think we want to switch this from an enum to text choices, especially not translated, any translation should be happening upstream of the database.

I think it's unimportant but OK to translate this - the translations will show up in the Django Admin (only). Though generally I'm not in the habit of translating Django admin stuff for Open edX.

Currently it's trying to use the default serializer but that won't work with a tuple so you might need to make a custom serializer for just that field.

There's no tuple involved - it's still a CharField and before and after this change it should be just a simple string value like "error" or "success" that's stored in MySQL or returned from the REST API. (Though in the middle, before this change, the Python-land view of this value was a special enum value object, not a plain string.)

I don't think any changes to the serializer are needed?

I reviewed the diff here - looks good. And since our REST API tests like

https://github.com/openedx/openedx-learning/blob/2ba5cf4cb26b13ff395d7010a9d0399fd3091b31/tests/openedx_tagging/core/tagging/test_views.py#L2621

are passing unchanged, this is good with me.

Copy link
Contributor

@feanil feanil left a comment

Choose a reason for hiding this comment

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

@bradenmacdonald thanks for taking a look, I missed that the initial model was also a Charfield mis-read the choices bit. Thanks for the clarification.

@feanil feanil merged commit 29f011c into main Apr 5, 2024
@feanil feanil deleted the farhan/upgrade-drf branch April 5, 2024 18:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

6 participants