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

Fix #761: Attributes that depend on party type don't show up on party creation #1076

Merged
merged 1 commit into from
Feb 7, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
104 changes: 104 additions & 0 deletions cadasta/core/form_mixins.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,14 @@
from django.forms import Form, ModelForm

from jsonattrs.forms import form_field_from_name
from django.contrib.contenttypes.models import ContentType
from tutelary.models import Role

from .mixins import SchemaSelectorMixin


class SuperUserCheck:

def __init__(self, *args, **kwargs):
super().__init__(*args, **kwargs)
self._su_role = None
Expand All @@ -12,3 +19,100 @@ def is_superuser(self, user):

return any([isinstance(pol, Role) and pol == self.su_role
for pol in user.assigned_policies()])


class AttributeFormMixin(SchemaSelectorMixin):
def create_model_fields(self, field_prefix, attribute_map, new_item=False):
for selector, attributes in attribute_map.items():
for name, attr in attributes.items():
fieldname = '{}::{}::{}'.format(
field_prefix, selector.lower(), name)
atype = attr.attr_type
field_kwargs = {
'label': attr.long_name, 'required': attr.required
}
field = form_field_from_name(atype.form_field)
if not new_item:
self.set_initial(field_kwargs, attr.name, attr)
if atype.form_field in ['ChoiceField', 'MultipleChoiceField']:
if (attr.choice_labels is not None and
attr.choice_labels != []):
chs = list(zip(attr.choices, attr.choice_labels))
else:
chs = [(c, c) for c in attr.choices]
field_kwargs['choices'] = chs
if atype.form_field == 'BooleanField':
field_kwargs['required'] = attr.required
if len(attr.default) > 0:
self.set_default(field_kwargs, attr, boolean=True)
else:
if attr.required and new_item:
field_kwargs['required'] = True
if len(attr.default) > 0 and len(str(
field_kwargs.get('initial', ''))) == 0:
self.set_default(field_kwargs, attr)
self.fields[fieldname] = field(**field_kwargs)

def set_default(self, field_kwargs, attr, boolean=False):
if len(attr.default) > 0:
if boolean:
field_kwargs['initial'] = (attr.default != 'False')
else:
field_kwargs['initial'] = attr.default

def set_initial(self, field_kwargs, name, attr):
if hasattr(self, 'instance'):
attrvals = getattr(self.instance, self.attributes_field)
if name in attrvals:
if attr.attr_type.form_field == 'BooleanField':
field_kwargs['initial'] = (
attrvals[name]
if isinstance(attrvals[name], bool)
else attrvals[name] != 'False'
)
else:
field_kwargs['initial'] = attrvals.get(name, None)

def process_attributes(self, key, entity_type=''):
attributes = {}
for k, v in self.cleaned_data.items():
if k.startswith(key + '::'):
_, type, name = k.split('::')
if type in [entity_type.lower(), 'default']:
attributes[name] = v
if hasattr(self, 'instance'):
setattr(self.instance, self.attributes_field, attributes)
else:
return attributes


class AttributeForm(AttributeFormMixin, Form):
def add_attribute_fields(self, content_type):
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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..

label = '{}.{}'.format(content_type.app_label, content_type.model)
attributes = self.get_model_attributes(self.project, label)
new_item = self.data.get('new_item') == 'on'
self.create_model_fields(
content_type.model, attributes, new_item=new_item
)


class AttributeModelForm(AttributeFormMixin, ModelForm):
Copy link
Contributor

Choose a reason for hiding this comment

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

This is just to satisfy my own curiosity: Why do you need both the Form and ModelForm?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oooh interesting. That makes sense.

def add_attribute_fields(self):
content_type = ContentType.objects.get_for_model(self.Meta.model)
label = '{}.{}'.format(content_type.app_label, content_type.model)
attributes = self.get_model_attributes(self.project, label)
new_item = self.data.get('new_item') == 'on'
self.create_model_fields(
content_type.model, attributes, new_item=new_item
)

def save(self, *args, **kwargs):
entity_type = kwargs.get('entity_type', '')
project_id = kwargs.get('project_id', None)
instance = super().save(commit=False)
content_type = ContentType.objects.get_for_model(instance)
if self.attributes_field is not None:
self.process_attributes(content_type.model, entity_type)
if project_id is not None and hasattr(instance, 'project_id'):
setattr(instance, 'project_id', project_id)
return super().save()
69 changes: 66 additions & 3 deletions cadasta/core/mixins.py
Original file line number Diff line number Diff line change
@@ -1,12 +1,17 @@
from collections import OrderedDict

from django.conf import settings
from django.contrib import messages
from django.shortcuts import redirect
from django.contrib.contenttypes.models import ContentType
from django.core.urlresolvers import reverse
from django.shortcuts import redirect
from django.utils.translation import gettext as _

from jsonattrs.models import Schema, compose_schemas
from tutelary import mixins


class PermissionRequiredMixin(mixins.PermissionRequiredMixin):

def handle_no_permission(self):
msg = super().handle_no_permission()
messages.add_message(self.request, messages.WARNING,
Expand Down Expand Up @@ -52,10 +57,68 @@ def update_permissions(permission, obj=None):
def set_permissions(self, request, view=None):
if (hasattr(self, 'get_organization') and
self.get_organization().archived):
return False
return False
if (hasattr(self, 'get_project') and self.get_project().archived):
return False
if obj and self.get_object().archived:
return False
return permission
return set_permissions


class SchemaSelectorMixin():

def get_attributes(self, project):
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

content_type_to_selectors = self._get_content_types_to_selectors()
attributes_for_models = {}
for content_type, selector_fields in content_type_to_selectors.items():
label = '{}.{}'.format(content_type.app_label, content_type.model)
model = content_type.model_class()
choices = []
selectors = OrderedDict({})
attributes_for_models[label] = OrderedDict({})

for selector_field in selector_fields:
field = model._meta.get_field(selector_field.partition('.')[0])
if field.choices:
choices = [choice[0] for choice in field.choices]
else:
selector = project
sf = selector_field.partition('.')[-1]
sf = sf.replace('.pk', '_id')
selector = getattr(selector, sf, None)
if selector:
selectors[sf] = selector

if selectors and not choices:
defaults = list(selectors.values())
schemas = Schema.objects.lookup(
content_type=content_type, selectors=defaults)
if schemas:
attributes, _, _ = compose_schemas(*schemas)
attributes_for_models[label]['DEFAULT'] = attributes

if selectors and choices:
for choice in choices:
conditionals = list(selectors.values()) + [choice]
schemas = Schema.objects.lookup(
content_type=content_type,
selectors=conditionals)
if schemas:
attributes, _, _ = compose_schemas(*schemas)
attributes_for_models[label][choice] = attributes

return attributes_for_models

def get_model_attributes(self, project, content_type):
attributes_for_models = self.get_attributes(project)
return attributes_for_models[content_type]

def _get_content_types_to_selectors(self):
content_type_to_selectors = dict()
for k, v in settings.JSONATTRS_SCHEMA_SELECTORS.items():
a, m = k.split('.')
content_type_to_selectors[
ContentType.objects.get(app_label=a, model=m)
] = v
return content_type_to_selectors
5 changes: 4 additions & 1 deletion cadasta/core/static/css/main.css

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

66 changes: 66 additions & 0 deletions cadasta/core/static/js/party_attrs.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
/* eslint-env jquery */

function disableConditionals() {
$('.party-co').addClass('hidden');
$('.party-gr').addClass('hidden');
$('.party-in').addClass('hidden');
$('.party-co .form-control').prop('disabled', 'disabled');
$('.party-gr .form-control').prop('disabled', 'disabled');
$('.party-in .form-control').prop('disabled', 'disabled');
}

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice one, thanks.. wasn't happy with that stuff..

function enableConditions(val) {
const 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');
}
}

function toggleParsleyRequired(val) {
const typeChoices = ['in', 'gr', 'co'];
$.each(typeChoices, function(idx, choice) {
if (val === choice) {
$.each($('.party-' + val + ' .form-control'), function(idx, value) {
if (value.hasAttribute('data-parsley-required')) {
$(value).attr('data-parsley-required', true);
$(value).prop('required', 'required');
}
});
} else {
$.each($('.party-' + choice + ' .form-control'), function(idx, value) {
if (value.hasAttribute('data-parsley-required')) {
$(value).attr('data-parsley-required', false);
$(value).prop('required', '');
}
});
}
});
}

function toggleStates(val) {
if (val === '') {
disableConditionals();
} else {
enableConditions(val);
toggleParsleyRequired(val);
}
}

$().ready(function() {
const val = $('.party-type').val().toLowerCase();
toggleStates(val);
});


$('select.party-type').on('change', function(e) {
const val = e.target.value.toLowerCase();
toggleStates(val);
});

$('select.party-select').on('change', function(e) {
toggleStates('');
});
6 changes: 4 additions & 2 deletions cadasta/core/static/js/rel_tenure.js
Original file line number Diff line number Diff line change
@@ -1,11 +1,13 @@
/* eslint-env jquery */

$('button#add-party').click(function() {
$('div#select-party').toggleClass('hidden');
$('div#new-item').toggleClass('hidden');
});

$('table#select-list tr').click(function(event) {
var target = $(event.target).closest('tr');
var relId = target.attr('data-id');
const target = $(event.target).closest('tr');
const relId = target.attr('data-id');
target.closest('tbody').find('tr.info').removeClass('info');
target.addClass('info');
$('input[name="id"]').val(relId);
Expand Down
Empty file.
40 changes: 40 additions & 0 deletions cadasta/core/templatetags/filters.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
from django import template
from django.forms import ChoiceField, FileField

register = template.Library()


@register.filter(name='field_value')
def field_value(field):
"""Return the value for this BoundField."""
if field.form.is_bound:
if isinstance(field.field, FileField) and field.data is None:
val = field.form.initial.get(field.name, field.field.initial)
else:
val = field.data
else:
val = field.form.initial.get(field.name, field.field.initial)
if callable(val):
val = val()
if val is None:
val = ''
return val


@register.filter(name='display_choice_verbose')
def display_choice_verbose(field):
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure, but does this filter work well with translations?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

"""Return the displayed value for this BoundField."""
if isinstance(field.field, ChoiceField):
value = field_value(field)
for (val, desc) in field.field.choices:
if val == value:
return desc


@register.filter(name='set_parsley_required')
def set_parsley_required(field):
if field.field.required:
field.field.widget.attrs = {
'data-parsley-required': 'true'
}
return field
Loading