-
Notifications
You must be signed in to change notification settings - Fork 4
Provide a tools in order to generate django-form class from json contextualized definition #171
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
Conversation
13e1213
to
bff7624
Compare
demo/tests/test_form_from_schema.py
Outdated
schema = ContextFormSerializer(instance=formidable, context={ | ||
'role': 'jedi' | ||
}).data | ||
form = get_dynamic_form_class_from_schema(schema)() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is it here? a form instance or a class?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From what I understood: get_dynamic_form_class_from_schema(schema)
is a class, so get_dynamic_form_class_from_schema(schema)()
is an instance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this new tool in our toolbox. although, I'd recommend to rename the "form" variable in the test to reflect its true nature: it's a form_class
object, rather than a "ready to use" form instance.
It'll make the tests more readable
formidable/forms/__init__.py
Outdated
|
||
def get_dynamic_form_class_from_schema(schema, field_factory=None): | ||
""" | ||
Schema Already contextualized |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not a fan of this docstring. maybe it could be a bit more consistent, using a simple code sample, for example? And the first line should read:
Return a dynamically generated and contextualized form class
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"[WIP]"
if role: | ||
# The role is previously "prefetch" in order to avoid database | ||
# hit, we don't use a get() method in queryset. | ||
return self.field.accesses.all()[0] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what happens here if the field.accesses.all() is empty?
I'm not sure, but if field.accesses.all()
is a queryset, you can use .first()
that'll eventually return None
if the queryset is empty. (but I don't know if it has side effects)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The role is previously "prefetch" in order to avoid database
hit, we don't use a get() method in queryset"
Not sur first will not hit the database with prefetch related call before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 for .first()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When using "first" instead of "all()[0]"
======================================================================
FAIL: test_queryset_without_role (tests.test_forms.TestDynamicForm)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/home/moumoutte/projects/django-formidable/demo/tests/test_forms.py", line 305, in test_queryset_without_role
form.get_django_form_class(role='jedi')
File "/home/moumoutte/.virtualenvs/formidable/local/lib/python2.7/site-packages/django_perf_rec/api.py", line 91, in __exit__
self.save_or_assert()
File "/home/moumoutte/.virtualenvs/formidable/local/lib/python2.7/site-packages/django_perf_rec/api.py", line 127, in save_or_assert
assert self.record == orig_record, msg
AssertionError: Performance record did not match for TestDynamicForm.test_queryset_without_role
db: SELECT ... FROM "formidable_field" WHERE "formidable_field"."form_id" = # ORDER BY "formidable_field"."order" ASC
db: SELECT ... FROM "formidable_item" WHERE "formidable_item"."field_id" IN (...) ORDER BY "formidable_item"."order" ASC
db: SELECT ... FROM "formidable_access" WHERE ("formidable_access"."access_id" = # AND "formidable_access"."field_id" IN (...))
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_access" WHERE "formidable_access"."field_id" = # ORDER BY "formidable_access"."id" ASC LIMIT #
+ db: SELECT ... FROM "formidable_access" WHERE "formidable_access"."field_id" = # ORDER BY "formidable_access"."id" ASC LIMIT #
+ db: SELECT ... FROM "formidable_access" WHERE "formidable_access"."field_id" = # ORDER BY "formidable_access"."id" ASC LIMIT #
+ db: SELECT ... FROM "formidable_access" WHERE "formidable_access"."field_id" = # ORDER BY "formidable_access"."id" ASC LIMIT #
db: SELECT ... FROM "formidable_preset" WHERE "formidable_preset"."form_id" = #
It's not. |
Need #173 in order to continue. |
9d21e02
to
c4db3b8
Compare
84673c7
to
18f9120
Compare
|
||
class FormFieldFactory(BaseFormFieldFactory): | ||
|
||
field_map = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you could add a test to ensure that this map has the same keys as field_builder.FormFieldFactory.field_map
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test added in 87d1cac.
Instead of using acronyms, we use verbose name prefixed with `Base`.
dc6bc26
to
87d1cac
Compare
Ready for review once again. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, nice work !
ChangeLog --------- * Added Django 1.10 support (#203). * Dropped Python 3.3 support (#207). * Fixed the swagger doc generation and rendering (#210). * Fix wrong field type for Checkbox (#208). * Don't rely on database ordering in `NestedListSerializer` (#215) * Provide a tools in order to generate django-form class from json contextualized definition (#171)
Use case
When the contextualized schema is already stored in database (First step to reach the versioning directly in formidable) generate the associated django form class.
Used with Contextualized definition
Remaining Standard Fields to Handle
Remaining Format Field to handle