-
Notifications
You must be signed in to change notification settings - Fork 19
Issue #265 - user panel control #288
base: master
Are you sure you want to change the base?
Changes from 3 commits
1589264
b72a369
053e7ff
4b07a40
c9bcc0f
9ca619a
5acfcbd
f26f03c
ffcc520
cccd9ce
86e5a0e
4f69062
9b76a6a
5b281bf
101b62e
a9d117c
6a06c73
30f2295
88cc1c0
117fa2f
5a3650c
c439d20
b8352d1
39053de
87213c6
659b606
444243d
9b257c9
6ae8f90
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,6 +2,7 @@ | |
from django.forms.extras import widgets | ||
from django.utils.translation import ugettext_lazy as _ | ||
|
||
from piplmesh import panels | ||
from piplmesh.account import fields, form_fields, models | ||
|
||
class UserUsernameForm(forms.Form): | ||
|
@@ -164,3 +165,59 @@ def clean_confirmation_token(self): | |
if not self.user.email_confirmation_token.check_token(confirmation_token): | ||
raise forms.ValidationError(_("The confirmation token is invalid or has expired. Please retry."), code='confirmation_token_incorrect') | ||
return confirmation_token | ||
|
||
def panel_form_factory(): | ||
""" | ||
Function which generates the form for selecting homepage panels. | ||
""" | ||
def __init__(self, *args, **kwargs): | ||
super(forms.Form, self).__init__(*args, **kwargs) | ||
|
||
# Add information about dependencies to display in template | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need this now? Not anymore? |
||
for name in self.fields: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
panel = panels.panels_pool.get_panel(name) | ||
self.fields[name].dependencies = panel.get_dependencies() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I must say that I find this ugly. Attaching some external information to the field. This is OK if there is no other way, but in this case there is: you can retrieve this information ad-hoc in But OK, let it be like this for now. Let see other things. |
||
|
||
def clean(self): | ||
cleaned_data = super(forms.Form, self).clean() | ||
add = [] | ||
|
||
while True: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would not display errors for missing dependencies for automatically selected dependencies under the later, but they should be listed under initial panel. So if panel A requires panel B which in turn requires panel C, error message should be "Panel A depends on panels B and C, which have been selected." There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (And not B nor C have been selected. If one is, then message mentions only the other one.) |
||
if add: | ||
# User has not selected some of the dependencies, so we select them | ||
for val in add: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Replace |
||
cleaned_data[val] = True | ||
self.data[val] = 'on' | ||
add = [] | ||
|
||
for name, val in cleaned_data.iteritems(): | ||
if not val: | ||
continue | ||
|
||
panel = panels.panels_pool.get_panel(name) | ||
errors = [] | ||
for dep in panel.get_dependencies(): | ||
if not cleaned_data[dep]: | ||
errors.append(_("Panel '%s' depends on panel '%s', it has been selected." % (name, dep))) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK. This is misuse of the API. Here you should or fix dependencies or display an error. You cannot do both. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hm, but it works nicely. :-) OK. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But, hm. It is not clear to the user if he has to submit form again or not, when dependent panels are automatically selected. With current code, because you are misusing errors, it is not saved until she submits again. So I see two options:
For later you will have to have access to request, which you can pass as a constructor to the form (see |
||
add.append(dep) | ||
|
||
if errors: | ||
self._errors[name] = self.error_class(errors) | ||
|
||
# All requirements satisfied | ||
if not add: | ||
break | ||
|
||
return cleaned_data | ||
|
||
panel_list = panels.panels_pool.get_all_panels() | ||
|
||
properties = {} | ||
for panel in panel_list: | ||
properties[panel.get_name()] = forms.BooleanField(required=False, initial=True) | ||
|
||
form = type('PanelForm', (forms.Form,), properties) | ||
form.__init__ = __init__ | ||
form.clean = clean | ||
|
||
return form |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -48,6 +48,16 @@ class TwitterAccessToken(mongoengine.EmbeddedDocument): | |
key = mongoengine.StringField(max_length=150) | ||
secret = mongoengine.StringField(max_length=150) | ||
|
||
class UserPanels(mongoengine.EmbeddedDocument): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you make model like it is defined here. So the idea is that you have a list/map of enabled plugins. So anything which is in user object is enabled. Not that you have a flag. There is no reason to have a flag. User adds a panel or removes it. This is it. |
||
panels_collapsed = mongoengine.DictField() | ||
panels_order = mongoengine.DictField() | ||
panels_disabled = mongoengine.ListField() | ||
|
||
def get_panels(self): | ||
return [panel for panel \ | ||
in panels.panels_pool.get_all_panels() \ | ||
if panel not in map(panels.panels_pool.get_panel, self.panels_disabled)] | ||
|
||
class User(auth.User): | ||
username = mongoengine.StringField( | ||
max_length=30, | ||
|
@@ -83,10 +93,15 @@ class User(auth.User): | |
|
||
email_confirmed = mongoengine.BooleanField(default=False) | ||
email_confirmation_token = mongoengine.EmbeddedDocumentField(EmailConfirmationToken) | ||
|
||
# TODO: Model for panel settings should be more semantic. | ||
panels_collapsed = mongoengine.DictField() | ||
panels_order = mongoengine.DictField() | ||
|
||
user_panels = mongoengine.EmbeddedDocumentField(UserPanels) | ||
|
||
def __init__(self, *args, **kwargs): | ||
super(User, self).__init__(*args, **kwargs) | ||
|
||
# If the user has not previously saved any panel data, make sure we have an object to query | ||
if self.user_panels == None: | ||
self.user_panels = UserPanels() | ||
|
||
@models.permalink | ||
def get_absolute_url(self): | ||
|
@@ -95,10 +110,6 @@ def get_absolute_url(self): | |
def get_profile_url(self): | ||
return self.get_absolute_url() | ||
|
||
def get_panels(self): | ||
# TODO: Should return only panels user has enabled (should make sure users can enable panels only in the way that dependencies are satisfied) | ||
return panels.panels_pool.get_all_panels() | ||
|
||
def is_anonymous(self): | ||
return not self.is_authenticated() | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,39 @@ | ||
{% load i18n %} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK. This will have to be improved. It is mostly duplication of form.html. This is not good. It should be done in extendable way. But I will do that. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (After I pull this in.) |
||
|
||
<form action="{{ form_action }}" method="post" id="panel_form"> | ||
{{ form.non_field_errors }} | ||
{% csrf_token %} | ||
{% with next|default:request_get_next|default:"" as next_url %} | ||
<div style="display: none;"><input type="hidden" name="{{ REDIRECT_FIELD_NAME }}" value="{{ next_url }}" /></div> | ||
{% endwith %} | ||
{% if form.fields %} | ||
<ul class="form_fields"> | ||
{% for field in form %} | ||
{% if field.errors %} | ||
<li class="field_error"> | ||
{{ field.errors }} | ||
</li> | ||
{% endif %} | ||
<li class="field"> | ||
<div class="text"> | ||
<label for="{{ field.id_for_label }}"> | ||
{{ field.label }} | ||
</label> | ||
{% if field.field.dependencies %} | ||
<div class="dependencies"> | ||
{% trans "Required panels" %}: | ||
<ul> | ||
{% for dep in field.field.dependencies %} | ||
<li>{{ dep }}</li> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Indentation. |
||
{% endfor %} | ||
</ul> | ||
</div> | ||
{% endif %} | ||
</div> | ||
<div class="input">{{ field }}</div> | ||
</li> | ||
{% endfor %} | ||
</ul> | ||
{% endif %} | ||
<div class="buttons"><input type="submit" value="{{ form_submit }}" /></div> | ||
</form> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
{% extends "plain.html" %} | ||
|
||
{% load future i18n %} | ||
|
||
{% block title %}{% trans "Account" %}{% endblock %} | ||
|
||
{% block content %} | ||
<div id="menu_list"> | ||
<p><a href="{% url "profile" username=user.username %}">{% trans "Profile" %}</a></p> | ||
<p><a href="{% url "account" %}">{% trans "Account" %}</a></p> | ||
<p id="current_item"><a href="{% url "user_panels" %}">{% trans "Panels" %}</a></p> | ||
</div> | ||
<div class="panels"> | ||
{% with form_submit=_("Submit") %} | ||
{% include "form/panels.html" %} | ||
{% endwith %} | ||
</div> | ||
{% endblock %} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -402,6 +402,34 @@ def dispatch(self, request, *args, **kwargs): | |
def get_form(self, form_class): | ||
return form_class(self.request.user, **self.get_form_kwargs()) | ||
|
||
class PanelView(generic_views.FormView): | ||
template_name = 'user/panels.html' | ||
form_class = forms.panel_form_factory() | ||
success_url = urlresolvers.reverse_lazy('user_panels') | ||
|
||
def form_valid(self, form): | ||
user = self.request.user | ||
user.user_panels.panels_disabled = [key for key, val in form.cleaned_data.iteritems() if not val] | ||
user.save() | ||
messages.success(self.request, _("You have successfully set your panel defaults.")) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No need for the message because you are returning to the same page. You would need this if you would be redirecting to some other page and would need to give the user feedback. |
||
return super(PanelView, self).form_valid(form) | ||
|
||
def get_initial(self): | ||
user = self.request.user | ||
return dict(zip(user.user_panels.panels_disabled, [False] * len(user.user_panels.panels_disabled))) | ||
|
||
def get_form_kwargs(self): | ||
""" | ||
Returns the keyword arguments for instanciating the form, | ||
copying request.POST so we can change users' selection on validation. | ||
""" | ||
kwargs = {'initial': self.get_initial()} | ||
if self.request.method in ('POST', 'PUT'): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ehm, forms have only POST. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is not REST API. |
||
kwargs.update({ | ||
'data': dict(self.request.POST), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't understand how this helps? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hm, the only thing you are doing here is making data mutable? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's so we can change which panels the user ticked if they didn't select all of the prerequisites. If we're not changing the selection anymore (not using form errors) then it'll probably not be neccessary anymore. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is difference between displaying error messages and selecting dependencies automatically. So this is needed to be able to select dependencies automatically. But I am just wondering here why it is necessary for it to be here and not for example to copy this in view or form constructor. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Anyway, I think we could do this better. Dependencies should be resolved in JavaScript on the client. So user should get feedback there, dynamically. On the server I would just test if everything is correct and display an error. Error should be possible in practice only if somebody is doing something nasty. In reality JavaScript would take care that everything is correct. But we just check for every case. So this your approach to require user to submit the form to verify it is so 90'. |
||
}) | ||
return kwargs | ||
|
||
def logout(request): | ||
""" | ||
After user logouts, redirect her back to the page she came from. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,8 +16,8 @@ def __init__(self): | |
from . import panels_pool | ||
|
||
for dependency in self.get_dependencies(): | ||
if not panels_pool.panels_pool.has_panel(dependency): | ||
raise exceptions.PanelDependencyNotRegistered("Panel '%s' depends on panel '%s', but later is not registered" % (self.get_name(), dependency)) | ||
if not panels_pool.has_panel(dependency): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are you sure it should be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I get
if I try it. |
||
raise exceptions.PanelDependencyNotRegistered("Panel '%s' depends on panel '%s', but the latter is not registered" % (self.get_name(), dependency)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Vau, I didn't know that |
||
|
||
@classmethod | ||
def get_name(cls): | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
import random | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why additional panel? You could just in original panel file extend another panel class and register it. To reduce duplication. So, use inheritance. |
||
|
||
from django.conf import settings | ||
from django.contrib.webdesign import lorem_ipsum | ||
from django.utils.translation import ugettext_lazy as _ | ||
|
||
from piplmesh import panels | ||
|
||
class DummyTestPanel(panels.BasePanel): | ||
dependencies = ('dummy',) | ||
|
||
def get_context(self, context): | ||
context = super(DummyTestPanel, self).get_context(context) | ||
|
||
context.update({ | ||
'header': _("Dummy test panel"), | ||
'content': '\n\n'.join(lorem_ipsum.paragraphs(random.randint(1, 1))), | ||
}) | ||
return context | ||
|
||
if settings.DEBUG: | ||
panels.panels_pool.register(DummyTestPanel) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
{% extends "panel/panel.html" %} | ||
|
||
{% block content %} | ||
{{ content|linebreaks }} | ||
{% endblock %} |
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.
Interesting approach, but it is a bit hackish. ;-) I much more prefer metaclass approach. Here is done for models, but it is very similar. So you define a function which adds class attributes at creation time. Can you convert your code into it?