CONTENTS OF THIS FILE
---------------------
* Introduction
* Requirements
* Installation
* GIT and WEB access
INTRODUCTION
------------
Current Maintainer: Adrian Dimitrov
Replace default permissions interface with more responsitive one,
usefule when website operate with large number of roles and permissions.
Feautures:
* AJAX loaded permissions table
* Filter which roles to show
REQUIREMENTS
------------
* Drupal 7.x
INSTALLATION
------------
1) Unarchive the zip and put directory into your sites/all/modules
2) Enable it from your admin panel -> Modules
GIT AND WEB ACCESS
-----------------
* Homepage: https://github.com/dimitrov-adrian/lighty_permissions_ui
* GIT access: https://github.com/dimitrov-adrian/lighty_permissions_ui.git
| Comment | File | Size | Author |
|---|---|---|---|
| #19 | dimitrov_errors.txt | 5.35 KB | novalnet |
Comments
Comment #0.0
dimitrov.adrian commentedDescription update.
Comment #1
komlenic commentedThis is a brilliant module that I think has some real utility. I tested it on a site with a particularly wide default permissions table, with several dozen roles.
Have you researched existing similar modules or considered combining your efforts? In a brief search I found:
None of those seem to do exactly what your module does, however.
That being said, I found a few problems:
I found a bug which can be duplicated by
There are some misspellings in your .info file. (responsitive, usefule)
You should probably make a sandbox project on drupal.org (see http://drupal.org/node/1068950 ) which will make it easier for other reviewers to test your module and get the code ready to be hosted on drupal.org. I did not run automated code testing on your module - be sure to run it through http://ventral.org/pareview or equivalent. In a quick read-through I did not see any obvious problems, though I did wonder if line 46 was necessary as $form is already declared to be an array in the function defaults?
Your README.txt in your master branch should probably only contain one line "See major version branches." as described at http://drupal.org/node/1127732 at step 5. Having a copy of your 7.x-1.x README.txt in master is probably a bad idea (though it may not technically block approval, I'm not sure) - consider the case where your module might have more than one version branch: which version of the README.txt would be in master?
Not knowing of any modules that do what this does, I would definitely use it.
Comment #2
dimitrov.adrian commentedI am solving the bug with the wrong redirection when showing/hiding description.
Also added storing a currently selected roles to settings, so while browsing they are stored.
There is no errors and notices while I was checking the module with phpcs with drupal configuration, and i am sure with phareview there is nothing too.
About the modules that you listing, they not doing what I am doing, I was researching about one day, and nothing, so i was starting to write this module.
And thanks for the interest, I hope that it's clear and useful enough to be approved.
Comment #3
adammaloneYou still need to put a link to your sandbox project page in the description to follow the guidelines for creating a project. This is your link: http://drupal.org/sandbox/dimitrov.adrian/1453946
It's also probably recommended that you link to your drupal git repo: http://git.drupal.org/sandbox/dimitrov.adrian/1453946.git
Comment #4
dimitrov.adrian commentedI putted the links just now, now I think that the description is by the guidelines.
Cheers.
Comment #5
dimitrov.adrian commentedComment #6
jygastaud commentedHi,
Automated review mention a small coding error on README.txt
Source: http://ventral.org/pareview - PAReview.sh online service
Comment #7
jygastaud commentedAfter enable of the module on 7.x-1.x branch via drush, I get notices and warnings on admin/people/permissions page:
Then when I click on "anonymous role" I get warnings
However, if I have a role checked, I haven't any warnings.
Also, I saw that when you are on page: admin/people/permissions/roles and you choose "edit role X", for example, role 2 (authenticated user) you're right redirect to admin/people/permissions/2 but the checkbox of the role is not checked.
Comment #8
dimitrov.adrian commented@jygastaud: thanks for the report, the bug is fixed now. Also fixed and the README.txt new line after end of file.
Comment #9
jygastaud commentedHi Adrian,
Well bug is fixed.
Howver still no effect when you are on page: admin/people/permissions/roles and you choose "edit role X", for example, role 2 (authenticated user) you're right redirect to admin/people/permissions/2 but the checkbox of the role is not checked.
Moreover, when you check (or uncheck) a lot of roles at the same time some roles are not loaded (or unloaded). Maybe you should limit the simultaneous treatments.
Comment #10
jygastaud commentedComment #11
dimitrov.adrian commentedBug on admin/people/permissions/roles and "edit role X" is fixed.
About the multiple selections in same time, i think this is problem on Drupal's form API 'radios', but will think about it.
Comment #12
patrickd commented@jygastaud
please don't post such automated reviews into issues in future, it's quite demotivating for applicants to hit this "wall of errors"
rather attach a txt-file containing the report, or a direct link to it. :)
Comment #13
jygastaud commented@patrickd
In fact errors come from a manual review but you right ;)
Next time I will do a txt file and attach it.
Thanks for advice.
Comment #14
dimitrov.adrian commentedThanks about the discussion.
Today I fixed and the "problem" with the multiple selections.
Comment #15
jygastaud commentedHi,
Nice fixed with checkbox. Works perfect for me and roles selection too.
I am not really able to provide suggests of optimizations but I noted that you should check your comments. Note tha every comments should end by . , ? or !. (eg: Line 19 of .module).
Also automated review suggests you remove the blank line at the end of comments 28 and 42.
Comment #16
dimitrov.adrian commentedHello,
10x for the report, seems my configuration of phpcs with Drupal standards missing it.
However, I fixed it just now.
Comment #17
dimitrov.adrian commentedFixed.
Comment #18
jygastaud commentedHi,
I will make a in deep review of your module asap to if need complete my previous comments.
From now my first advice will be to get a review bonus by reviewing other modules in pending.
Read that (http://drupal.org/node/1410826) for details.
That really help me get approuved quickly, so you should consider to do that also.
Hope your module will be soon pubish cause it works well and could be really useful in some context.
Comment #19
novalnet commentedReview of the 7.x-1.x branch:
This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. Get a review bonus and we will come back to your application sooner.
Source: http://ventral.org/pareview - PAReview.sh online service
Comment #20
dimitrov.adrian commentedThis is other my module that have no relation with current?
Comment #21
crobinson commentedYes, the automated review from http://drupal.org/node/1453964#comment-5804294 is not for this module. Do you have two applications open in the queue? This is a common misunderstanding: this process is about reviewing authors and their coding skills, not reviewing modules. Do you have multiple modules submitted for review? If so, pick just one and focus on that. Once the module passes review, you'll have full access to create new modules without going through the process again.
I have done a full code review of this project. First, it is enormously useful and I look forward to it becoming a full project. I think there are a few final things to address first, though.
1. SUGGESTION: Is it necessary to store the role selector value in a variable? On a site with multiple administrators this means they will all be remembering the same values when returning to the form. This is bad if those users commonly manage different roles (you have a Human Resources admin, a Content admin, an Events admin, a News admin, etc.) Alternative: use user_save() to store this setting in the user's 'data' field.
This is a module-quality suggestion, not a code-quality suggestion. I do not believe you should be required to address it before gaining approval, nor should other reviewers use it as a pre-requisite. I'm just passing it along as an important feature for bigger sites.
2. You do not need to change this, but please be aware that while function documentation is good, you do not need to document it so fully in some cases. Functions such as lighty_permissions_ui_change_view are standard form callbacks and you don't need to document @param/@return there. It is NOT necessary to remove this - again, just passing it along.
It's better to focus on better documenting the ones that do real work, like lighty_permissions_ui_user_admin_permissions(), especially in the body of code - what is each block of code doing, and why?
3. The Drupal community has established a guideline regarding 80-column lines of code. Your structure gets past automated code review, but it's pretty obvious you're on the edge here. I'm actually personally not a huge fan of this limit myself, but it is the standard, and there are people that code on netbooks (I presume).
You should try to break up some of these long lines, particularly 12-14, 52, 134, and 140. Line 134 is an example of a grey area because Drupal core code regularly does this sort of thing with long strings, but I'd say it should be split because there's a ternary operator here and it takes a lot of horizontal scrolling to find the else condition.
4. The AJAX callback is very slow even on a relatively empty site with only two test users. On a big test site with 30 users and several dozen modules installed (lots of permissions) it takes so long that my browser times out while waiting for the response.
I have two suggestions for addressing this. The first is to do profiling. You are using many PHP array modifiers like array_intersect_key() and array_filter() and some of these are slow. Some of these issues were fixed in later versions of PHP. If you cannot resolve the issue without PHP 5.3.x you should note this in your README.txt because Drupal officially only requires PHP 5.2.x.
You can also do caching and it looks like it would be very helpful here, especially for things like the results of this code:
These callbacks can be expensive, and they change very rarely - only when modules are enabled/disabled. The user module gets away with this kind of code because it doesn't have an AJAX call to deal with.
5. In these same lines of code, you should call user_permission_get_modules() instead of doing the module_invoke yourself. You can then cache the results of this function and loop through that to modify your form.
6. Did you select the name of your project for a specific reason? "Lighty" is a popular Web server product, and I was confused about this project's relationship (or lack of a relationship) with that project. I initially thought it was a permissions GUI for Drupal sites running on Lighty. You should mention this in your README and project page description if you keep the name.
7. Your JS file uses a document ready() call instead of a Behavior. There are use cases where this is acceptable, but this does not seem to be one of them. Also, you are not using the standard jQuery closure all modules are required to use. Please examine a JS file from almost any other module.
8. There is a missing semicolon on line 19 of your JS file.
9. You are targeting "#edit-show-role-selector :checkbox". A faster/better technique is "#edit-show-role-selector .form-checkbox" (or even just 'input', since nothing else is in this DOM wrapper).
10. Your lines are beyond the 80-column limit here as well. This seems unnecessary because the only reason they are this long is your use of a 32-letter variable name and jQuery instead of $.
Comment #22
klausiClosing due to lack of activity. Feel free to reopen if you are still working on this application.
Comment #23
klausiProject 1: http://drupal.org/node/1587176
Project 2: http://drupal.org/node/1772350
Project 3: http://drupal.org/node/1453964
Project 4: http://drupal.org/node/1406352
Project 5: http://drupal.org/node/1438600
Project 6: http://drupal.org/node/1929114
As successful completion of the project application process results in the applicant being granted the 'Create Full Projects' permission, there is no need to take multiple applications through the process. Once the first application has been successfully approved, then the applicant can promote other projects without review. Because of this, posting multiple applications is not necessary, and results in additional workload for reviewers ... which in turn results in longer wait times for everyone in the queue. With this in mind, your secondary applications have been marked as 'closed(duplicate)', with only one application left open (chosen at random).
If you prefer that we proceed through this review process with a different application than the one which was left open, then feel free to close the 'open' application as a duplicate, and re-open one of the project applications which had been closed.
Comment #23.0
klausiUpdate git URL.
Comment #24
avpaderno