Description

This module wraps the open source JustGiving SDK, allowing it to be easily used by Drupal 7 modules. It then provides a sub module which creates a block showing the top 10 fundraisers for the charity with links to their donation page. On the roadmap is open sourcing more of the project (http://toughnbuff.childrenssociety.org.uk) , including profile integration, enabling a full JustGiving campaign to be run with it.

pareview : http://pareview.sh/pareview/httpgitdrupalorgsandboxtimmarsh2542942git

Git clone command
git clone --branch 7.x-1.x http://git.drupal.org/sandbox/tim_marsh/2542942.git justgiving

Project Page
https://www.drupal.org/sandbox/tim_marsh/2542942

Differences from similar projects
This module is geared towards a community taking on a campaign - much like toughnbuff, rather than a single user showing their data, or soliciting donations (which are covered by 2 sandbox modules)

https://www.drupal.org/sandbox/tr33m4n/1760988 - focussed on a single users donations, rather than at the campaign level
https://www.drupal.org/sandbox/KrisPomphrey/2202035 - JustGiving for webforms - doesnt integrate with the user, and is a more manual integration

Manual Reviews of Other Projects

CommentFileSizeAuthor
#12 d7review.png151.69 KBrashid_786

Comments

tim_marsh created an issue. See original summary.

tim_marsh’s picture

tim_marsh commented
Title: D7 Justgiving » [D7] Justgiving
gaja_daran’s picture

gaja_daran commented

Hi Tim,

Manual Review:

I am unable to enable justgiving_charity_leaderboard module. Getting following error.
Fatal error: Call to undefined function __justgiving_client() in /var/www/drupal/sites/all/modules/custom/2542942/modules/justgiving_charity_leaderboard.module on line 84
If justgiving_charity_leaderboard module required justgiving module then set as dependency module.

If I enable both module then I get following error.
Fatal error: Class 'JustGivingClient' not found in /var/www/drupal/sites/all/modules/custom/2542942/justgiving.module on line 156

Kindly fix it.
Thanks.

gaja_daran’s picture

gaja_daran commented
Status: Needs review » Needs work
tim_marsh’s picture

tim_marsh commented

Ah - ok , thankyou .
The first one is me not getting the dependencies right - I'll fix that now.

The fatal error is the library not being found, do you think I need to add more information about downloading the 3rd party library in the Read-me ?

tim_marsh’s picture

tim_marsh commented

Thankyou again for the feedback
to remedy the errors I have

  1. Corrected the module dependencies, the leaderboard module depends on the main module
  2. Added a hook_requirements to check for the 3rd party module being in place, before enabling the main module
tim_marsh’s picture

tim_marsh commented
Status: Needs work » Needs review

Thankyou again for the feedback
to remedy the errors I have

  1. Corrected the module dependencies, the leaderboard module depends on the main module
  2. Added a hook_requirements to check for the 3rd party module being in place, before enabling the main module
PA robot’s picture

PA robot commented
Status: Needs review » Needs work

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

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.

yonko.tsvetkov’s picture

yonko.tsvetkov commented

Hello,

I tested Justgiving module on my drupal test site.

1. Git clone created module directory with name "2542942".
I enabled module without errors but module directory name has to be changed to justgiving.
Edit:

Git clone command
git clone --branch 7.x-1.x http://git.drupal.org/sandbox/tim_marsh/2542942.git

to 

Git clone command
git clone --branch 7.x-1.x http://git.drupal.org/sandbox/tim_marsh/2542942.git justgiving
cd justgiving

2. Pareview review show some errors:

/justgiving.module: all functions should be prefixed with your module/theme name to avoid name clashes

function __justgiving_client() {
function __justgiving_client_for_user($username, $password) {

-------------------

FILE: /var/www/drupal-7-pareview/pareview_temp/README.md
----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
18 | WARNING | Line exceeds 80 characters; contains 84 characters
----------------------------------------------------------------------

FILE: /var/www/drupal-7-pareview/pareview_temp/justgiving.install
---------------------------------------------------------------------------
FOUND 2 ERRORS AND 4 WARNINGS AFFECTING 5 LINES
---------------------------------------------------------------------------
9 | WARNING | [ ] Format should be "* Implements hook_foo().", "*
| | Implements hook_foo_BAR_ID_bar() for xyz_bar().",, "*
| | Implements hook_foo_BAR_ID_bar() for
| | xyz-bar.html.twig.", or "* Implements
| | hook_foo_BAR_ID_bar() for xyz-bar.tpl.php.".
9 | ERROR | [x] Doc comment short description must end with a full
| | stop
13 | WARNING | [ ] Line exceeds 80 characters; contains 84 characters
20 | WARNING | [ ] Line exceeds 80 characters; contains 112 characters
21 | WARNING | [ ] Line exceeds 80 characters; contains 129 characters
30 | ERROR | [ ] The $text argument to l() should be enclosed within
| | t() so that it is translatable

3. Would be good to add pareview review link( http://pareview.sh/pareview/httpgitdrupalorgsandboxtimmarsh2542942git ) in the issue summary.

tim_marsh’s picture

tim_marsh commented

Hi Thanks for that, @yonko.tsvetkov , this is the first module Ive submitted so keen to get it completely right,
Ive fixed the pareview errors for the install file.

I'll get the function names changed tonight- its an old habit using __modulename to denote 'private' methods - those not meant to be called outside the module

tim_marsh’s picture

tim_marsh commented
Status: Needs work » Needs review

current pareview status: http://pareview.sh/pareview/httpgitdrupalorgsandboxtimmarsh2542942git

actions taken:

  1. refactored function names
  2. fixed formatting isssues in the install file.
  3. only warnings remain in the pareview status which should be removed as more functionality added
rashid_786’s picture

rashid_786 commented
Status: Needs review » Needs work
StatusFileSize
new d7review.png151.69 KB

Hi Tim,
Thanks for your contribution,

I tried test the module on my local machine found some issues.
* You should mention git clone command along with directory name on review page on top as @Yonko recommended.

Git clone command
git clone --branch 7.x-1.x http://git.drupal.org/sandbox/tim_marsh/2542942.git justgiving
cd justgiving

* When i downloaded the library and extract it, i found the different structure from the one you mentioned help requirement so i get error and unable to test this module. Pls refer attached file.

tim_marsh’s picture

tim_marsh commented
Issue summary: View changes
tim_marsh’s picture

tim_marsh commented
Status: Needs work » Needs review

Hi @rashid_786
thanks for your review , I've updated the issue.

with regards to the library -
I had this in the readme

https://github.com/JustGiving/JustGiving.Api.Sdk

store this in the libraries folder for your site so hook_libraries_info can find it.

you can do this by doing the following

```
cd sites/all/libraries
git clone https://github.com/JustGiving/JustGiving.Api.Sdk
ln -s JustGiving.Api.Sdk justgiving
```
And should look like this:

```
libraries/justgiving/JustGivingClient.php
libraries/justgiving/ApiClients
```

which explains how to put the library into the right location.

Its now this

```
cd sites/all/libraries
git clone https://github.com/JustGiving/JustGiving.Api.Sdk
ln -s JustGiving.Api.Sdk/PHP/ justgiving
```

or if you are on windows, you'll need to copy the PHP folder to be called justgiving

And should look like this:

```
libraries/justgiving/JustGivingClient.php
libraries/justgiving/ApiClients
```

notice the PHP, that should be correct now

rashid_786’s picture

rashid_786 commented
Issue tags: +PAreview: security

Automated Review

Pareview says: error in README.md.
It would be nice to put the link to your pareview review on the issue summary.

Manual Review

Individual user account
Yes
No duplication
Yes
Master Branch
Yes
Licensing
Yes
3rd party assets/code
Yes
README.txt/README.md
Yes
Code long/complex enough for review
Yes
Secure code
Yes
Coding style & Drupal API usage
Some recommendations:
  1. Enhance your readme.md and make helpful for window user also. As per my suggestions help contents should be common for Linux/Window in case of no major differences and mention the same in hook_requirements()
  2. Please, provide a link to your pareview review on the issue summary.

If added, please don't remove the security tag, we keep that for statistics and to show examples of security problems.

This review uses the Project Application Review Template.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Issue tags: -PAreview: security

@rashid_786: you did not identify any security issue in your review? Please only add the tag if you find an actual security problem.

tim_marsh’s picture

tim_marsh commented
Issue summary: View changes
tim_marsh’s picture

tim_marsh commented

@rashid_786 - Ive

  1. added the link into the issue
  2. in the README.md, fixed the line length warnings

    3. ,

  3. updated the messaging in the install hook and readme

    4. ,

the only thing pareview lists are 2 warning for unused variables. I think that these aid the readability of the code, but can be removed if required.

sheldonkreger’s picture

sheldonkreger commented

I noticed that you set several variables. You need to implement hook_uninstall() and use variable_del() to clear them out when the module is uninstalled.

Quick output from my Grep:

justgiving.module:58:    '#default_value' => variable_get('justgiving_text_number', '70070'),
justgiving.module:68:    '#default_value' => variable_get('justgiving_api_key', NULL),
justgiving.module:78:    '#default_value' => variable_get('justgiving_api_version', 1),
justgiving.module:87:    '#default_value' => variable_get('justgiving_username', ''),
justgiving.module:95:    '#default_value' => variable_get('justgiving_password', ''),
justgiving.module:103:    '#default_value' => variable_get('justgiving_endpoint', "https://api-sandbox.justgiving.com/"),
justgiving.module:116:  return variable_get('justgiving_api_key', NULL);
justgiving.module:123:  return variable_get('justgiving_api_version', 1);
justgiving.module:129:  return variable_get('justgiving_username', NULL);
justgiving.module:135:  return variable_get('justgiving_password', NULL);
justgiving.module:141:  return variable_get('justgiving_endpoint', "https://api-sandbox.justgiving.com/");
modules/justgiving_charity_leaderboard.module:35:    '#default_value' => variable_get('justgiving_charity_leaderboard_charity_id', '166'),
modules/justgiving_charity_leaderboard.module:78:  $charity_id = variable_get('justgiving_charity_leaderboard_charity_id', '166');
modules/justgiving_charity_leaderboard.module:96:    $variables = array(
modules/justgiving_charity_leaderboard.module:102:    $output = theme('item_list', $variables);
sheldonkreger’s picture

sheldonkreger commented
Status: Needs review » Needs work

Other than issue in previous comment, I think this is ready to go. I don't think unused variables throw warnings in PHP and are therefore harmless. Correct me if I"m wrong :-)

tim_marsh’s picture

tim_marsh commented

@sheldonkreger - brilliant review - thankyou . I'll get that done.

tim_marsh’s picture

tim_marsh commented
Status: Needs work » Needs review

Ive updated as per the last review - all variables are now removed when their owning module is uninstalled .

sheldonkreger’s picture

sheldonkreger commented
Status: Needs review » Reviewed & tested by the community

Since this meets all the manual review criteria and the variable deletion issue has been fixed, I'm marking this as RTBC.

opdavies’s picture

opdavies
He/him
Primary language English
Location Wales
commented

+1

tim_marsh’s picture

tim_marsh commented

Is there anything I can do to move this forwards - as its been reviewed and tested for 29 days now ?

opdavies’s picture

opdavies
He/him
Primary language English
Location Wales
commented

See https://groups.drupal.org/node/142454.

chandeepkhosa’s picture

chandeepkhosa commented

+1

tim_marsh’s picture

tim_marsh commented
Issue summary: View changes
damienmckenna’s picture

damienmckenna
Location TN, USA
commented
Status: Reviewed & tested by the community » Fixed

Thanks for your contribution, Tim!

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:

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.

Status: Fixed » Closed (fixed)

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