JIRA Issue Collector

Hi Kasperg:

In your jira_issue_collector_permission function, what's the permission "use PHP for JIRA Issue Collector visibility"? It seems it not be used.

Hi kasperg,

Thanks very much for sharing this very interesting module.

Very quick thing (it would be great if you could fix that):

Project Automated Review reported some issues visible at:
http://ventral.org/pareview/httpgitdrupalorgsandboxkasperg1867570git
(very minor - warning)

FILE: ...al-7-pareview/sites/all/modules/pareview_temp/test_candidate/README.txt
--------------------------------------------------------------------------------
FOUND 0 ERROR(S) AND 1 WARNING(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
2 | WARNING | Line exceeds 80 characters; contains 105 characters
--------------------------------------------------------------------------------

I just thought you might have missed that (sorry.... you're going to have to break that link :-) ).

Otherwise Project page looks good, with screenshots and installation instructions [Project page + README.TXT OK].

Master branch has been removed as well [OK]

Other points of Automated Review seem to be fine.

Manual Review: Watchout for unistall variables
I haven't checked the code that much to be honest, but I saw a few variable_get calls around.
Usually that means you've defined variables, in this case looks like it comes from jira_issue_collector_admin_settings_form, for example: jira_issue_collector_code.
So I just wanted to recommend that you should perhaps double check that when the module is uninstalled the variables should be deleted.
For that you could perhaps have a look at other modules, for example: Chosen, chosen.install (I tried to pick a rather simple module as an example).
I haven't taken the time to test that myself yet, but thought it might be useful to point that out.

That's all I could find so far, but I will certainly keep checking and let you know if I am able to find anything else.
I hope these several comments can help improving this great module.

Cheers!

Hi kasperg,

Thanks for your explanation. It is a good use of role&permission to do UI visibility.

Hi.

Manual review

** jira_issue_collector.module, line 44 in jira_issue_collector_permission()

Instead of manually adding warning to a permission's description, you should use 'restrict access' key described here. It will add warning automatically.

** jira_issue_collector.module, line 96

Why do you use array_sum() to check the size of an array? It would be much more natural to use count() function or empty() construct to do this check.

** jira_issue_collector.module, line 127

Consider using drupal_static() to store static variable.

** jira_issue_collector.admin.inc, line 32

Is there any particular reason why a text field is so long (size==260)? In my browser, the width of this text field is 2356px, which is much wider than my display and it shows the horizontal scroll bar.

** jira_issue_collector.admin.inc, line 116

This regular expression is not valid. ? is a special character, it must be either escaped or removed. Such string is matched by this regexp:

http://example.com?junkCollectorId=123

Update 1:

** jira_issue_collector.admin.inc, line 11

Declaration of the jira_issue_collector_admin_settings_form() is not valid. It should accept two parameters: $form and &$form_state. Therefore, $form should not be declared on the next line.

** jira_issue_collector.admin.inc, lines 48-52

      $roles === $role_options
    

There is no need to create new array, simply use $roles on line 57.

manual review:

1. !url: "' . check_plain($url) . '",": this should use check_url() instead, no?
2. jira_issue_collector_init(): put the javascript into a dedicated JS file and use Drupal.behaviors instead of document.ready. Use Drupal.settings to pass down variables from PHP to Javscript. See http://drupal.org/node/756722
3. _jira_issue_collector_visibility_pages(): why do you need the static cache here? the function is only called once? The comment says "Cache visibility setting in hook_init for hook_footer." but there is no hook_footer()?

Not major blockers, so I guess this is RTBC. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

no objections for more than a week, so ...

Thanks for your contribution, kasperg!

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.