Cannot nullify notes for an add-on in a collection

kumar303 commented 6 years ago

Describe the problem and steps to reproduce it:

Set add-on notes to null like this example:

POST a body of {"addon":"17432","notes":null} to /api/v3/accounts/account/10631143/collections/food/addons/

What happened?

{"notes":["This field may not be null."]}

What did you expect to happen?

The docs say that notes can be null so I expected this to work.

Anything else we should know?

Maybe the docs are just incorrect?

eviljeff commented 6 years ago

Does just leaving notes out of the request body work?

btw notes is supposed to be optional the same as description is optional for the Collection itself - do you send description = null in that case?

kumar303 commented 6 years ago

Yes, leaving notes out of the request works but I'm not sure how we would be able to update existing notes to effectively delete them in that case. Are we expected to set notes to an empty string instead?

kumar303 commented 6 years ago

I was mostly thinking of PATCH requests in that last comment

eviljeff commented 6 years ago

Looks like setting null \ None is generally prevented in DRF http://www.django-rest-framework.org/api-guide/fields/#allow_null - though I'm not sure how you're supposed to clear optional values otherwise 😕

eviljeff commented 6 years ago

Some comments before we make any changes -

setting an empty string will work, though obviously an empty string is different than null, and you'd get back that empty string if you queried the collection again (not sure if frontend treats null differently than '')
setting null as part of a full dict of localisations should work - i.e. "description": {"en-US": null} rather than "description": null with lang=en-US param in the request url.

In the latter example, what would the expected behaviour be for a object with multiple localisations anyway? e.g. if Collection.description = {"en-US": "stuff", "fr-FR": "le stúff"}; would sending description=None with lang=fr-FR just clear the French localisation or all of them?

eviljeff commented 6 years ago

Notes from investigating: Hacking around allows_null selectively worked out okay. https://github.com/eviljeff/olympia/tree/7832-null-values-clear-translation-field Trying to have the Translation be deleted if localized_string == None worked out less okay. Doing in in the TranslationSerializerField was too high level because it doesn't know about the Translation object; doing it in Translation was too low level and broke all the things; TranslationDescriptor seems like the place to do it, but it wasn't clear how it actually worked and fitted into the system.

Maybe just hacking the API so it accepts setting None to an existing Translation (where not required) is enough? @diox because he got the first reading of the above notes.

bobsilverberg commented 6 years ago

@eviljeff I'm about to start working on the feature to edit and/or delete notes for an add-on in a collection, so I was wondering where this stands. I can see how it's basically the same issue as we've discussed previously for review text. Is simply making the notes field into a non-localized text field an option? I know that's not trivial, and would require a migration, but it seems like maybe that would be better than trying to hack around the issue. I don't imagine that we actually need notes about a collection add-on to be localizable, do we @jvillalobos?

bobsilverberg commented 6 years ago

It might be that we can simply set the notes field to '' and make the front-end smart enough to know that that means there are no notes. I suppose that's the simplest fix for this.

jvillalobos commented 6 years ago

We don't need them to be localizable, no.

bobsilverberg commented 6 years ago

This has been replaced by mozilla/addons#5716.

mozilla / addons