Hi,
pageguide.js is an interactive visual guide to elements on web pages.
Instead of cluttering your interface with static help message, or explanatory text,
add a pageguide and let your users learn about new features and functions.
I have developed a module for that page guide with the proper GUI, So drupal users simply add page guide tour content and path with the GUI.
Project git : git clone --branch 7.x-1.x http://git.drupal.org/sandbox/pathirakaliappan/2301783.git page_guide
cd page_guide
Project page : https://www.drupal.org/sandbox/pathirakaliappan/2301783
Manual reviews of other projects:
https://www.drupal.org/node/2321063#comment-9197889
https://www.drupal.org/node/2336387#comment-9197843
https://www.drupal.org/node/2448867#comment-9705967
Comments
Comment #1
PA robot CreditAttribution: PA robot commentedThere are some errors reported by automated review tools, did you already check them? See http://pareview.sh/pareview/httpgitdrupalorgsandboxpathirakaliappan23017...
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.
Comment #2
leopathu CreditAttribution: leopathu commentedHi,
I reviewed the module at http://pareview.sh/pareview/httpgitdrupalorgsandboxpathirakaliappan23017... and also have fixed all the errors and warning, but I have used the compress version of pageguide.min.css so there is showing more errors at pageguide.min.css. And also showing 2 errors at my pageguide.js file. It can't fixed because i used << symbol in this file.
Comment #3
leopathu CreditAttribution: leopathu commentedComment #4
leopathu CreditAttribution: leopathu commentedComment #5
er.pushpinderrana CreditAttribution: er.pushpinderrana commentedRemoving bonus tag, you have not done manual reviews of other project applications.
Comment #6
dbt102 CreditAttribution: dbt102 commentedHi leopathu,
I've conducted a manual review of your code following the review template format and have listed my comments below. Hope this helps ...
1.1 The git clone command you have posted in your application issue summary is not correct. It should be git clone --branch 7.x-1.x http://git.drupal.org/sandbox/pathirakaliappan/2301783.git page_guide
cd page_guide
see https://www.drupal.org/project/2301783/git-instructions
1.2 No one else has contributed a project integrating pageguide.js functionality with Drupal yet, so this is good.
1.3 I see you have two sandbox projects going ( pageguide and newsfeeds ) but looking at “Issues for Drupal.org Project applications” you have only one application. This is correct.
2.1 The repository DOES contain code.
2.2 Your branch IS up-to-date with ‘origin/7.x-1.x'. Meaning you are working with a version specific branch
3.1 Bad line endings were found, always use unix style terminators. See https://www.drupal.org/coding-standards#indenting
It looks like you need to turn on “Line Endings / Unix “ in your text editor. I would guess that you submitted the code from an editor with windows line endings enabled. Submitting the code with Unix line endings will cleanup much of the pareview errors/warnings. This may be masking preview security review.
4.1 Code repository does not contain a LICENSE.txt file, however the pageguide.js does say it is MIT license. Need to implement Libraries dependancy and move the file into sites/all/libraries directory. (see 4.2)
4.2 The repository DOES contain 3rd party code.
Third party code is not generally allowed on Drupal.org and should be deleted. This policy is described in the getting involved handbook. It also appears in the terms and conditions you agreed to when you signed up for Git access, which you may want to re-read, to be sure you're not violating other terms.
The Libraries API module is a recommended method for adding 3rd party dependencies without directly including the code on Drupal.org.
5.1 The project page does not contain enough useful information. Will need to update it with dependencies (Libraries), download the page guide js libraries. How to setup, etc.
Project pages should be helpful; there are literally thousands of modules, themes and installation profiles and site builders need a clear way to understand what a project does. Please have a look at the tips for a great project page, you may also use HTML-tags for better structure.
5.2 The README.txt file is not very helpful.
This file should contain a basic overview of what the module does and how someone may use it. Please make sure your README.txt follows the guidelines for in-project documentation and is based upon the README.txt Template.
The contents of the file may be a repeat of the synopsis on the project page.
5.3 The code is minimally commented, but clean and easy to follow.
6.1 Not using unix line endings means your code is not compliant with Drupal Coding Standards. (see also comment under 3.1)
As coding standards make sure projects are coded in a consistent style we please you to have a try to follow them whenever possible.
In addition to validating that your module is aligned with Drupal's coding standards, there are several available tools which can automatically detect and flag a number of common security issues which would otherwise delay approval of your application.
Commonly used tools are Coder (and Coder Sniffer) which are bundled by the PAReview.sh script and its online version at pareview.sh/.
Note that issues found are possibly false positives and fixing all issues is not a requirement for getting through the application process. Automated reviews may point you to possible security issues - what does not mean they are really security issues - note that it's a common case that automated reviews can have false positives.
7.1 You should include a configure button on modules page that takes you to admin/config/user-interface/page-guide, where you setup page guide.
You should add a help page for your module on admin/help
More suggestions and hints can be found at what mistakes are commonly done.
Comment #7
dbt102 CreditAttribution: dbt102 commentedbased on the issues identified in the manual review https://www.drupal.org/node/2338257#comment-9155971 changing the status to "needs work"
Comment #8
leopathu CreditAttribution: leopathu commentedHi dbt102,
Thanks for reviewed this module, I read your instruction carefully and it would be very useful to me.
Now I have fixed the issue one by one what mentioned at https://www.drupal.org/node/2338257#comment-9155971
And also reviewed my code in http://pareview.sh/pareview/httpgitdrupalorgsandboxpathirakaliappan23017...
There is no any errors and warning for this module.
Comment #9
saniyat CreditAttribution: saniyat commentedHi leopathu,
I have check your module thoroughly and got some minor and major problem:
General Bugs/Review Report:
Security Bugs(Major):
Comment #10
saniyat CreditAttribution: saniyat commentedComment #11
leopathu CreditAttribution: leopathu commentedHi saniyat,
Thanks for review,
I have fixed all the minor and major problems (what saniyat mentioned at #9) in my module.
And also fixed the cross side scripting (XSS) issue.
Comment #12
leopathu CreditAttribution: leopathu commentedComment #13
gaurav.pahuja CreditAttribution: gaurav.pahuja commentedAutomated Review
Best practice issues identified by pareview.sh / drupalcs / coder. Please don't copy/paste all of the results unless they are short. If there are a lot, then post a link to the automated review and mention that problems should be addressed.
Manual Review
The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.
Comment #14
leopathu CreditAttribution: leopathu commentedHi gaurav.pahuja,
Thanks for your valuable review. Now I have fixed the module installation via drush issue.
This is the latest automated reviewed http://pareview.sh/pareview/httpgitdrupalorgsandboxpathirakaliappan23017... for the latest code.
And also I have fixed the cross side scripting issue so i removed security issue tags.
Comment #15
leopathu CreditAttribution: leopathu commentedComment #16
Sabareesh Prasad CreditAttribution: Sabareesh Prasad commentedChecked this module and this works fine. Also the above mentioned issues are fixed and the coding standard looks good too. This feature doesnt' exist in Drupal so far, so worth attempting it.
Comment #17
leopathu CreditAttribution: leopathu commentedComment #18
er.pushpinderrana CreditAttribution: er.pushpinderrana commentedPlease don't RTBC your own issues, see the workflow: https://www.drupal.org/node/532400.
Getting review bonus would help speed up the process and make sure it gets on the review admins radar.
Please treat these rules with understanding and be patient.
Comment #19
leopathu CreditAttribution: leopathu commentedComment #20
mpdonadioCan you outline how this is different from Joyride JQuery for Drupal Site Tours, which is essentially what the Drupal 8 Tour Module is based on? Set back to Needs Review when you have done this. Thanks.
Comment #21
leopathu CreditAttribution: leopathu commentedHi mpdonadio,
The page guide module visually difference than the Joyride module. It will be best visual experience for the page guide.
You could compare the both module features here http://zurb.com/playground/jquery-joyride-feature-tour-plugin
http://tracelytics.github.io/pageguide/
Comment #22
mpdonadioOK, similar functionality, but since this is a different JS library, users may have preference for one over the other. Assigning to myself for next review.
Comment #23
mpdonadioAutomated Review
Review of the 7.x-1.x branch (commit 5689d06):
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
The module is currently filtering on input (which is wrong), but the module is throwing JS errors for me, so I can't adequately test this.
In page_guide_requirements() you already have a hard dependency on libraries; your check is unnecessary.
You may want an explicit 'view page guide permission' in the future.
(+) page_guide_list() needs a proper docblock w/ a @return and a mention that it is the page callback. It should also return a proper render array and not themed output directly.
page_guide_step_list(), ditto.
(+) A lot of your functions need proper docblocks. See https://www.drupal.org/node/1354
I think you can set the destination directly with confirm_form() w/o messing with $_GET[].
(+) In general, using #attached is preferred over drupal_add_css, drupal_add_js, and drupal_add_libray. You should also add your CSS here, instead of via the .info.
(+) Your behavior needs to use the content and settings that get passed in. I would also move this to a separate file instead of doing it inline.
(+) page_guide_page_alter should be using drupal_attributes() instead of string concatenation for the attributes.
(+) page_guide_page_alter() has an untranslated string. Double check the module.
(*) Your parameter usage in your forms in the .admin.inc is wrong. arg() is evil, and should almost always be avoided. You can both use a form as a page callback, and pass it arguments. See https://api.drupal.org/api/drupal/modules%21system%21system.api.php/func...
(*) In page_guide_create_validate(), why the filter_xss(); this isn't going to output.
(*) The filter_xss() in page_guide_create_submit() are wrong. Filter on output, not input. See https://www.drupal.org/node/28984 and https://www.drupal.org/node/263002
You should see if you can figure out a way to validate the URL on entry form.
After you save the initial page guide, you should be redirected to the create page for it.
(*) The module is throwing JS errors for me, so I can't adequately test for XSS.
The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.
The plaining usage and arg() are pretty big API issues and are worth to go back to Needs Work.
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.
Comment #24
leopathu CreditAttribution: leopathu commentedHi mpdonadio,
Thanks for your valuable reviews,
I have fixed all the (+) and (*) issues, what you mentioned at #23.
Comment #25
er.pushpinderrana CreditAttribution: er.pushpinderrana commentedAssigning to myself for next review, which will hopefully be tonight.
Comment #26
er.pushpinderrana CreditAttribution: er.pushpinderrana commentedAutomated Review
Review of the 7.x-1.x branch (commit 93d7207):
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
permenently
.arg()
function still exists that looks weird.filter_xss()
but the documentation forfilter_xss()
states:Filters an HTML string to prevent cross-site scripting (XSS) vulnerabilities. It means if you use filter_xss(), then the string passed to the function is supposed to be HTML. However, wrapping your HTML with
filter_xss()
in PHP calls isn't great style; it's likely to upset the designer and doesn't ensure that the CSS class is valid, use check_plain() instead. SeeSELECT *
, is it really required in every case, recheck again. Because best practices says only fetch required fields otherwise it just increase extra overhead if using*
.Still this issue exists that need to be fix before making stable release.
page_guide_init.js
missing in your module folder that preventing me to test your module further because of 404 js error that stop further execution of js.Agreed with @mpdonadio.
Same with me because of this missing file.
But still I tested this module further by changing in code and found due to this css
was unable to see actual page guide output.
The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.
Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.
Comment #27
leopathu CreditAttribution: leopathu commentedHi,
I have fixed all the issues, what @er.pushpinderrana said that in his comments #26.
I forgot to add the js file in the git. so that the missing files error occured. Now I have added that also.
Comment #28
Sabareesh Prasad CreditAttribution: Sabareesh Prasad commentedThis looks good.
Comment #29
ultimateboy CreditAttribution: ultimateboy commentedI'm not convinced this module is correctly following licensing restrictions of d.o.
The module contains .css directly from the pageguide github repo: https://github.com/tracelytics/pageguide
This github repo contains a LICENSE.txt file states that "The above copyright notice and this permission notice shall be included in all copies or substantial portions of the Software." However this is not the case.
I'd recommend changing the installation instructions to instruct the user to download the js and css from the pageguide library and not include these in your project at all. If you need to provide additional css to make this library work (like you provide a few extra lines of js code) then that's fine. But including a large amount of css from this external library seems like an issue to me.
Comment #30
leopathu CreditAttribution: leopathu commentedHi,
I have removed the css from the module and added that in the library folder. and also updated in the README.txt file about the library informations.
Comment #31
naveenvalechaAssigning to myself for next review that might be in tonight.
Comment #32
naveenvalechaThanks for your contribution.You are near RTBC
Manual Review : Read 4dd0e...
t("pageguide.js is an interactive visual guide to elements on web pages. Instead of cluttering your interface with static help message, or explanatory text,add a pageguide and let your users learn about new features and functions.")
use single quotes where possible for Drupal code standards and a very slight performance benefit.'#empty' => t('Your table is empty'),
Write some more user friendly text something like that "Currently we have no contents in page guides".'op' => l(t('create'), 'admin/config/user-interface/' . $list->pg_id . '/page-guide-step') . ' | ' . l(t('edit'), 'admin/config/user-interface/page-guide/' . $list->pg_id . '/edit') . ' | ' . l(t('delete'), 'admin/config/user-interface/page-guide/' . $list->pg_id . '/delete'),
Divide this code in multiple lines.page_guide_step_list
: Apply the above 2 suggestions for this one as well.page_guide_step_get_list
this will also clean the code in function page_guide_page_alter
t('Are you sure you want to delete this step?')
The message should be more specific and liket('Are you sure you want to delete this page guide step?')
The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.
I would recommend you, please help to review other project applications to get a review bonus back. This will put you on the high priority list, then git administrators will take a look at your project right away :)
Comment #33
leopathu CreditAttribution: leopathu commentedComment #34
leopathu CreditAttribution: leopathu commentedHi,
@naveenvalecha, Thanks for your valuable reviews. it helped to improve the module. I have fixed all the issues what you mentioned on #32. And also reviewed another 3 new modules, so add the review ponus tags.
Comment #35
naveenvalechaAssigning to myself give it second shot that may be tonight.
Comment #36
naveenvalechaManual Review : Read 432accd..
admin/config/user-interface/page-guide
andadmin/config/user-interface/%/page-guide-step
Looks you can easily move to page_guide.admin.inc.It will give some performance hits as well.Similarly of the other functions that are the usable at the admin part.Assigning to @klausi to give it a final look if he has time.
Comment #37
klausiRemoving review links there were not manual reviews (git clone commands can be fixed by anybody).
Comment #38
klausimanual review:
But otherwise looks good to me, so ...
Thanks for your contribution, leopathu!
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.