Issue 101630045: Added userwise acl view (Closed)
|
Created:
9 years, 9 months ago by ajgupta93 Modified:
9 years, 9 months ago Reviewers:
thomas.j.waldmann, RogerHaase Visibility:
Public. |
DescriptionAdded userwise acl view
Patch Set 1 #Patch Set 2 : Made the checkboxes readonly, added user name in the view and cleaned pep8 errors #
Total comments: 13
Patch Set 3 : Removed space before comma, fixed indentation, and used fqname list #
Total comments: 6
Patch Set 4 : Fixed Indentation #
Total comments: 11
Patch Set 5 : Used fqnames and uids instead of names and removed checkboxes #
Total comments: 2
Patch Set 6 : Loaded tablesorter.js before common.js #Patch Set 7 : Changed the document retrieval method #Patch Set 8 : Added superuser permission #
Total comments: 17
Patch Set 9 : Changes as per comments #Patch Set 10 : Minor correction #
Total comments: 1
MessagesTotal messages: 13
tested with several other patches, useful report.
Sign in to reply to this message.
https://codereview.appspot.com/101630045/diff/40001/MoinMoin/apps/admin/templ... File MoinMoin/apps/admin/templates/admin/user_acl_info.html (right): https://codereview.appspot.com/101630045/diff/40001/MoinMoin/apps/admin/templ... MoinMoin/apps/admin/templates/admin/user_acl_info.html:10: <h3>User Name : {{ user_name|join(' , ') }}</h3> please use plural for lists. user_names and there is no blank before a comma. https://codereview.appspot.com/101630045/diff/40001/MoinMoin/apps/admin/templ... MoinMoin/apps/admin/templates/admin/user_acl_info.html:16: <th>{{ _("Can Read") }}</th> i am sure most users can read. but whether they may read some specific item is a different thing. fix globally, please. https://codereview.appspot.com/101630045/diff/40001/MoinMoin/apps/admin/templ... MoinMoin/apps/admin/templates/admin/user_acl_info.html:26: <td><a href="{{ url_for('frontend.modify_item', item_name=item['name'][0]) }}">{{ item['name']|join(' , ') }}</a></td> same again: crashes for nameless items. no blank before comma. https://codereview.appspot.com/101630045/diff/40001/MoinMoin/apps/admin/templ... MoinMoin/apps/admin/templates/admin/user_acl_info.html:33: {% endfor %} fix indentation https://codereview.appspot.com/101630045/diff/40001/MoinMoin/apps/admin/templ... File MoinMoin/apps/admin/templates/admin/userbrowser.html (right): https://codereview.appspot.com/101630045/diff/40001/MoinMoin/apps/admin/templ... MoinMoin/apps/admin/templates/admin/userbrowser.html:39: <form action="{{ url_for('admin.user_acl_info', user_name=u.name[0]) }}" method="GET"> would crash for nameless items. well, maybe we don't have nameless users, so you might be lucky here. https://codereview.appspot.com/101630045/diff/40001/MoinMoin/apps/admin/views.py File MoinMoin/apps/admin/views.py (right): https://codereview.appspot.com/101630045/diff/40001/MoinMoin/apps/admin/views... MoinMoin/apps/admin/views.py:256: all_items = flaskg.storage.indexer.ix[LATEST_REVS].searcher().documents() maybe there is an easier way https://codereview.appspot.com/101630045/diff/40001/MoinMoin/apps/admin/views... MoinMoin/apps/admin/views.py:261: fqname = split_fqname(item['name'][0]) why do you call split_fqname on the first name? what's with the other names?
Sign in to reply to this message.
https://codereview.appspot.com/101630045/diff/40001/MoinMoin/apps/admin/templ... File MoinMoin/apps/admin/templates/admin/user_acl_info.html (right): https://codereview.appspot.com/101630045/diff/40001/MoinMoin/apps/admin/templ... MoinMoin/apps/admin/templates/admin/user_acl_info.html:10: <h3>User Name : {{ user_name|join(' , ') }}</h3> On 2014/07/05 15:33:30, Thomas.J.Waldmann wrote: > please use plural for lists. user_names > > and there is no blank before a comma. Done. https://codereview.appspot.com/101630045/diff/40001/MoinMoin/apps/admin/templ... MoinMoin/apps/admin/templates/admin/user_acl_info.html:16: <th>{{ _("Can Read") }}</th> On 2014/07/05 15:33:30, Thomas.J.Waldmann wrote: > i am sure most users can read. > but whether they may read some specific item is a different thing. > fix globally, please. Done. https://codereview.appspot.com/101630045/diff/40001/MoinMoin/apps/admin/templ... MoinMoin/apps/admin/templates/admin/user_acl_info.html:26: <td><a href="{{ url_for('frontend.modify_item', item_name=item['name'][0]) }}">{{ item['name']|join(' , ') }}</a></td> On 2014/07/05 15:33:30, Thomas.J.Waldmann wrote: > same again: crashes for nameless items. > no blank before comma. I'll have to add fqname support to modify_items first and also there is another traceback regarding the names problem. So i'll do both them first and then add fqname support here in a later changeset. https://codereview.appspot.com/101630045/diff/40001/MoinMoin/apps/admin/templ... MoinMoin/apps/admin/templates/admin/user_acl_info.html:33: {% endfor %} On 2014/07/05 15:33:30, Thomas.J.Waldmann wrote: > fix indentation Done. https://codereview.appspot.com/101630045/diff/40001/MoinMoin/apps/admin/views.py File MoinMoin/apps/admin/views.py (right): https://codereview.appspot.com/101630045/diff/40001/MoinMoin/apps/admin/views... MoinMoin/apps/admin/views.py:256: all_items = flaskg.storage.indexer.ix[LATEST_REVS].searcher().documents() On 2014/07/05 15:33:30, Thomas.J.Waldmann wrote: > maybe there is an easier way I dont find any other way. Plus this is the docmented way too - https://pythonhosted.org/Whoosh/api/searching.html#whoosh.searching.Searcher.... https://codereview.appspot.com/101630045/diff/40001/MoinMoin/apps/admin/views... MoinMoin/apps/admin/views.py:261: fqname = split_fqname(item['name'][0]) On 2014/07/05 15:33:30, Thomas.J.Waldmann wrote: > why do you call split_fqname on the first name? > > what's with the other names? Done.
Sign in to reply to this message.
https://codereview.appspot.com/101630045/diff/60001/MoinMoin/apps/admin/templ... File MoinMoin/apps/admin/templates/admin/user_acl_info.html (right): https://codereview.appspot.com/101630045/diff/60001/MoinMoin/apps/admin/templ... MoinMoin/apps/admin/templates/admin/user_acl_info.html:7: {% endif %} indent lines within if https://codereview.appspot.com/101630045/diff/60001/MoinMoin/apps/admin/templ... File MoinMoin/apps/admin/templates/admin/userbrowser.html (right): https://codereview.appspot.com/101630045/diff/60001/MoinMoin/apps/admin/templ... MoinMoin/apps/admin/templates/admin/userbrowser.html:4: <script src="{{ url_for('serve.files', name='jquery_tablesorter', filename='jquery.tablesorter.js') }}"></script> if you indent this, then also indent super above https://codereview.appspot.com/101630045/diff/60001/MoinMoin/apps/admin/templ... MoinMoin/apps/admin/templates/admin/userbrowser.html:45: </table> indentation does not match <table..> above (last editor's fault).
Sign in to reply to this message.
https://codereview.appspot.com/101630045/diff/60001/MoinMoin/apps/admin/templ... File MoinMoin/apps/admin/templates/admin/user_acl_info.html (right): https://codereview.appspot.com/101630045/diff/60001/MoinMoin/apps/admin/templ... MoinMoin/apps/admin/templates/admin/user_acl_info.html:7: {% endif %} On 2014/07/07 13:01:07, RogerHaase wrote: > indent lines within if Done. https://codereview.appspot.com/101630045/diff/60001/MoinMoin/apps/admin/templ... File MoinMoin/apps/admin/templates/admin/userbrowser.html (right): https://codereview.appspot.com/101630045/diff/60001/MoinMoin/apps/admin/templ... MoinMoin/apps/admin/templates/admin/userbrowser.html:4: <script src="{{ url_for('serve.files', name='jquery_tablesorter', filename='jquery.tablesorter.js') }}"></script> On 2014/07/07 13:01:07, RogerHaase wrote: > if you indent this, then also indent super above Done. https://codereview.appspot.com/101630045/diff/60001/MoinMoin/apps/admin/templ... MoinMoin/apps/admin/templates/admin/userbrowser.html:45: </table> On 2014/07/07 13:01:07, RogerHaase wrote: > indentation does not match <table..> above (last editor's fault). Done.
Sign in to reply to this message.
https://codereview.appspot.com/101630045/diff/80001/MoinMoin/apps/admin/templ... File MoinMoin/apps/admin/templates/admin/user_acl_info.html (right): https://codereview.appspot.com/101630045/diff/80001/MoinMoin/apps/admin/templ... MoinMoin/apps/admin/templates/admin/user_acl_info.html:6: <h3>User Name(s) : {{ user_names|join(', ') }}</h3> hmm,usually h2 comes after h1. https://codereview.appspot.com/101630045/diff/80001/MoinMoin/apps/admin/templ... MoinMoin/apps/admin/templates/admin/user_acl_info.html:22: <td><a href="{{ url_for('frontend.modify_item', item_name=item['name'][0]) }}">{{ item['name']|join(', ') }}</a></td> i already pointed out some times that item['name'][0] will crash for nameless items. https://codereview.appspot.com/101630045/diff/80001/MoinMoin/apps/admin/templ... MoinMoin/apps/admin/templates/admin/user_acl_info.html:23: <td><input type="checkbox" disabled="disabled" name="read" value="{{ item['read'] }}" {% if item['read'] %}checked="checked"{% endif %}></td> this somehow seems a bit duplicate. you have a value (that changes depending on read permission). AND you have a checkbox that does also. also, you should always think about usability (as I already pointed out) and scalability. this table might get rather long and it will be hard to see what all these True False True False False means. So I suggest you (e.g.) put "read" as value (not "True") if the user may read. And "" if the user may not read. Then it is immediately clear what it means without having to scroll upwards lots of lines or remember the order of permissions and the related column numbers. BTW, why is this a input field? I don't see a form here. https://codereview.appspot.com/101630045/diff/80001/MoinMoin/apps/admin/templ... File MoinMoin/apps/admin/templates/admin/userbrowser.html (right): https://codereview.appspot.com/101630045/diff/80001/MoinMoin/apps/admin/templ... MoinMoin/apps/admin/templates/admin/userbrowser.html:35: <form action="{{ url_for('admin.user_acl_info', user_name=u.name[0]) }}" method="GET"> ^^ https://codereview.appspot.com/101630045/diff/80001/MoinMoin/apps/admin/views.py File MoinMoin/apps/admin/views.py (right): https://codereview.appspot.com/101630045/diff/80001/MoinMoin/apps/admin/views... MoinMoin/apps/admin/views.py:257: all_items = flaskg.storage.indexer.ix[LATEST_REVS].searcher().documents() still no better method? https://codereview.appspot.com/101630045/diff/80001/MoinMoin/apps/admin/views... MoinMoin/apps/admin/views.py:263: fqnames = split_fqname_list(item['name']) is that function new? seems strange to do it like that in the face of list comprehensions.
Sign in to reply to this message.
https://codereview.appspot.com/101630045/diff/80001/MoinMoin/apps/admin/templ... File MoinMoin/apps/admin/templates/admin/user_acl_info.html (right): https://codereview.appspot.com/101630045/diff/80001/MoinMoin/apps/admin/templ... MoinMoin/apps/admin/templates/admin/user_acl_info.html:6: <h3>User Name(s) : {{ user_names|join(', ') }}</h3> On 2014/07/07 13:37:32, Thomas.J.Waldmann wrote: > hmm,usually h2 comes after h1. Done. https://codereview.appspot.com/101630045/diff/80001/MoinMoin/apps/admin/templ... MoinMoin/apps/admin/templates/admin/user_acl_info.html:22: <td><a href="{{ url_for('frontend.modify_item', item_name=item['name'][0]) }}">{{ item['name']|join(', ') }}</a></td> On 2014/07/07 13:37:31, Thomas.J.Waldmann wrote: > i already pointed out some times that item['name'][0] will crash for nameless > items. Done. https://codereview.appspot.com/101630045/diff/80001/MoinMoin/apps/admin/templ... MoinMoin/apps/admin/templates/admin/user_acl_info.html:23: <td><input type="checkbox" disabled="disabled" name="read" value="{{ item['read'] }}" {% if item['read'] %}checked="checked"{% endif %}></td> On 2014/07/07 13:37:32, Thomas.J.Waldmann wrote: > this somehow seems a bit duplicate. > > you have a value (that changes depending on read permission). > AND you have a checkbox that does also. > > also, you should always think about usability (as I already pointed out) and > scalability. > this table might get rather long and it will be hard to see what all these True > False True False False means. > > So I suggest you (e.g.) put "read" as value (not "True") if the user may read. > And "" if the user may not read. > Then it is immediately clear what it means without having to scroll upwards lots > of lines or remember the order of permissions and the related column numbers. > > BTW, why is this a input field? I don't see a form here. Done. https://codereview.appspot.com/101630045/diff/80001/MoinMoin/apps/admin/templ... File MoinMoin/apps/admin/templates/admin/userbrowser.html (right): https://codereview.appspot.com/101630045/diff/80001/MoinMoin/apps/admin/templ... MoinMoin/apps/admin/templates/admin/userbrowser.html:35: <form action="{{ url_for('admin.user_acl_info', user_name=u.name[0]) }}" method="GET"> On 2014/07/07 13:37:32, Thomas.J.Waldmann wrote: > ^^ Done. https://codereview.appspot.com/101630045/diff/80001/MoinMoin/apps/admin/views.py File MoinMoin/apps/admin/views.py (right): https://codereview.appspot.com/101630045/diff/80001/MoinMoin/apps/admin/views... MoinMoin/apps/admin/views.py:263: fqnames = split_fqname_list(item['name']) On 2014/07/07 13:37:32, Thomas.J.Waldmann wrote: > is that function new? seems strange to do it like that in the face of list > comprehensions. Done.
Sign in to reply to this message.
https://codereview.appspot.com/101630045/diff/100001/MoinMoin/templates/base.... File MoinMoin/templates/base.html (right): https://codereview.appspot.com/101630045/diff/100001/MoinMoin/templates/base.... MoinMoin/templates/base.html:85: <script src="{{ url_for('serve.files', name='jquery_tablesorter', filename='jquery.tablesorter.js') }}"></script> as common.js is likely to execute functions within tablesorter, tablesorter should be loaded before common.js
Sign in to reply to this message.
https://codereview.appspot.com/101630045/diff/100001/MoinMoin/templates/base.... File MoinMoin/templates/base.html (right): https://codereview.appspot.com/101630045/diff/100001/MoinMoin/templates/base.... MoinMoin/templates/base.html:85: <script src="{{ url_for('serve.files', name='jquery_tablesorter', filename='jquery.tablesorter.js') }}"></script> On 2014/07/07 20:47:31, RogerHaase wrote: > as common.js is likely to execute functions within tablesorter, tablesorter > should be loaded before common.js Done.
Sign in to reply to this message.
https://codereview.appspot.com/101630045/diff/160001/MoinMoin/apps/admin/temp... File MoinMoin/apps/admin/templates/admin/user_acl_info.html (right): https://codereview.appspot.com/101630045/diff/160001/MoinMoin/apps/admin/temp... MoinMoin/apps/admin/templates/admin/user_acl_info.html:7: {% endif %} do you call it without headline also? https://codereview.appspot.com/101630045/diff/160001/MoinMoin/apps/admin/temp... MoinMoin/apps/admin/templates/admin/user_acl_info.html:16: <th>{{ _("Admin") }}</th> acl permissions are written lowercase in the acl string. if you write them uppercase here, you might mislead users in thinking that they can have them uppercase in acls, too. Please fix this everywhere where you did it. also, maybe rather do not translate these (for similar reasons). https://codereview.appspot.com/101630045/diff/160001/MoinMoin/apps/admin/temp... MoinMoin/apps/admin/templates/admin/user_acl_info.html:22: <td><a href="{{ url_for('frontend.modify_item', item_name=item['fqname']) }}">{% if item['name']|length %}{{ item['name']|join(', ') }}{% else %}Item Id : {{ item['itemid'] }}{% endif %}</a></td> do you need "| length" here? doesn't an empty list evaluate as falsy anyway? and please note that in many western languages, you NEVER put a blank before a colon. https://codereview.appspot.com/101630045/diff/160001/MoinMoin/apps/admin/temp... MoinMoin/apps/admin/templates/admin/user_acl_info.html:27: <td><p>{% if item['admin'] %}Admin{% endif %}</p></td> do you need the <p>? https://codereview.appspot.com/101630045/diff/160001/MoinMoin/apps/admin/temp... File MoinMoin/apps/admin/templates/admin/userbrowser.html (right): https://codereview.appspot.com/101630045/diff/160001/MoinMoin/apps/admin/temp... MoinMoin/apps/admin/templates/admin/userbrowser.html:13: <td><a href="{{ url_for('frontend.show_item', item_name=u.fqname) }}">{{ u.name|join(',') }}</a>{{ u.disabled and " (%s)" % _("disabled") or ""}}</td> at other places, you join with ", " (which is correct), please be consistent. https://codereview.appspot.com/101630045/diff/160001/MoinMoin/apps/admin/temp... MoinMoin/apps/admin/templates/admin/userbrowser.html:14: <td>{{ u.groups|join(',') }}</td> it looks like you are mixing whitespace (indentation) changes with functional code changes. it is advised to not do that, it just makes stuff harder to review. https://codereview.appspot.com/101630045/diff/160001/MoinMoin/apps/admin/view... File MoinMoin/apps/admin/views.py (right): https://codereview.appspot.com/101630045/diff/160001/MoinMoin/apps/admin/view... MoinMoin/apps/admin/views.py:257: all_items = flaskg.storage.documents(wikiname=app.cfg.interwikiname) better! :) https://codereview.appspot.com/101630045/diff/160001/MoinMoin/apps/admin/view... MoinMoin/apps/admin/views.py:264: 'itemid': item.meta.get(ITEMID, ''), is the empty string you give as default here better than just using None which get returns by default if you do not override the default? https://codereview.appspot.com/101630045/diff/160001/MoinMoin/apps/admin/view... MoinMoin/apps/admin/views.py:273: title_name=_(u'User ACL Info'), are you calling this template from somewhere else? if not, you could just put headline / title_name into the template...
Sign in to reply to this message.
https://codereview.appspot.com/101630045/diff/160001/MoinMoin/apps/admin/temp... File MoinMoin/apps/admin/templates/admin/user_acl_info.html (right): https://codereview.appspot.com/101630045/diff/160001/MoinMoin/apps/admin/temp... MoinMoin/apps/admin/templates/admin/user_acl_info.html:7: {% endif %} On 2014/07/12 15:17:37, Thomas.J.Waldmann wrote: > do you call it without headline also? Done. https://codereview.appspot.com/101630045/diff/160001/MoinMoin/apps/admin/temp... MoinMoin/apps/admin/templates/admin/user_acl_info.html:16: <th>{{ _("Admin") }}</th> On 2014/07/12 15:17:37, Thomas.J.Waldmann wrote: > acl permissions are written lowercase in the acl string. > > if you write them uppercase here, you might mislead users in thinking that they > can have them uppercase in acls, too. Please fix this everywhere where you did > it. > > also, maybe rather do not translate these (for similar reasons). Done. https://codereview.appspot.com/101630045/diff/160001/MoinMoin/apps/admin/temp... MoinMoin/apps/admin/templates/admin/user_acl_info.html:22: <td><a href="{{ url_for('frontend.modify_item', item_name=item['fqname']) }}">{% if item['name']|length %}{{ item['name']|join(', ') }}{% else %}Item Id : {{ item['itemid'] }}{% endif %}</a></td> On 2014/07/12 15:17:37, Thomas.J.Waldmann wrote: > do you need "| length" here? doesn't an empty list evaluate as falsy anyway? > > and please note that in many western languages, you NEVER put a blank before a > colon. Done. https://codereview.appspot.com/101630045/diff/160001/MoinMoin/apps/admin/temp... MoinMoin/apps/admin/templates/admin/user_acl_info.html:27: <td><p>{% if item['admin'] %}Admin{% endif %}</p></td> On 2014/07/12 15:17:37, Thomas.J.Waldmann wrote: > do you need the <p>? Done. https://codereview.appspot.com/101630045/diff/160001/MoinMoin/apps/admin/temp... File MoinMoin/apps/admin/templates/admin/userbrowser.html (right): https://codereview.appspot.com/101630045/diff/160001/MoinMoin/apps/admin/temp... MoinMoin/apps/admin/templates/admin/userbrowser.html:13: <td><a href="{{ url_for('frontend.show_item', item_name=u.fqname) }}">{{ u.name|join(',') }}</a>{{ u.disabled and " (%s)" % _("disabled") or ""}}</td> On 2014/07/12 15:17:37, Thomas.J.Waldmann wrote: > at other places, you join with ", " (which is correct), please be consistent. My bad. Although the join statement was written by a previous developer i mixed up the whitespace change so it may have caused confusion. https://codereview.appspot.com/101630045/diff/160001/MoinMoin/apps/admin/temp... MoinMoin/apps/admin/templates/admin/userbrowser.html:14: <td>{{ u.groups|join(',') }}</td> On 2014/07/12 15:17:37, Thomas.J.Waldmann wrote: > it looks like you are mixing whitespace (indentation) changes with functional > code changes. > > it is advised to not do that, it just makes stuff harder to review. Done. https://codereview.appspot.com/101630045/diff/160001/MoinMoin/apps/admin/view... File MoinMoin/apps/admin/views.py (right): https://codereview.appspot.com/101630045/diff/160001/MoinMoin/apps/admin/view... MoinMoin/apps/admin/views.py:264: 'itemid': item.meta.get(ITEMID, ''), On 2014/07/12 15:17:37, Thomas.J.Waldmann wrote: > is the empty string you give as default here better than just using None which > get returns by default if you do not override the default? Done. https://codereview.appspot.com/101630045/diff/160001/MoinMoin/apps/admin/view... MoinMoin/apps/admin/views.py:273: title_name=_(u'User ACL Info'), On 2014/07/12 15:17:37, Thomas.J.Waldmann wrote: > are you calling this template from somewhere else? > if not, you could just put headline / title_name into the template... Title name is used by an inherited template so have to pass that, although put the headline in the template itself.
Sign in to reply to this message.
https://codereview.appspot.com/101630045/diff/220001/MoinMoin/apps/admin/view... File MoinMoin/apps/admin/views.py (right): https://codereview.appspot.com/101630045/diff/220001/MoinMoin/apps/admin/view... MoinMoin/apps/admin/views.py:63: groups=user_groups)) Please ignore this change. This was caused due to update in repository.
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
