wlanslovenija / PiplMesh

A social info portal for wireless networks.
http://dev.wlan-si.net/wiki/PiplMesh
Other
40 stars 19 forks source link

Issue #265 - user panel control #288

Open Naltharial opened 11 years ago

Naltharial commented 11 years ago

265: Added basic support for users to select which panels they should show. Dependencies checked on form validation.

TODO: Required/recommended extension for inter-panel dependencies.

Naltharial commented 11 years ago

Changed the form to metaclass, changed the Panels model. Not sure why Issue #278 has a double-nested MapField, however. Or why we're passing number_of_columns as an argument. So right now, each user has a list of their panels and every panel can be collapsed and is at exactly one (column,order) position.

mitar commented 11 years ago

Based on the width of the window, number of columns for panels change. So we have double nested mapfiled because we are storing layout of panels for each number of columns there is. So once we map panel id to Panel. And then we map number of columns to PanelState, which tells where for this number of columns panel is.

mitar commented 11 years ago

So you will have to fix this. :-)

Naltharial commented 11 years ago

Added the second MapField. I've just pulled collapsed out of PanelLayout, because it's not connected to number of columns.

mitar commented 11 years ago

Not true. It can be. If you have very narrow window on your laptop, you might not want to display all panels. You do not want them to be removed, but just collapsed. On your big monitor on your PC, you have more columns and you want all of panels to be shown fully.

Naltharial commented 11 years ago

Well, sure, it just wasn't connected currently the way the JavaScript part is set up (requests to panel/collapse/ do not include number_of_columns). I can connect it, but that would mean changing the JavaScript behind it, which I thought was part of another issue.

mitar commented 11 years ago

OK. You decide. So I am explaining why model was proposed in the way it was. It is true that current code does not behave like that. But the end it should. So you can fix it yourself, or add TODO/ticket for this.

Naltharial commented 11 years ago

Added the number to the requests for collapse and changed the model.

Naltharial commented 11 years ago

I've moved most of the checking to panels_validate view, which is called by JavaScript when user input is to be validated, before submitting the form.

mitar commented 11 years ago

Please update from main repository. It cannot be automatically merged.

mitar commented 11 years ago

Why? Why are you requiring call to the server? I would agree if validation would be very complex. But here it is simple and is especially important user feedback. So what you do on error. And if you code that in JavaScript, you can provide much nicer feedback there. So simply, you can have a form validation which checks if all dependencies are resolved and raises error if not. And then more complex JavaScript code which can in real-time, as user clicks around, enable dependent panels automatically and write some messages bellow that panels have been enabled because they are dependent.

Naltharial commented 11 years ago

Removed AJAX, sent panels as JSON to client so checking can be done completely in JavaScript.

I've tried merging, but PiplMesh stops working if I do:

  File "D:\My Documents\FRI\IPRI\PiplMesh\PiplMesh\piplmesh\frontend\views.py", line 128, in <module>
@mongoengine_signals.post_save.connect_via(sender=api_models.Notification)
AttributeError: '_FakeSignal' object has no attribute 'connect_via'

if I remove that signal, the updates are being requested at http://127.0.0.1:8001/updates/user/[GUID]/, which pushserver returns 404 on. Should I update anything else except pip install -r requirements.txt?

mitar commented 11 years ago

Please update from main repository, so that it can be automatically merged.

mitar commented 11 years ago

As you can see here, _FakeSignal is used only if you do not have blinker package available. Have you installed it with pip? It is in requirements.txt.

mitar commented 11 years ago

which pushserver returns 404 on.

Are your regex for url same as here? So (.+)?

Naltharial commented 11 years ago

I ran requirements through. It looks like the blinker entry didn't work. I got

Downloading/unpacking blinker==1.2 (from -r requirements.txt (line 9))

and the setup conpleted, but then still the _FakeSignal error occured. I had to manually run pip install blinker to fix it. Maybe a version issue? The pushserver issue went away after a while, so the merge should be completed now.

I created a JSON filter instead of doing it manually.

mitar commented 11 years ago

Which version of blinker does pip freeze report for you now?

mitar commented 11 years ago

OK. So we have to decide on schema. Because some code things are related to that.

Naltharial commented 11 years ago

Strangely, it reports the same version, 1.2. Might be something with me using Windows, probably not a general bug.

mitar commented 11 years ago

So overall things are looking good. But there are some slight improvements still needed. In the internal API you defined. And in user interface.

If you are in hurry, you can maybe open ticket for user interface improvements.

mitar commented 11 years ago

Tests are failing. See details.

mitar commented 11 years ago

Tests are failing. Run ./manage.py test and see.

mitar commented 11 years ago

Please update from main repository.

Naltharial commented 11 years ago

I've replaced the get_columns method, the frontend does seem to work even with new panels that haven't been ordered yet. I had to use panels_pool in view though, because otherwise you don't get dependencies for panels that aren't currently selected.

Test cases also seem to pass now.

mitar commented 11 years ago

the frontend does seem to work even with new panels that haven't been ordered yet.

Yes. This is good. So probably only frontend should decide how to order new panels. Not backend

I had to use panels_pool in view though, because otherwise you don't get dependencies for panels that aren't currently selected.

I agree. But we had get_all_panels method or something on user object before, why is that one not good? It can return for now the same list as panels_pool. But later on it could be limited (by, for example, permissions (debug panel could be available only to developers), or payment (we could have some panels available only if you buy them)).

mitar commented 11 years ago

Feature-wise, things are good now. So just check all your code for possible tabs and fix indentations and this is ready to go in.

There is one minor detail you can do, if you want, or open a ticket. You have to enable for elements before submit. But this is ugly as it changes user feedback and maybe confuses her. So some other solution should be used. What Internet recommends? Maybe a hidden input could be inserted which is kept in sync with checkbox? Maybe each checkbox could have its own hidden input all the time, kept in sync. JavaScript could rename all checkboxes and add hidden input box instead with that form name.

Naltharial commented 11 years ago

I've added a second set of hidden inputs, so we don't have to un-disable the checkboxes. They're IntegerField, because hidden checkboxes don't properly submit state.

Naltharial commented 11 years ago

Fixed.

mitar commented 11 years ago

You had some problems with link property? Or you just didn't upgrade from requirements.txt?

mitar commented 11 years ago

I moved account Django app to separate Python package. You will have to update this pull request.

https://github.com/mitar/django-mongo-auth

mitar commented 11 years ago

OK, now everything should work with external authentication package.