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

add presets to ContextFormSerializer #176

Merged
merged 6 commits into from
Feb 21, 2017

Conversation

mgu
Copy link
Contributor

@mgu mgu commented Feb 15, 2017

refs #173

@@ -4,6 +4,7 @@ RenderContextSerializer.test_queryset:
- db: SELECT ... FROM "formidable_item" WHERE "formidable_item"."field_id" IN (...) ORDER BY "formidable_item"."order" ASC
- db: SELECT ... FROM "formidable_validation" WHERE "formidable_validation"."field_id" IN (...)
- db: SELECT ... FROM "formidable_default" WHERE "formidable_default"."field_id" IN (...)
- db: 'SELECT ... FROM "formidable_preset" WHERE "formidable_preset"."form_id" = #'
Copy link
Contributor

Choose a reason for hiding this comment

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

it adds a SQL query. I'd be glad to have it retrieved using a "select_related" queryset

Copy link
Contributor

Choose a reason for hiding this comment

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

select_related will not work. It's a prefetch_related we want to have ,and the prefetch will actually generate one more queries.

But @brunobord is right, if you actually have more than one presets , multiple queries will be generated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

do you mean I should prefetch preset arguments ?

Copy link
Contributor

@moumoutte moumoutte left a comment

Choose a reason for hiding this comment

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

I would like to have more consistent data test. I know you have lake of tools in order to provide it and check consistency , so I apologize.


preset1 = form.presets.create(slug='foo', message='message1')
preset1.arguments.create(slug='arg_field', field_id='field_A')
preset1.arguments.create(slug='arg_value', value='42')
Copy link
Contributor

Choose a reason for hiding this comment

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

OMG We can create a presets argument linked to nothing.... Seems more important to resolve this issue

#172


form = Formidable.objects.create(label='my_test_form')

preset1 = form.presets.create(slug='foo', message='message1')
Copy link
Contributor

Choose a reason for hiding this comment

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

And we can create a presets we actually is not implemented....

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, I expect to have a presets which actually working in order to avoid future explosion when we will add check on existing Presets (Hell yeah, I'm confident !)

Please create a comparison Presets with three arguments,

The field argument, with its little name "left" , the operator argument with its little name, "operator" and the last one but not least the value you want to compare.

@@ -328,6 +328,62 @@ class TestForm(FormidableForm):
with django_perf_rec.record(path='perfs/'):
serializer.data

def test_presets(self):

form = Formidable.objects.create(label='my_test_form')
Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, let me introduce you FormidableForm , our Python Builder in order to generate Formidable objects.

I don't know how you are familiar with Django Form, but Formidable project provides a way to generate data test easily. In order to have more consistent data, I suggest to have a Form with appropriate field which actually match your presets Arguments.

So, you can start to import (if its not the case),

from formidable.forms import FormidableForm, fields

Begin with declaring a new form in your method, and add appropriate fields.

class MyAwesomeForm(FormidableForm):
     Field_A = fields.IntegerField()

Then you are able to generate the appropriate Formidable objects

formidable = MyAwesomeForm.to_formidable(label='my_test_form')

Now you can plug the validations presets in it.

Mickaël Guérin added 5 commits February 21, 2017 18:19
Improve tests to use this new syntax

```
class MyTestForm(FormidableForm):
    first_name = fields.CharField()
    last_name = fields.CharField()

    class Meta:
        presets = [
            ConfirmationPresets(
                [PresetBoundArgument('left', field_id='first_name'),
                 PresetBoundArgument('right', value='Obi-Wan')],
                message='first'),
            ConfirmationPresets(
                [PresetBoundArgument('left', field_id='last_name'),
                 PresetBoundArgument('right', value='Kenobi')],
                message='last')
        ]
```
@mgu mgu force-pushed the 173_ContextFormSerializer_misses_presets_fields branch from 63f3d25 to 2f860ad Compare February 21, 2017 17:23
@mgu mgu requested a review from moumoutte February 21, 2017 17:32
@mgu mgu added the 3 - Review label Feb 21, 2017
@mgu mgu requested a review from brunobord February 21, 2017 17:32
@mgu mgu self-assigned this Feb 21, 2017
Copy link
Contributor

@moumoutte moumoutte left a comment

Choose a reason for hiding this comment

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

Really Nice work, many thanks. This contribution is highly appreciate.

@moumoutte moumoutte merged commit e2e1690 into master Feb 21, 2017
@moumoutte moumoutte deleted the 173_ContextFormSerializer_misses_presets_fields branch February 21, 2017 17:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants