openedx / openedx-learning

GNU Affero General Public License v3.0
5 stars 8 forks source link

Upgrade DRF #175

Closed farhan closed 6 months ago

farhan commented 6 months ago

Description: Bump DRF to v3.15.1

References:

feanil commented 6 months ago

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 commented 6 months ago

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

bradenmacdonald commented 6 months ago

@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.