Dmitri-Sintsov / django-jinja-knockout

Django datatables and widgets, both AJAX and traditional. Display-only ModelForms. ModelForms / inline formsets with AJAX submit and validation. Works with Django templates.
https://django-jinja-knockout.readthedocs.org/
Other
144 stars 29 forks source link

Default selection/Not required selection ForeignKeyGridWidget #14

Closed bobsburgers closed 3 years ago

bobsburgers commented 3 years ago

I have 2 issues and solving either will do what I need. Everything shows up and works. I might be asking about features, but I feel like I am missing something obvious.

Marking fields as not required does not seem to work. The text is not bold so I can tell there is a change. I still get Select a valid choice. That choice is not one of the available choices.

'KoGridRelationView' referenced below. registrar = models.ForeignKey(AccountRegistrarModel, on_delete=models.CASCADE, blank=True, null=True, verbose_name='Default Registrar') `widgets = {'registrar': ForeignKeyGridWidget(grid_options={ 'pageRoute': 'account_registrar_fk_widget', 'dialogOptions': {'size': 'size-wide'},

Override default search field label (optional):

            'searchPlaceholder': 'Search For account',
            'required': False,
        }),`

I see you did that here on line 38. but its for a different thing i think? https://github.com/Dmitri-Sintsov/djk-sample/blob/3805ad9b9731df6933feda86a75e9aa3b616399a/club_app/forms.py

other thing I would like to do is set default options. Like there are 20 themes but the user has a favorite. This would also solve the above problem in a messy patch way with a universal default of none option.

Everything else has been so easy once I found it. But I have looked for a day or two now and I cant figure it out. Hope to make some pull requests soon best I can.

Dmitri-Sintsov commented 3 years ago

Line 38 belongs to ClubForm.add_tag_set_checkbox which is disabled (commented out) by default:

# self.add_tag_set_checkbox()

Method add_tag_set_checkbox is an example of standard Django CheckboxSelectMultiple widget usage.

Please use MultipleKeyGridWidget for the many to many relation field, instead.

bobsburgers commented 3 years ago

I think i asked the question bad. Sorry about that. I was able to figure it out based on what you said. But do not have a good solution yet.

Using the below works if you add something to the field. if you dont it returns Select a valid choice. That choice is not one of the available choices.

even with required=False.

Far as I can tell the issue is that an empty ForeignKeyGridWidget returns '0' as a string which does not match the options here for empty value in django model so it thinks your trying to select FK with id of 0. EMPTY_VALUES = (None, '', [], (), {})

self.fields['registrar'] = forms.ModelChoiceField( widget= ForeignKeyGridWidget(grid_options={ 'pageRoute': 'account_registrar_fk_widget', }), queryset=AccountRegistrarModel.objects.all(), required=False, )

Dmitri-Sintsov commented 3 years ago

Fixed in 20b50c061f1933ca0c7e9af6fe33b19e841e98d3.

Please let me know whether it works for you.

Dmitri-Sintsov commented 3 years ago

As for the default choices, foreign keys usually do not specify these, but in case one wants, he/she could override queryset arg of the widget.

bobsburgers commented 3 years ago

As for the default choices, foreign keys usually do not specify these, but in case one wants, he/she could override queryset arg of the widget.

Yea using this other form of adding the widget it was really clear i could do it in all the normal ways. That's part of what makes your implementation so cool to me.

Far as your code change I have not gotten to test it but hope to in the next 2 work days. Far as I can tell it addresses the exact issue. And since all my template is based off your template if it works there it will work for me.

Ill be sure to do a better review. For now ill mark closed. And if you like ill make a pull on your example with that added some way and maybe some other suggestions. At least do some of the grunt work for you.

Thanks for your super quick response!

bobsburgers commented 3 years ago

I spoke to soon. whenever there is a blank value AND another field failed save. there is now this error. To me it look like it really wants it to be None instead of blank str. Its not a huge deal.

i tried changing line 282 of widgets.py to None did not fix it.

You can see in the image 3 of the same foreign key form widgets. one has required the other 2 do not. If all 3 filled out it works fine. If the not required ones are empty and the required ones filled it works. If the required one is empty and the not required one are filled it returns 'select a valid choice' which is as expected. if one or more of the not required is empty AND a required field is empty it returns the error image Field 'id' expected a number but got ''.

\django\db\models\fields\__init__.py, line 1774, in get_prep_value return validators_ def get_prep_value(self, value): value = super().get_prep_value(value) if value is None: return None try: return int(value) … except (TypeError, ValueError) as e: raise e.__class__( "Field '%s' expected a number but got %r." % (self.name, value), ) from e def get_internal_type(self):

Dmitri-Sintsov commented 3 years ago

Your error message indicates an error in ModelField. ModelField level is too low level to translate empty string '' into None. Such mapping should be performed at ModelForm field level.

https://docs.djangoproject.com/en/2.2/_modules/django/forms/fields/ states that Field EMPTY_VALUES should skip validators.

Perhaps one should implement custom clean method for the form / formset, to translate ` intoNone` properly. However it's really strange that such action is not performed automatically by Django.

Maybe I will try to create the similar form with three foreign key fields some time later.

Let me know whether custom Form clean worked for you.

bobsburgers commented 3 years ago

I know custom form clean works because it was one of the second or third things I tried. But it just felt wrong. Like i should not have to be doing that. Or there should be a better way. Ultimately I abandoned entirely since my solution did not solve all the required issues which you solved with your recent commit.

Ok ill implement that in a fork of the demo and send it over when i get a chance this week. Thanks!

Dmitri-Sintsov commented 3 years ago

Please try 173a5d1 or (almost the same) 9704c97.

bobsburgers commented 3 years ago

Please try 173a5d1 or (almost the same) 9704c97.

That works great based on prelim tests! Thanks for your help!