[D7] Credly

There are some errors reported by automated review tools, did you already check them? See http://pareview.sh/pareview/httpgitdrupalorgsandboxmajorrobot2236277git

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and put yourself on the high priority list, then we will take a look at your project right away :-)

Also, you should get your friends, colleagues or other community members involved to review this application. Let them go through the review checklist and post a comment that sets this issue to "needs work" (they found some problems with the project) or "reviewed & tested by the community" (they found no major flaws).

I'm a robot and this is an automated message from Project Applications Scraper.

Initial installation works fine.

Settings page asks for credly username, not aware that I was asked for name so used registered email address (this lets me into credly.com) also tried id from credly user url, unable to authenticate:

Unable to authenticate with Credly. Please check settings against Credly issued information and try again.

I have api key from https://developers.credly.com/my-apps, and secret key, checked no trailing space in form etc

Happy to trial module if I can get over this hurdle.

Hello,
Thanks for the contribution. Here is my review :

in credly_settings_validate and credly_badge_add_form_submit you have a drupal_set_message with error status and a form_set error, the last one is suffisant i think, no need for the extra drupal_set_message.

This module uses curl library, but this in not a Drupal requirement, so the module should implement its hook_requirements for this and document it in README.txt
You can check simpletest_requirements in simpletest module that does the same as needed here.

in theme_credly_badges there is a weird str_replace. is this some custom stuff ? maybe adding a comment would make it clear.

also the body of theme_credly_badges may be replaced by a simple theme('item_list', $vars); i think instead of hardcoding the li ul tags

Thanks again

hi,

thanks again for your contribution, i noticed all your changes.
we can make theme_credly_badges better with no hard coded html tag.
replacing '<a href="https://credly.com/credit/' . $badge->id . '"> <img src="' . $badge_image . '"/></a>';
as in https://api.drupal.org/comment/49213#comment-49213.

i don't have extra notices. it seems good for me.
Thanks

hi,
i looked again at the codebase and it seems ok.
i tested the module by installing it, i creadted an app and tested with a drupal user that is registered in credly.com
i can access the tab with badges and operations.
it seems good

also putting back the PAReview: review bonus tag as i checked those are good too.

Thanks

Review of the 7.x-1.x branch:

• 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:

1. credly_settings_form(): doc block is wrong, this is not a hook. See https://drupal.org/node/1354#forms . Same for credly_settings_validate() and possibly others
2. '#prefix' => "<p>If you are viewing this page, your settings have been validated ...: all user facing text must run through t() for translation.
3. credly_rest(): all check_plain() calls in this function are wrong, your are not printing those settings to HTML? Make sure to read https://drupal.org/node/28984 again.
4. credly_authenticate(): the filter_xss_admin() is wrong here because the "%" placeholder will already run check_plain() for you.
5. credly_badges_page(): same here: the filter_xss() is wrong here since t() with the appropriate placeholders will already sanitize the variables for you.
6. theme_credly_badges(): $badge->image seems to be a URL, so you should use check_url() instead of check_plain().

So although your sanitization function usage is a bit worrisome I don't see exploitable security issues, so I think we can proceed. But please do fix those.

Thanks for your contribution, majorrobot!

I updated your account so you can 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 stay 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.