Fix #761: Attributes that depend on party type don't show up on party creation #1076
Conversation
|
|
cadasta/core/form_mixins.py
Outdated
| attr.choice_labels != []): | ||
| chs = list(zip(attr.choices, attr.choice_labels)) | ||
| else: | ||
| # chs = list(map(lambda c: (c, c), attr.choices)) |
There was a problem hiding this comment.
Can you remove the comment here?
| <div class="clearfix form-group{% if field.errors %} has-error{% endif %}"> | ||
| <label class="control-label" for="{{ field.id_for_label }}">{{ field.label }}</label> | ||
| {% if field|field_type == "datefield" %} | ||
| {% render_field field class+="form-control datepicker" %} | ||
| {{ field|add_class:"form-control datepicker"|set_parsley_required }} |
There was a problem hiding this comment.
The set_parsley_required filter is not necessary, I think. You're creating the field instances anyway in AttributeFormMixin, so you could add the attribute to the field's widget's attributes:. Something along the lines of:
field.widget.attrs['parsley_required'] = field.requiredThat way you can get rid of the filter and you can go back to {% render_field field class+="form-control datepicker" %} for this...
There was a problem hiding this comment.
Seems like the Widget is not instantiated at the point I need it to be in the AttributeFormMixin so I'd need code like this to make it work:
if attr.required:
widget_attrs = {'data-parsley-required': 'true'}
widget = field.widget(attrs=widget_attrs) # instatiate the widget and add attrs
args.update({'widget': widget}) # update the initial args on the field with the customized widget
This works but complicates the create_model_fields method on the mixin.. at least the filter is reusable?
There was a problem hiding this comment.
Argh, ok if the widget is not yet initialised then we shouldn't mess around with that. There might be weird side effects of that.
There was a problem hiding this comment.
Ok, have left this as is..
cadasta/core/form_mixins.py
Outdated
| else: | ||
| if attr.required and new_item: | ||
| args['required'] = True | ||
| if len(attr.default) > 0 and len( |
There was a problem hiding this comment.
This is throwing an error when an integer field is required: object of type 'int' has no len()
There was a problem hiding this comment.
Crapola! well spotted, thanks..
| $('.party-gr .form-control').prop('disabled', 'disabled'); | ||
| $('.party-in .form-control').prop('disabled', 'disabled'); | ||
| } | ||
|
|
There was a problem hiding this comment.
These could be refactored down to a single function:
function enableConditions(val) {
var types = ['co','gr','in']
types.splice(types.indexOf(val), 1)
$('.party-' + val).removeClass('hidden');
$('.party-' + val + ' .form-control').prop('disabled', '');
for (i in types) {
$('.party-' + types[i]).addClass('hidden')
$('.party-' + types[i] + '.form-control').prop('disabled', 'disabled');
}
}
which could shorten toggelStates to:
if (val === '') {
disableConditionals();
} else {
enableConditions(val);
toggleParsleyRequired(val);
}
Which is maybe less clear, but if we add more party types in the future you'd only have to change one value.
There was a problem hiding this comment.
Nice one, thanks.. wasn't happy with that stuff..
| assert party.name == 'The Beatles' | ||
| assert party.type == 'GR' | ||
| print('++++ BLAH 3 ++++') |
There was a problem hiding this comment.
Do you still need these print statements?
There was a problem hiding this comment.
These have been removed already..
There was a problem hiding this comment.
Oh right, red means delete. Whoops!
|
|
||
| {% for field in form %} | ||
| {% if "party::in" in field.name %} | ||
| <div class="form-group{% if field.errors %} has-error{% endif %} party-in hidden"> |
There was a problem hiding this comment.
Since the rest of the form is the same, is there a way to wrap the if statements around just the div with the party-{type} class?
There was a problem hiding this comment.
Ok, this is better now I think..
| ) | ||
|
|
||
|
|
||
| class AttributeModelForm(AttributeFormMixin, ModelForm): |
There was a problem hiding this comment.
This is just to satisfy my own curiosity: Why do you need both the Form and ModelForm?
There was a problem hiding this comment.
We need attribute forms that don't inherit from ModelForm. If you look at the existing TenureRelationshipForm in spatial/forms.py you'll see that it can't inherit from django-jsonattrs AttributeModelForm because its creating instances of two models, Party and TenureRelationship, so I've added an AttributeForm that subclasses a plain django Form. There might be a way to merge them into a singe AttributeForm but I think its cleaner to have two to choose from.
There was a problem hiding this comment.
Oooh interesting. That makes sense.
cadasta/core/mixins.py
Outdated
| field = model._meta.get_field(selector_field.partition('.')[0]) | ||
| if field.choices: | ||
| choices = [ | ||
| (field.name, choice[0]) for choice in field.choices] |
There was a problem hiding this comment.
You're creating an array of 2-tuples here where the first tuple item is field.name but later, you don't actually use the field name since you are only referencing the choice[1] item. Maybe you can simplify choices to just a simple array.
There was a problem hiding this comment.
You're right.. will fix this.
cadasta/core/mixins.py
Outdated
| content_type=content_type, | ||
| selectors=conditionals) | ||
| if schemas: | ||
| key = '{value}'.format(value=choice[1]) |
There was a problem hiding this comment.
Isn't choice[1] already a string? Why do you need to do this string formatting?
|
|
||
|
|
||
| class AttributeForm(AttributeFormMixin, Form): | ||
| def add_attribute_fields(self, content_type): |
There was a problem hiding this comment.
This method is exactly the same as AttributeModelForm's corresponding method except the latter already knows its content_type based on the form's model field. Why not DRY these two methods into one method in AttributeFormMixin?
There was a problem hiding this comment.
Making content_type an optional parameter and checking for meta.Model would probably allow this. However, I prefer having two separate form subclasses. I think its clearer to subclass from AttributeForm or AttributeModelForm rather than from AttributeFormMixin or AttributeModelForm but that's just a personal preference.. I don't feel we have to apply the DRY principle in every case..
|
|
||
| class SchemaSelectorMixin(): | ||
|
|
||
| def get_attributes(self, project): |
There was a problem hiding this comment.
This method provides an easy way to get the attributes for any model that have attributes by creating a big dictionary. But for forms that only handle 1 or 2 models, you are doing a lot of data manipulation including possibly unneeded database calls via Schema.objects.lookup. Can't we have a more lightweight solution for cases where the view doesn't need to know the attributes of the whole project?
There was a problem hiding this comment.
You're right about this, I think this mixin could be improved. There should be a way to just query by content_type without having to query for all project attributes. Also there should be some simple caching mechanism to avoid attribute lookups altogether if its already been done. I don't want to make these changes at this point as this mixin is used across forms, exports and imports which are all in progress. I'll open an issue once all the pending PR's are merged.
|
|
||
|
|
||
| @register.filter(name='display_choice_verbose') | ||
| def display_choice_verbose(field): |
There was a problem hiding this comment.
I'm not sure, but does this filter work well with translations?
There was a problem hiding this comment.
Nevermind. I checked for myself and found out that this code is OK. However, the party type labels are themselves not coded properly for lazy translation in the platform and so I have submitted PR #1093 to fix that.
| {% csrf_token %} | ||
|
|
||
| {% for field in form %} | ||
| {% if field.name|slice:":7" == "party::" %} |
There was a problem hiding this comment.
What about attributes that apply to any type of party? It seems these attributes won't get displayed in this template.
There was a problem hiding this comment.
Nevermind. I checked for myself and confirmed that this works.
| return kwargs | ||
|
|
||
| def get_context_data(self, *args, **kwargs): |
There was a problem hiding this comment.
This method seems unnecessary.
| k = k[length::] | ||
| attributes[k] = v | ||
| return attributes | ||
| def clean(self): |
There was a problem hiding this comment.
Is it possible to DRY this method with the similar one in PartyForm, maybe using a mixin?
There was a problem hiding this comment.
Hmm.. possibly. I think there are significant differences between the two though.. TenureRelationshipForm.clean needs to remove all Party related ValidationErrors if saving a new entity, in addition to errors related to party type.. while the PartyForm.clean method only cleans up errors related to party type. I'm not in favour of creating mixins that are not genuinely reusable.. Where else would that proposed mixin be used? I think we have a lot of mixins in the code already and I find them quite confusing at times..
| # remove validation errors for required fields | ||
| # which are not related to the current party type | ||
| new_entity = self.cleaned_data.get('new_entity', False) | ||
| if not new_entity: |
There was a problem hiding this comment.
If new_entity is false, this means that the new tenure relationship only needs the ID of an existing party. Therefore the checks here are unnecessary.
There was a problem hiding this comment.
Because all attribute fields for all party types have to be added to the form (otherwise we can't switch between attribute form fields based on party type selections in the UI), if any of these unused fields are required, when the form is submitted they generate ValidationErrors. This code removes these validation errors. There are two cases handled here:
-
The user is saving an existing
Partyso no validation is required.. we remove allValidationErrorsrelated to the party fields. -
If the user is saving a new
Partyof typeIN, the form may still have requiredGRorCOattributes that we want to ignore.. we need to remove any validation errors not related to the current party type at this point otherwise the form won't save..
There was a problem hiding this comment.
General comment: It seems this PR has some code specific to parties simply because it is the only model that has a schema selector based on type (based on the JSONATTRS_SCHEMA_SELECTORS Django setting). But in the future it is conceivable that we may have conditional attributes based on the type of spatial unit or tenure relationship. So it would be nice if we are able to make things more generic.
|
@seav I think there are places where the code is specific to |
|
@seav the |
Oh, yeah. Ah. So the problem is that |
|
I think there will be a point where we will need conditional attributes for locations as well. Some partners do want to collect things like points of interest and parcel boundaries in one project. So this piece being reusable would be good, but if that is quite a bit of work I would say we could wait until implementing conditional attributes for locations and relationships. |
|
I think, we should wait with implementing conditional attributes and make this a separate task. Brian has been working on this for a bit and I think it will be good for him to tick this off. |
Proposed changes in this pull request
Fixes #761
SchemaSelectorMixinto provide a queryable interface to all project attributes namespaced byContentType.AttributeFormMixinto provide common attribute form processing.AttributeFormsubclass of the plain DjangoForm, which enables creation of multiple domain entities with attributes.AttributeModelFormsubclass of Django'sModelFormwhich enables creation of a single domain entity with attributes.AttributeModelFormoverrides theAttributeModelFormprovided bydjango-jsonattrs.TenureRelationshipFormto use the newAttributeForm.LocationForm,PartyForm,TenureRelationshipEditFormto use the newAttributeModelForm.LocationsAddandTenureRelationshipAddviews to remove hardcoded schema selectors form keyword arguments.django-jsonattrform fields with parsley validation attributes.typewhen creating or editing Parties.pytest-covlibrary to version2.4.0ipythonversion5.1.0to provide a more functional python shell in the dev vm.When should this PR be merged
Anytime
Risks
None
Follow up actions
None
Checklist (for reviewing)
General
migrationlabel if a new migration is added.Is the risk level assessment sufficient? The risks section should contain all risks that might be introduced with the PR and which actions we need to take to mitigate these risks. Possible risks are database migrations, new libraries that need to be installed or changes to deployment scripts.
Functionality
Code
Tests
Documentation