ACL Report view for admin to view acls of each item in a list and hyperlinks to modify them

ACL Report view for admin to view acls of each item in a list and hyperlinks to modify them

[RogerHaase, 2014-07-04 14:38:13]

looks ok after fixing comment, wait for tw

https://codereview.appspot.com/109440043/diff/110001/MoinMoin/apps/admin/temp...
File MoinMoin/apps/admin/templates/admin/aclreport.html (right):

https://codereview.appspot.com/109440043/diff/110001/MoinMoin/apps/admin/temp...
MoinMoin/apps/admin/templates/admin/aclreport.html:11: <table class="table
table-hover tablesorter tablesorter-default moin-sortable"
data-sortlist="[[0,0],[1,0]]">
a nit, but item name in col 0 must be unique, so there is no need to sort
columns 0 and 1

[Thomas.J.Waldmann, 2014-07-05 13:02:06]

https://codereview.appspot.com/109440043/diff/130001/MoinMoin/apps/admin/temp...
File MoinMoin/apps/admin/templates/admin/aclreport.html (right):

https://codereview.appspot.com/109440043/diff/130001/MoinMoin/apps/admin/temp...
MoinMoin/apps/admin/templates/admin/aclreport.html:15: <th>{{ _("ACL Rights")
}}</th>
ACL means Access Control List.

What are "ACL Rights"?

https://codereview.appspot.com/109440043/diff/130001/MoinMoin/apps/admin/temp...
MoinMoin/apps/admin/templates/admin/aclreport.html:21: <td><a href="{{
url_for('frontend.modify_item', item_name=item['name'][0]) }}">{{ item['name'] |
join(' | ') }}</a></td>
this will crash for nameless items.

https://codereview.appspot.com/109440043/diff/130001/MoinMoin/apps/admin/view...
File MoinMoin/apps/admin/views.py (right):

https://codereview.appspot.com/109440043/diff/130001/MoinMoin/apps/admin/view...
MoinMoin/apps/admin/views.py:260: all_items =
flaskg.storage.indexer.ix[LATEST_REVS].searcher().documents()
i think there is an easier way.

https://codereview.appspot.com/109440043/diff/130001/MoinMoin/apps/admin/view...
MoinMoin/apps/admin/views.py:266: items_acls.append({'name': item['name'],
'acl': app.cfg.acl_mapping[1][1]['default']})
crashes for nameless items.

instead of generating a (potentially huge) list of all items just to annoy the
administrator who has to find out then what's default acl and what's not, you
rather should tell that all not-listed items of course have the configured acls
applied and the listed ones additionally have the listed special acls.

alternatively, print "Default" if it is the default acl, so it is easier to see
them.

[ajgupta93, 2014-07-05 13:54:01]

https://codereview.appspot.com/109440043/diff/130001/MoinMoin/apps/admin/temp...
File MoinMoin/apps/admin/templates/admin/aclreport.html (right):

https://codereview.appspot.com/109440043/diff/130001/MoinMoin/apps/admin/temp...
MoinMoin/apps/admin/templates/admin/aclreport.html:15: <th>{{ _("ACL Rights")
}}</th>
On 2014/07/05 13:02:05, Thomas.J.Waldmann wrote:
> ACL means Access Control List.
> 
> What are "ACL Rights"?

I was referring to the ACL Strings. I'll change that to "ACL".

https://codereview.appspot.com/109440043/diff/130001/MoinMoin/apps/admin/temp...
MoinMoin/apps/admin/templates/admin/aclreport.html:21: <td><a href="{{
url_for('frontend.modify_item', item_name=item['name'][0]) }}">{{ item['name'] |
join(' | ') }}</a></td>
On 2014/07/05 13:02:05, Thomas.J.Waldmann wrote:
> this will crash for nameless items.

Ok.. i'll change both the view and 'modify_item' to add fqname support.

https://codereview.appspot.com/109440043/diff/130001/MoinMoin/apps/admin/view...
File MoinMoin/apps/admin/views.py (right):

https://codereview.appspot.com/109440043/diff/130001/MoinMoin/apps/admin/view...
MoinMoin/apps/admin/views.py:266: items_acls.append({'name': item['name'],
'acl': app.cfg.acl_mapping[1][1]['default']})
On 2014/07/05 13:02:05, Thomas.J.Waldmann wrote:
> crashes for nameless items.
> 
> instead of generating a (potentially huge) list of all items just to annoy the
> administrator who has to find out then what's default acl and what's not, you
> rather should tell that all not-listed items of course have the configured
acls
> applied and the listed ones additionally have the listed special acls.
> 
> alternatively, print "Default" if it is the default acl, so it is easier to
see
> them.

Ok.. I'll do that

[ajgupta93, 2014-07-07 12:00:18]

https://codereview.appspot.com/109440043/diff/130001/MoinMoin/apps/admin/view...
File MoinMoin/apps/admin/views.py (right):

https://codereview.appspot.com/109440043/diff/130001/MoinMoin/apps/admin/view...
MoinMoin/apps/admin/views.py:266: items_acls.append({'name': item['name'],
'acl': app.cfg.acl_mapping[1][1]['default']})
On 2014/07/05 13:54:01, ajgupta93 wrote:
> On 2014/07/05 13:02:05, Thomas.J.Waldmann wrote:
> > crashes for nameless items.
> > 
> > instead of generating a (potentially huge) list of all items just to annoy
the
> > administrator who has to find out then what's default acl and what's not,
you
> > rather should tell that all not-listed items of course have the configured
> acls
> > applied and the listed ones additionally have the listed special acls.
> > 
> > alternatively, print "Default" if it is the default acl, so it is easier to
> see
> > them.
> 
> Ok.. I'll do that

On second thought i'll implement the nameless items support once fqname support
is added to modify_item function, as stated in the other cr. After that is done,
i'll add fqname support to this view too.

[RogerHaase, 2014-07-07 13:05:43]

https://codereview.appspot.com/109440043/diff/150001/MoinMoin/apps/admin/temp...
File MoinMoin/apps/admin/templates/admin/aclreport.html (right):

https://codereview.appspot.com/109440043/diff/150001/MoinMoin/apps/admin/temp...
MoinMoin/apps/admin/templates/admin/aclreport.html:6: {% endif %}
indentation for block rather onconsistent

[ajgupta93, 2014-07-07 13:19:28]

https://codereview.appspot.com/109440043/diff/150001/MoinMoin/apps/admin/temp...
File MoinMoin/apps/admin/templates/admin/aclreport.html (right):

https://codereview.appspot.com/109440043/diff/150001/MoinMoin/apps/admin/temp...
MoinMoin/apps/admin/templates/admin/aclreport.html:6: {% endif %}
On 2014/07/07 13:05:43, RogerHaase wrote:
> indentation for block rather onconsistent

Done.

[Thomas.J.Waldmann, 2014-07-07 13:45:22]

https://codereview.appspot.com/109440043/diff/170001/MoinMoin/apps/admin/temp...
File MoinMoin/apps/admin/templates/admin/aclreport.html (right):

https://codereview.appspot.com/109440043/diff/170001/MoinMoin/apps/admin/temp...
MoinMoin/apps/admin/templates/admin/aclreport.html:17: <td><a href="{{
url_for('frontend.modify_item', item_name=item['name'][0]) }}">{{ item['name'] |
join(', ') }}</a></td>
^^

https://codereview.appspot.com/109440043/diff/170001/MoinMoin/apps/admin/view...
File MoinMoin/apps/admin/views.py (right):

https://codereview.appspot.com/109440043/diff/170001/MoinMoin/apps/admin/view...
MoinMoin/apps/admin/views.py:262: default_acl =
app.cfg.acl_mapping[1][1]['default']
[1][1]?

such stuff should make you suspicious. it is either bad because it is badly
readable or it might be bad because it is just wrong or not dealing with all the
cases.

[ajgupta93, 2014-07-09 10:05:29]

https://codereview.appspot.com/109440043/diff/170001/MoinMoin/apps/admin/temp...
File MoinMoin/apps/admin/templates/admin/aclreport.html (right):

https://codereview.appspot.com/109440043/diff/170001/MoinMoin/apps/admin/temp...
MoinMoin/apps/admin/templates/admin/aclreport.html:17: <td><a href="{{
url_for('frontend.modify_item', item_name=item['name'][0]) }}">{{ item['name'] |
join(', ') }}</a></td>
On 2014/07/07 13:45:21, Thomas.J.Waldmann wrote:
> ^^

Done.

https://codereview.appspot.com/109440043/diff/170001/MoinMoin/apps/admin/view...
File MoinMoin/apps/admin/views.py (right):

https://codereview.appspot.com/109440043/diff/170001/MoinMoin/apps/admin/view...
MoinMoin/apps/admin/views.py:262: default_acl =
app.cfg.acl_mapping[1][1]['default']
On 2014/07/07 13:45:21, Thomas.J.Waldmann wrote:
> [1][1]?
> 
> such stuff should make you suspicious. it is either bad because it is badly
> readable or it might be bad because it is just wrong or not dealing with all
the
> cases.

Done.

[Thomas.J.Waldmann, 2014-07-12 15:27:01]

https://codereview.appspot.com/109440043/diff/240001/MoinMoin/apps/admin/temp...
File MoinMoin/apps/admin/templates/admin/item_acl_report.html (right):

https://codereview.appspot.com/109440043/diff/240001/MoinMoin/apps/admin/temp...
MoinMoin/apps/admin/templates/admin/item_acl_report.html:6: {% endif %}
do you use it without headline?

https://codereview.appspot.com/109440043/diff/240001/MoinMoin/apps/admin/temp...
MoinMoin/apps/admin/templates/admin/item_acl_report.html:17: <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>
|length needed?

no space before colon.

https://codereview.appspot.com/109440043/diff/240001/MoinMoin/apps/admin/view...
File MoinMoin/apps/admin/views.py (right):

https://codereview.appspot.com/109440043/diff/240001/MoinMoin/apps/admin/view...
MoinMoin/apps/admin/views.py:263: item_namespace = item.meta.get(NAMESPACE, '')
is None a problem or why do you use ''?

https://codereview.appspot.com/109440043/diff/240001/MoinMoin/apps/admin/view...
MoinMoin/apps/admin/views.py:275: items_acls.append({'name': item_name,
'itemid': item_id, 'fqname': fqname, 'acl': 'Default (' + default_acl + ')'})
this does not belong into a view.

duplicate code

no comment about what this does / hard to read.

also, do not use ns[0] ns[1], but split the tuple into reasonably named
components.

[ajgupta93, 2014-07-14 14:49:14]

https://codereview.appspot.com/109440043/diff/240001/MoinMoin/apps/admin/temp...
File MoinMoin/apps/admin/templates/admin/item_acl_report.html (right):

https://codereview.appspot.com/109440043/diff/240001/MoinMoin/apps/admin/temp...
MoinMoin/apps/admin/templates/admin/item_acl_report.html:6: {% endif %}
On 2014/07/12 15:27:01, Thomas.J.Waldmann wrote:
> do you use it without headline?

Done.

https://codereview.appspot.com/109440043/diff/240001/MoinMoin/apps/admin/temp...
MoinMoin/apps/admin/templates/admin/item_acl_report.html:17: <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:27:01, Thomas.J.Waldmann wrote:
> |length needed?
> 
> no space before colon.

Done.

https://codereview.appspot.com/109440043/diff/240001/MoinMoin/apps/admin/view...
File MoinMoin/apps/admin/views.py (right):

https://codereview.appspot.com/109440043/diff/240001/MoinMoin/apps/admin/view...
MoinMoin/apps/admin/views.py:263: item_namespace = item.meta.get(NAMESPACE, '')
On 2014/07/12 15:27:01, Thomas.J.Waldmann wrote:
> is None a problem or why do you use ''?

Done.

https://codereview.appspot.com/109440043/diff/240001/MoinMoin/apps/admin/view...
MoinMoin/apps/admin/views.py:275: items_acls.append({'name': item_name,
'itemid': item_id, 'fqname': fqname, 'acl': 'Default (' + default_acl + ')'})
On 2014/07/12 15:27:01, Thomas.J.Waldmann wrote:
> this does not belong into a view.
> 
> duplicate code
> 
> no comment about what this does / hard to read.
> 
> also, do not use ns[0] ns[1], but split the tuple into reasonably named
> components.

Done.

[RogerHaase, 2014-07-14 16:30:27]

https://codereview.appspot.com/109440043/diff/300001/MoinMoin/apps/admin/view...
File MoinMoin/apps/admin/views.py (right):

https://codereview.appspot.com/109440043/diff/300001/MoinMoin/apps/admin/view...
MoinMoin/apps/admin/views.py:274: for config in app.cfg.acl_mapping:
I think he meant something similar to:

for namespace, acl_config in ap.cfg.acl_mapping:

so you avoid config[0]... below

[ajgupta93, 2014-07-14 17:14:24]

https://codereview.appspot.com/109440043/diff/300001/MoinMoin/apps/admin/view...
File MoinMoin/apps/admin/views.py (right):

https://codereview.appspot.com/109440043/diff/300001/MoinMoin/apps/admin/view...
MoinMoin/apps/admin/views.py:274: for config in app.cfg.acl_mapping:
On 2014/07/14 16:30:27, RogerHaase wrote:
> I think he meant something similar to:
> 
> for namespace, acl_config in ap.cfg.acl_mapping:
> 
> so you avoid config[0]... below

Done.

[Thomas.J.Waldmann, 2014-07-17 16:39:58]

https://codereview.appspot.com/109440043/diff/320001/MoinMoin/apps/admin/view...
File MoinMoin/apps/admin/views.py (right):

https://codereview.appspot.com/109440043/diff/320001/MoinMoin/apps/admin/view...
MoinMoin/apps/admin/views.py:268: if not item_acl:
this "not" triggers for "" and also for None.

i think you just meant "if item_acl is None:" and didn't want to include the
empty acl "".

https://codereview.appspot.com/109440043/diff/320001/MoinMoin/apps/admin/view...
MoinMoin/apps/admin/views.py:271: item_acl = 'Default (' + acl_config['default']
+ ')'
how about using string formatting?