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

Solution proposed in 656dca3 for issue #187 is not user-friendly nor usable in frontend #211

Closed
caumons opened this issue Oct 21, 2013 · 4 comments
Assignees

Comments

@caumons
Copy link

caumons commented Oct 21, 2013

(I'm opening a new issue because #187 has been closed and I can't reopen it).

I've just tried the new 0.7 release and I've seen that you've included a None CheckboxField. If it's checked, then you insert a None value for that field into the database, else an empty string '' if no value was present.

I think this is a hack, instead of a real solution. Final users don't have to even know if the value has to be saved as a NULL SQL value or an empty string. This should be done automatically.

Instead of relying on this field, I'd remove it completely and apply the following simple logic:

  • If the field is empty and nullable, then insert a None value
  • If the field is empty and NOT nullable, then insert an empty string ''

This would be a real solution. Transparent for the user and this would finally fix the issue.

Some months ago, I did a pull request #188 which hasn't been discussed, nor merged. In that pull request I introduced a new form to allow users write the translations for the languages they wish. Forcing them to introduce at least one translation for required translatable fields. So, I can't force them to even see this None checkbox. They'd think... what the hell is this? (In fact, I thought so when I've seen it for the first time).

zlorf added a commit that referenced this issue Oct 23, 2013
This will hopefully at last resolve the problem with nullable CharFields.
@zlorf
Copy link
Collaborator

zlorf commented Oct 23, 2013

I agree, that this 'None' checkbox can be annoying in frontend.
Here is a branch ultimate-null-formfield-solution with proposal:
https://github.com/deschler/django-modeltranslation/tree/ultimate-null-formfield-solution

Summary:
Generally formfields behave just like you described:

  • If the field is empty and nullable, then insert a None value
  • If the field is empty and NOT nullable, then insert an empty string ''

But it can be overridden (for example, to handle #187 - unique constraint) by new setting in TranslationOptions:

class NTTranslationOptions(TranslationOptions):
    fields = ('cnn', 'cna', 'cu', 'inn', 'ina', 'iu')
    empty_values = {'cu': None, 'ina': '', 'iu': 'both'}

None and '' are quite meaningful by their own (what will be saved to database in case of empty field). 'both' will result in what can be seen now - normal widget and None checkbox, which allow to save either empty string or None.

Moreover, one might want to use this 'both' widget, but only in admin - therefore I introduced both_empty_values_fields option in TranslationAdmin.

AND! This only apply to CharFields and it's subclasses. Other fields don't allow two empty values.

@deschler
Copy link
Owner

deschler commented Nov 7, 2013

@zlorf: Ultimate! 👍 I feel this can be merged into master and the 0.7.x branch. Since Django 1.6 is out of the door we need to make a bugfix release because of #214 and i want to include this change.

@deschler deschler closed this as completed Nov 7, 2013
@zlorf
Copy link
Collaborator

zlorf commented Nov 8, 2013

I need to write some words in docs about this, though.

@zlorf zlorf reopened this Nov 8, 2013
@ghost ghost assigned zlorf Nov 8, 2013
@zlorf zlorf closed this as completed in a3a274a Nov 9, 2013
@caumons
Copy link
Author

caumons commented Nov 9, 2013

Hi @zlorf and @deschler! It has taken a lot of time since the bug was initially reported and we finally seem to have a useful solution! It's been a really hard bug to deal with...

I've reviewed the latest commit, where @zlorf has added the docs and I think that it's perfectly explained and if the code works as described, I think that now we have a fully working solution ready for production environments.

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

No branches or pull requests

3 participants