[D7] SuperSaaS

Git clone command for the sandbox is missing in the issue summary, please add it.

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.

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

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

HI,
Please provide proper Git clone command:
Please follow instructions:

You can find the correct git clone command for your sandbox by clicking on the Version control tab, removing the checkbox in front of "Maintainer", and clicking Show. You can then copy-paste the git clone command from the codeblock below "Setting up repository for the first time".

Anyone can fix the git clone command in the issue summary, please do that. That alone should not block this application.

Reviewed by Coder module

Problem:
Line 61: Use the Form API to build forms to help prevent against CSRF attacks. (Drupal Docs) [security_18]
$out = '<form method="post" action="http://' . ($domain ? $domain : t('www.supersaas.com')) . '/api/users">';

Solution: Described in the patch file.

fix for the git clone command.

Hi,

*. It is not a right way to include a raw form in the module file. The raw form include in the supersass.module,

function supersaas_block_view($delta = '') { 
 ...
 $out = '<form method="post" action="http://' . ($domain ? $domain : t('www.supersaas.com')) . '/api/users">';
 ...

can be refactored by moving the form into a hook_form() or custom form builder function implementation. Also you can return the form in the hook_block_view() by using drupal_get_form(). For more info - https://www.drupal.org/node/1394018

*. The raw java script include in the module file doesn't seems to be right.

$out .= '</form><script type="text/javascript">function confirmBooking() {';
$out .= "var reservedWords = ['test','supervise','supervisor','superuser','user','admin','supersaas'];";
$out .= "for (i = 0; i < reservedWords.length; i++) {if (reservedWords[i] === '{$user->name}') {return confirm('";
$out .= t('Your username is a supersaas reserved word. You might not be able to login. Do you want to continue?') . "');}}}</script>";

You can either use drupal_add_js() or if you want to include the JS to a form then you can make use of Form API #attached property. For more info - https://www.drupal.org/node/304255

Thanks,

Hi,

*. Please remove one of the README file. It contains both the README.txt and README.md file. It would be best that if you can format the file based on the README template - https://www.drupal.org/node/2181737 and also provide an additional info like "After enable the module goto blocks configuration page to enable the 'SuperSaaS Login' block" in the README file.

*. Please remove the error_log() statements in supersaas_help() and supersaas_permission().

*. I guess it is a good idea to make use of Drupal Form API wherever it is possible. Even though you are passing the data to a 3rd party site, it is best recommended to use Drupal Form API to construct the form and change either the $form['#action'] or include the $form['#redirect'] property to submit the data to the 3rd party url. Also it is recommended to make use of $form['#attached']['js'] to include any JS to the form.
On the other hand if you dont want to pass Drupal form-token, form-build-id then why don't you build a drupal_http_request() to post the data to SuperSaas service instead of using the raw form or Form API to make it follow the Drupal standards?.

Thanks,

Reviewed by coder module, but found few drupal coding standard errors, find the attached document.

And also check below link
http://pareview.sh/pareview/httpgitdrupalorgsandboxsupersaas2508934git

I've looked at the latest commit, which us for Drupal 8, and this is just a single point in the Project Application Review Template, but I suggest you address it while waiting for a more complete review:

Licensing

No.
- Please remove the COPYRIGHT.md file.
Drupal will add the appropriate license file automatically during packaging so your repository should not include it. See https://www.drupal.org/node/1587704#licensingchecks
- You must also remove CODE_OF_CONDUCT.md.
Drupal already has a code of conduct: https://www.drupal.org/dcoc. If you find it lacking, you need to go through normal community channels to get it amended. Your CODE_OF_CONDUCT.md comes acoss as misleading, as it describes a number of rights that project maintainers are supposed to have, and things that project maintainers are supposed to do. Neither is applicable here.
- I would also recommend that you remove CONTRIBUTING.md. There exists an established workflow for contribution to a project hosted at Drupal.org. Establishing an alternative contribution workflow for your project does not look to me as a good idea.

I hope somebody else gets around to look at the other review points soon - I've noticed you've waited 10 months for a manual review.

However, you may make things more complicated for reviewers than necessary by pushing two different branches (one for D7 and one for D8) into the repo at the same time. There's a lot of projects to review, and having to deal with two branches doubles the work for the reviewer.

In regards to multi-branches, actually I was not aware that my D8 branch will trigger an update for this issue. Do you suggest deleting D8 branch temporarily for now?

If you want this application resolved (I am not going to say "quickly", because that would raise false hopes), my advice would be that you delete the D8 branch. You can add it back after your project has been approved.

Or creating another issue specially for D8?

Don't do that! That will just trigger the "duplicate detector" sensor of our dear robot, and one of your issues (choosen at random) will immediately be closed.

I've tried to test your project, but I'm unable to configure it correctly. I've set it up as described in the README-file, and I've created an admin account at SupeSaaS website, and set it up as described here: https://www.supersaas.com/info/doc/integration/drupal_integration - including "Troubleshooting tips" at the bottom.

It works as expected when I test it inside the SuperSaaS dashboard (using the built-in test feature).

I am able to get the button to show up on the Drupal site, but pressing the button produces.

<errors>
  <error>Email is not a valid email address</error>
</errors>

Troubleshooting tips say:

If you see an error Email is not a valid email address, then please read the previous paragraph again to confirm you have made the correct settings in your SuperSaaS account

The previous paragraph says: "Note that the button only appears to users who are logged in to your Drupal site".

Since the button appears, and I am logged in, this is not really much help.

The part that says "confirm you have made the correct settings in your SuperSaaS account" isn't much help either. I already know that my settings are not correct (since it does not work). I even think that it is very likely that it is some email setting that is wrong.

However, checking the settings for my user at Drupal, and at SuperSaaS, both are set up with working email addresses, so it is not very obvious where I have to look to find the incorrect setting.

I've removed some random keywords you've added to the tag field.

Before adding tags read the issue tag guidelines. Do NOT use the tag field for adding random keywords.

There are two branches in the repo. There is some confusion about what branch the applicant want reviewed (see comments under "Master branch" below). This review is of the 7.x-1.x branch.

Automated Review

PAReview (ESLint) complains about an unused JavaScript variable that should be removed. There are no automated test cases. Adding test cases are strongly recommended, but they are not mandatory.

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Yes: Does not cause module duplication and/or fragmentation.

Master Branch

No: Does not follow the guidelines for master branch.
You've set the default branch to 8.x.2.x, but the [D7] in the title of your application and the git clone command provided indicate that you're submitting the 7.x-1.x branch for review. You need to make up your mind about what you want reviewed (and adjust either the default branch or the title accordingly).

Licensing

Yes: Follows the licensing requirements.

3rd party assets/code

Yes: Follows the guidelines for 3rd party assets/code.

Project page

No. Please take a moment to make your project page follow the Project page template and it may be a good idea to also read tips for a great project page.
In particular, you should add the section "Dependencies". This module depends on SaaS delivered by SuperSaaS. This dependency, and pricing must be disclosed on the project page.

README.txt/README.md

Yes: Follows the guidelines for in-project documentation and/or the README Template. It should be noted that "Troubleshooting Tips" didn't help me when I had trouble - but bad UX is not a blocker in the review queue (but may lose you some business).

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

Yes: Meets the security requirements. (But see comment about MD5 below.)

Coding style & Drupal API usage

1. You use md5() to compute the checksum sent to the SaaS. I understand that the choice of hash function may not be yours to make, but read this: https://www.drupal.org/writing-secure-code/hash-functions and share with whoever decides what hash function to use on SuperSaaS.
2. Do not include .gitignore in the repo you push (make it ignore itself).

The starred items (*) above are big issues and warrant the application going back to Needs Work. Items marked with a plus sign (+) should be addressed before a stable project release, but does not block the project from being promoted to a full project. The rest of the comments are only recommendations.

Conclusion

There are IMHO no more blockers. Moving to RTBC. Note that promotion will not happen until a git administrator has given this a second set of eyeballs.

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.

Apologies for the unconscionable four month delay. You are now a "vetted git user" and can promote projects to full status.