Basecamp

Manual Review

basecamp.admin.inc
- basecamp_admin_form: Make sure to enclose #description items in t()
- basecamp_account_report: consider using a theme for this, or at least wrap your output text with t()
- basecamp_people_report: wrap your output with t()

basecamp.blocks.inc
- basecamp_block_basecamp_tasks: wrap your links with t(), "edit" won't be translated currently
- basecamp_block_basecamp_create_task: $options[$record->config_id] = t($record->config_name); don't pass variables to t(), as it requires translators to go back through and find out actual text being translated; if it has to be a variable, translate it when you generate the value

I don't believe you should be using t() if $record->config_name is user-supplied text, as it appears to be. User-supplied text cannot be translated as you never know what it will be. You should use check_plain() or one of the other sanitizing functions listed here http://api.drupal.org/api/drupal/includes!common.inc/group/sanitization/7 since the entire contents of t() in your existing code is user-supplied.

Consider:
$options[$record->config_id] = check_plain($record->config_name);

project page

Please take a moment to make your project page follow tips for a great project page.

  $items['admin/config/services/basecamp'] = array(
    'title' => 'Basecamp',
    'description' => 'Configure settings for the Basecamp API.',
    'page callback' => 'drupal_get_form',
    'page arguments' => array('basecamp_admin_form'),
    'access arguments' => array('administer basecamp module'),
    'file' => 'basecamp.admin.inc',
  );

  $items['admin/config/services/basecamp/settings'] = array(
    'title' => 'Settings',
    'type' => MENU_DEFAULT_LOCAL_TASK,
    'weight' => 10,
  );

also if your settings menu item comes after the protected basecamp item you should add a proper access argument to any menu item!

as there are currently many applications in queue we need more reviewers,
so think about getting a review bonus and we will come back to your application sooner.

Hi great idea,

I'm able to see my assigned issues which is great, however when I try to add one I receive this error:
'Unprocessable Entity(400)'
I only receive this error when I don't assign anyone automatically.
Anyhow I should not receive a Task submitted. message when no task has been created.

some code formatting issues:
http://ventral.org/pareview/httpgitdrupalorgsandboxchrisleversuch1433882git

Hello

Manual Review :
1. Please use placeholders for dynamic messages.you can use placeholders in drupal_set_message(t($reply->error) . '(' . $reply->code . ')', 'error');
2. Use t() for all texts used in basecamp.module .
ex : Function basecamp_menu() contains 'title' => 'Basecamp' can be 'title' =>t('Basecamp')

Thanks

ex : Function basecamp_menu() contains 'title' => 'Basecamp' can be 'title' =>t('Basecamp')

Don't wrap your hook_menu titles in t(), as the default title callback for all menu entries is already the t() function. By doing so, you're calling t() twice on the same text (in essence you're doing t(t('Title')), very inefficient), which is unnecessary. See hook_menu's title callback attribute for further details.

I think it looks very nice now.

I wonder why you named your block functions basecamp_block_basecamp_xxx() - I guess it is to ensure no clashes with hook_block_xxx but it does make the function names quite long.

Not a biggie, though.

Maybe I doing something wrong but I can't find "My Info (top right)" for API Token.
In top right corner I have only "New features | Account | Upgrades | Sign out". Is there was site update?

Using http://groups.drupal.org/node/184389 and http://drupal.org/node/894256 as a template for this review.

README.txt

Check

project page

Check - As detailed as something like this could be.

Master Branch

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.

Major coding standards / best practice issues

An automated review of your project via PAReview.sh has found 1 issue(s) with your code.
152 | ERROR | Function comment short description must end with a full stop

An automated review of your project via Coder has found 1 issue(s) with your code.
Line 168: The $message argument to drupal_set_message() should be enclosed within t() so that it is translatable.

License

No License file, check.

access administration vs. administer site configuration

Irrelevant for this project. Project implements its own permissions.

files[] without classes or interfaces

None, check.

Licensing issues

No License file, check.

Multiple Applications

No. This is the only one.

Already Approved

Wouldn't that be nice?

Idle Application

Nope, still active.

Code too short

Project meets minimum requirements.

Duplicate module

No. There have been discussions, but no implementations. There is Open Atrium's Basecamp Import, however it is marked obsolete and I wouldn't consider this a duplicate of that anyway.

Attaching screenshots of the PAReview.sh and Coder outputs.

This seems really close to being done. Code looks clean, module works as expected and I don't see any glaring security holes that need to be fixed before pushing this through. Fix the piddly little errors and drop the master branch, then I'll mark it RTBC.

Awesome job, I will be using this module.

Edit: By the way, I used my company's Basecamp API stuff and this worked flawlessly with the Basecamp Classic projects.

Downloaded and installed and worked as expected.

Did not find any major issues in the manual review.

@chris.leversuch I'm not sure I have that authority. I'll try to get in touch with klausi for you though. Good stuff man -- it's at least RTBC in my head.

Sure you can set it to RTBC.

This must have fallen through the cracks somehow, sorry. Chris, accept my apologies.

Thanks for contributing to Drupal!

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.