This module allowes it to distribute the actual user-administration to administrators of a certain domain.
Thereby simplyfying the overview of actual users on a domain.
This can be done by an actual page, created by this module or over views.
The latter is currently recommended for larger sites since it allowes for additional filtering and pagersupport.

Simply install the module go to /admin/structure/domain/ selected added tab, configure rights per domain grant user the additional right and log in as said user.
you can now add users, select,edit or delete them as long as they hold roles specified for that very user or domain.

----------------------
Git-Link:
git clone --recursive --branch 7.x-1.x http://git.drupal.org/sandbox/cupcakemuncher/1837236.git domain_rights_management

Project-Link:
http://drupal.org/sandbox/cupcakemuncher/1837236

Participation:
Patch:
http://drupal.org/node/887910#comment-6933262

Comments

bripatand’s picture

It seems there are issues with your repository:

  • You should name your branch 7.x-1.x. 7.x-1.x-alpha should be a release (tag).
  • Modules file (.info, .module etc...) should be at the root folder where the .git folder is. In your case, they are in a sub folder domain_rights_management.
  • There is a typo in the git command above (double underscore in the folder name)

I would suggest you make an automatic review of your module here http://ventral.org/pareview

There are empty functions/hooks in you .install module. Unless you intend to implement them, I'd suggest you remove them.

I think it is better to use lowercase and underscore to name your tables. I could not find a standards as such in Drupal docs but most modules seem to respect this convention (and the core as well).

cupcakemuncher’s picture

Hi bripatand!
Thank you for the prompt feedback and the heads up on my messed up branch and tag-naming.
That one is fixed.
I did not know about the module not supposed to be put in a subfolder, which I fixed as well.
Nameing-Convetion for the tablenames is adhered.
I want to implement the hook_uninstall as to erase all the rights granted to the users, so that
once the module is activated I do not have users already assigned the role of local administrator.
This does not seem to bother other modules, for I did not see such provisions in their hook_uninstall.
Still thinking about implementing this mechanism, but I do not want to mess around a lot with the code while under revision, same with the naming of the database-tables. I think about refactoring and using defined constants to hold the tablesnames, as opposed to having to refactor the entire code using the IDE's search-and-replace-feature.

The typo in the checkout-command really isn't one.
I used the drupal.org's menus and checked the box for no-maintainer and made a straight copy'n'paste form there, so there is a purpose for this typo, I assum protecting the folks checking out.
But as requested, I will change it in my previous post.

I did a review on http://ventral.org/pareview before checking it in.
No errors I assum it was because of the subfolder-issue.
If I am not mistaken it uses the coder-module which I have installed locally.

Unless s.o. points me in the direction on how I can fix the following issues I will continue to ignore them, since even the domain-module fails the testing of the coder-module, on critical-setting and my one passes.
Setting it on normal or minor produces one of the following errors, depending on the setting
(and editing the code as requested does not sooth the coder-modules tests):

The check of the coder-module seems to not recognise a url after the @see-tag.
only one delimiter between @see and the url and it says that @see is to be followed by either a filename.... or a url.
So the url is in there and the coder-module still does not like it.
The codermodule also complains :
Line 36: Use an indent of 2 spaces, with no tabs
in the .module-file.
Even after I indeted the entire code 2 spaces(excluding <?php) and in case it uses the cache, running update.php.

It also complaints that in the following construct(original layout) the last */ should be indeted with 2 spaces:
/**
* some text
* ....
*/

Which would look bad, and the purpose of a styleguide is to produce easy to read, nice to look at, uniform code.
Here once again the domain-module, behaves the same and does not indet the */ with 2 spaces.

All errors pointed at on the minor-setting of the Coder-module are fixed as well as the spaces for the { brakets after the ) brakets and the spaces between parameters and conditions if (...) {...}.

paravibe’s picture

Please add a right git link. There is no 7.x-1.x-alpha branch.

Manual review:
The name of your project folder is "domain_rights__management", should be "domain_rights_management".

domain_rights_management.module: line 13, 18, 196 don’t use a CamelCase. Should be local_administration, administrator_domain_user_settings and so on.
line 107, there is no need to specify MENU_NORMAL_ITEM, because it uses by default.
domain_rights_management.install: Should be better formatted as for me. Line 9, your uninstall function is empty.
includes\ConfigurationMenuFormGeneration.inc, includes\HelperFunctions.inc and here also a camelcase.

paravibe’s picture

Issue summary: View changes

domain_rights__management changed into domain_rights_management as requested below.

cupcakemuncher’s picture

Perfect timing on your part;) Issue regarding the __ already addressed in my response above.
Was genereated by drupal.org itself.
Thanks for the hint with the wrong branch, totally missed that one.(took the -alpha out manually)
CamelCase-issue addressed.
I leave line 107 in there, does no harm, and everyone knows what type that link is even if he/she does not know the default-value.

I'll get to implementing hook_uninstall() within the next week, so it remains as well.
Regarding the formatting of the .install-file, if you tell me what bothers you, I can fix it.
cheers

cupcakemuncher

vineet.osscube’s picture

Status: Needs review » Needs work

Manual Review :

  • Use an indent of 2 spaces, with no tabs in all files.
  • Control statements should have one space between the control keyword and opening parenthesis at line 107 in includes/helper_functions.inc
  • Use uppercase for PHP constants, e.g. TRUE, FALSE at line no 111 in includes/helper_functions.inc
  • Use t() function at line no 25 in domain_rights_management.views.inc file

I suggest you to take a look at this page:
http://git.drupal.org/sandbox/cupcakemuncher/1837236.git

Here you can check source code whether it meets drupal coding standards or not, and advise you what to change in your code. You can repeat review after your commits, and can fix those errors.

cupcakemuncher’s picture

Hi!
I finally got around to refining the style.
Array-Indendtion should be fixed now.
Constants are written in capital letters now.
t() is used.
Escaping ' is circumvented using " in this rare cases.(went with the suggesting of the onlinechecker).
hook_uninstall is implemented.
Controlestatments do have spaces after them now, as do commas and brakets for functioncalls.

I also indented the entire file content, excluding <?php .
Now the onlinechecker complaints about wrong indention(over 90% of the errors now), so ...
I either do not indent the code within a function with an initial 2 spaces, or
you are wrong, I missunderstood you, or the checker is not up to date.
sincerely

cupcakemuncher

cupcakemuncher’s picture

Status: Needs work » Needs review
cupcakemuncher’s picture

Issue summary: View changes

forgot to change branch.

karljohann’s picture

Automated review:

http://ventral.org/pareview/httpgitdrupalorgsandboxcupcakemuncher1837236git

  • README.txt should be checked for spelling errors, i.e. module is spelled "modul" numerous times. :)
cupcakemuncher’s picture

Hi!
Ok so indenting the entire file with 2 spaces was a misunderstanding then.
Fixed that one.

Removed all the spelling mistakes I found in README.txt.

Fixed an error in hook_domain_insert().
Surfaced due to removing former includes and using module_load_include() instead.

Had my share of trouble with git, especially a quirky setup concerning smartgit, and openssh.
(So don't even try to understand the commits and branches I made there.)
So fixed my toolchain as well.

Removed ALL the mistakes the onlinechecker found and renamed loaderfunction.

Fixed errors in the documentation.

http://ventral.org/pareview/httpgitdrupalorgsandboxcupcakemuncher1837236git

alexmoreno’s picture

Hi cupcakemuncher,

thanks for your contribution and effort contributing this module.

I've ran the Code review (drupal module) over your module and, everything seems fine but in this file: configuration_menu_form_generation.inc

configuration_menu_form_generation.inc

Line 21: String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms [style_string_spacing]
  $ret    .= l(t('Add user'), 'admin/useradministration/CreateUser');
Line 22: String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms [style_string_spacing]
  $ret    .= '</li></ul>';
Line 64: String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms [style_string_spacing]
    $links    .= l(t('edit'), 'admin/useradministration/AlterUser/' . $array['uid']) . ' ';
Line 65: String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms [style_string_spacing]
    $links    .= l(t('delete'), 'admin/useradministration/delete/' . $array['uid']) . ' ';
Line 68: String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms [style_string_spacing]
      $links  .= l(t('unblock'), 'admin/useradministration/unblock/' . $array['uid']);
Line 71: String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms [style_string_spacing]
      $links  .= l(t('block'), 'admin/useradministration/block/' . $array['uid']);
Line 219: Control statements should have one space between the control keyword and opening parenthesis [style_control_spacing]
        catch(Exception $r) {
Line 245: Control statements should have one space between the control keyword and opening parenthesis [style_control_spacing]
    catch(Exception $r) {
Line 357: Control statements should have one space between the control keyword and opening parenthesis [style_control_spacing]
    catch(Exception $e) {

The paraview online tool seems to say everything is ok.

http://ventral.org/pareview/httpgitdrupalorgsandboxcupcakemuncher1837236git

Everything else seems fine and it is well commented.

Thanks again.

cupcakemuncher’s picture

Hi!
And I have to thank you for the compliment.
Isn't as much documentation as I initially wanted to keep, though.

I changed line 357, 245, 219.
The other outputs are triggered by the shorthand '.=' .
The errors are false positives because it is not possible to seperate '.' and '=' within the '.='-shorthand.
Same with domain_rights_management.module Line 18 and following.

Output:
Line 18: Use uppercase for PHP constants, e.g. NULL, TRUE, FALSE
'restrict access' => TRUE,

Sincerely

cupcakemuncher

klausi’s picture

We are currently quite busy with all the project applications and I can only review projects with a review bonus. Please help me reviewing and I'll take a look at your project right away :-)

alexmoreno’s picture

Status: Needs review » Needs work

cupcakemuncher, as klausi points, I recommend you to review at least three other projects and include them in the body of your application to proceed with your own project approval.

klausi’s picture

Status: Needs work » Needs review

@urwen: you did no manual review, so why do you set this to "needs work"?

alexmoreno’s picture

sorry if I made a mistake with this klausi, these are my first code reviews, i'm just learning alone :-(. I see that I don't exactly know when to set each status.

thelee’s picture

Manual review

This is fairly nitpicky, but especially in includes/configuration_menu_form_generation.inc, you have many typos related to the word "cancel" (which you spell "cancle"), both in user-facing URLs and in internal references. There are various other nitpicky grammar-related issues (not capitalizing the start of drupal_set_message() logs, not providing adequate spaces after a full stop) that, while not crucial, add to the overall end-user experience (UX).

Other than that, I think the rest of your code (and your module) is pretty solid and robust. The only other suggestion I would have is a more consistent use of db_query versus db_select; from my understanding, db_select is really intended for use for overly complicated queries with many parameters and db_query is much faster, but someone more expert with the database API should comment.

Hope you get cleared/approved soon!

thelee’s picture

Status: Needs review » Needs work
cupcakemuncher’s picture

Status: Needs work » Needs review

Hi thelee!
Thank you for the compliment.

Unfortunatly the day this project will be cleared might be a long while off.
Currently I do not have time to revisit other projects because my job requires me to learn new systems I am developing for.
Regardless of this fact, I will maintain this module.

Updated all your named issues regarding the end-user-experience.
Using db_select is no accident.
I chose db_select over db_query to provide a better understanding of the code from a programmers perspective,
to guarantee easier changes for future adaptions, to seperate single conditions,etc. and for not having to think about the underlying database (after all there are some subtle differences between the SQL-dialects) .
Only exception to this rule is in hook_uninstall, since that is a simple delete-query. All other queries are....bigger.
Besides AFAIK the major bottleneck of a webapplication is database-access, so I am not really worried about the few extra ticks spent on building the query.

In addition to that I got rid of some unused variables I stumbled upon.(legacy from earlier phases of development)
Patched a major bug regarding form-submission.
(A parameter had to be accepted as by-reference.(see function domain_rights_management_alter_user_form_submit() ) ).

Sincerely

cupcakemuncher

P.s.:Currently ventral.org seems to have issues with the validation. The changes made weren't major though. So I think it would validate just fine. The Coders-module tells everything is ok, except the usual false-positives.

thelee’s picture

Status: Needs review » Reviewed & tested by the community

cool, i hope no one minds me moving this in to RTBC then.

patrickd’s picture

Assigned: Unassigned » patrickd
patrickd’s picture

Status: Reviewed & tested by the community » Fixed

Your project page could be prettier, please have a look at the tips for a great project page, you may also use HTML-tags for better structure and screenshots.
Also the Readme looks a lot more informative, maybe you want to keep readme and project page in-sync? (few people are reading readme's)

Please set the 7.x-1.x as the default project branch! To do this go to http://drupal.org/node/1837236/edit/default-branch.

Code looks in general good enough for me but it could use some more inline comments ;-)

Thanks for your contribution!

I updated your account to let you promote this to a full project and also create new projects as either a sandbox or a "full" project.

Here are some recommended readings to help with excellent maintainership:

You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and get involved!

Thanks, also, for your patience with the review process. Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.

Thanks to the dedicated reviewer(s) as well.

cupcakemuncher’s picture

Thank you for promoting my project and adding the ability to make full projects.

I will sync readme-file and projectpage asap and thank you for the links.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

Issue summary: View changes

Documentation of my involment/participation in the drupal-community.