The Top of the Web (Bedst på nettet)

Disclaimer: I am a rookie reviewer (this is my first review), so there may be errors in the review below. I hope somebody more experienced also will chip in.

Automated review results

An automated review of your project using PAReview found two issues with your code. You may already know about these, but for the record, I've reproduced those below. As coding standards make sure projects are coded in a consistent style it may be a good idea to fix them. However, note that issues found are possibly false positives and fixing all issues is not a requirement for getting through the application process.

Master Branch (from PAReview)

It appears you are working in the "master" branch in git. You should really be working in a version specific branch. The most direct documentation on this is Moving from a master branch to a version branch. For additional resources please see the documentation about release naming conventions and creating a branch in git.

Translations (from PAReview)

Remove the translations folder, translations are done on http://localize.drupal.org

Translations

The strings in hook_menu seem to not be translatable.

What it does

From what I understand, this project just places the magic key sniffed by the "bedst paa nettet" website at the appropriate places in your Drupal website, and allows you to change or remove this key at at later date. I checked out the operation, and it worked as I expected, without any problems. However, I think the documentation (on the project page and in README.txt) should be clearer. All it currently says is: "This module provides easy to set up integration with The top of the web". This is true - but it does not really tell me what making the module part of my site entails. I understand that you provide separate documentation where you tell webmasters how to insert the key manually, so it may be self-evident that the module just automates this manual process, but I still think that it is a good idea to make the module's documentation self-contained.

Suggestion

The following note appears in README.txt: "you will not recieve errors for incorrect keys, so make sure the key you enter is the right
one." If you made the last character in the key some sort of check character, you may catch most simple typing errors.

I reviewed the module from a code perspective.

Overall it looks neat but some points could be improved:

• It's cleaner to not insert variables in a string
  • drupal_add_js("https://popin.survey-xact.dk/dynjs/$key/popin.js", 'external'); should be
  • drupal_add_js('https://popin.survey-xact.dk/dynjs/' . $key . '/popin.js', 'external');
• The javascript code should be moved to a separate file, using Drupal.settings to pass information from php to JavaScript.

If these two small issues and the issues mentioned in the previous comment are solved I see nothing standing in the way of releasing it as a full project.

Thank you for responding about hook_menu translations. You're right, of course.

I think the improved README.txt is fine.

I agree that you validate the key format (in hook_validate), but you only check the length and general format. A check character would make it much more robust against typing errors. But this was only a suggestion, and not something that should stop it from being promoted to a full project.

To my untrained eyes, it appears as if the two points raised by michielnugter has also been fixed, but I leave it to him to make the final call on that.

What remains from my first review is the following, pulled directly from PAReview:

Master Branch (from PAReview)

It appears you are working in the "master" branch in git. You should really be working in a version specific branch. The most direct documentation on this is Moving from a master branch to a version branch. For additional resources please see the documentation about release naming conventions and creating a branch in git.

The way I understand this requirement is that a Drupal project should not have a "master" branch. Instead, the working branch for a project destined for D7 should be called something like "7.x-1.0" (see the links that PAReview provides). (My apologies in advance if I've misunderstood this point about branch name conventions.)

It appears you are working in the "master" branch in git. You should really be working in a version specific branch. The most direct documentation on this is Moving from a master branch to a version branch. For additional resources please see the documentation about release naming conventions and creating a branch in git.
Review of the master branch:

• Remove the translations folder, translations are done on http://localize.drupal.org
• Coder Sniffer has found some issues with your code (please check the Drupal coding standards). See attachment.

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. You have to get a review bonus to get a review from me.

manual review:
What is the purpose of the module? The project page does not mention anything. What does it do and why should a user install it? See also http://drupal.org/node/997024

Otherwise I think this is nearly ready. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Master Branch (from PAReview)

There is still a master branch, make sure to set the correct default branch: http://drupal.org/node/1659588 . Then remove the master branch, see also step 6 and 7 in http://drupal.org/node/1127732

Translations (from PAReview)

Remove the translations folder, translations are done on http://localize.drupal.org.
I understand that the module is targeted for a Danish audience, and that having a translation into Danish is important. However, having a Danish translation available is not a requirement for passing the review. I therefore suggest that you remove the translations folder for the purpose of the review (to keep PAReview happy). You'll be able to add them back though the standard interface (i.e. localize.drupal.org) as soon as the project is promoted to a full project.

Code sniffer (from PAReview)

The file bedstpaanettet.info does not end in a newline character.

Documentation

I notice that the project page is now updated with the information already placed in README.txt that states the purpose of the module, and what it does. However, after looking closer at the module, I think the user documentation still needs to be improved. Who is the target audience for the module? What benefits can be gained from injecting the scripts and the key into one's site?
The screen-shot on the configuration page tells the user how to find the key when stage 3 of some registration process is reached, but not how to enter into this registration process. It also unclear if registration is open to everybody, or only to invitees (from the material on http://www.bedstpaanettet.dk/, it looks as if the latter is the case).

The JavaScript part is still incorrect and now not working. You really should test your code before publishing it on Drupal.

You cannot use php variables in an external JavaScript file. You can change it to the following.

Add the following line in the init hook:
drupal_add_js(array('bedstpaanettet_key' => $key), ' setting');

And change the JavaScript line to:
window.xact_surveyKey = Drupal.settings.bedstpaanettet_key;

Some other tips (no show stoppers but in general it will make it just that bit cleaner):

• The directory scripts is a bit too general, it could be any script. I looked at a lot of major contrib modules and the consensus seems that you either place a javascript file in the root with the projectname as the filename or if you have multiple files place them in a directory named js.
• The image key.png is not in the most optimal format. PNG is often very bad at normal images. It's not displayed a lot so it's not much of a problem but consider the attached GIF version. It's the same qualily but 100KB less.

The changes look good to me! I'm setting it to Reviewed as I can't think of any other improvements..

Revised GIT information

Thanks for your contribution, jm@bellcom.dk!

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:

• Dries' post on Responsible maintainers
• Best practices for creating and maintaining projects
• Maintaining a drupal.org project with Git
• Commit messages - providing history and credit
• Release naming conventions.
• Helping maintainers in the issue queues

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.

Oh, I forgot:

manual review:

1. bedstpaanettet_admin_form(): doc block is wrong, this is not a hook. See http://drupal.org/node/1354#forms
2. Same for bedstpaanettet_admin_form_validate().