Check JS sandbox project - https://www.drupal.org/sandbox/deepchandra84/2742711
GIT URL: http://git.drupal.org/sandbox/deepchandra84/2742711.git

Git clone command:

git clone --branch 7.x-1.x https://git.drupal.org/sandbox/deepchandra84/2742711.git check_js
cd check_js

Link to PAreview
http://pareview.sh/pareview/httpgitdrupalorgsandboxdeepchandra842742711git

All sites uses javascript a lot and lot's of feature depends on it but if browser doesn't support javascript or javascript is disabled in browser, then this module allows to display a user friendly disclaimer message to end user to enable the javascript in their browser. Disclaimer message is configurable in admin settings.

CommentFileSizeAuthor
#5 javascript_disabled.png25.48 KBdeepchandra84

Comments

deepchandra84 created an issue. See original summary.

deepchandra84’s picture

deepchandra84 commented
Issue summary: View changes
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/httpgitdrupalorgsandboxdeepchandra842742711git

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.

ashwinsh’s picture

ashwinsh
He/Him
Primary language English
Location Pune
commented

Manual Review

1. Basic application checks
   1.1 Ensure your application contains a repository and project page link.
   [No: Git repository url is missing.]
   1.2 Ensure your project is not duplication.
   [Yes: Does not cause] module duplication and/or fragmentation.
2. Basic repository checks
   2.1 Ensure the repository actually contains code.
   [Yes]
3. Security Review
   3.1 Ensure the project does not contain any security issues.
   [Yes: Meets] the security requirements. / No: List of security issues identified.
4. Licensing checks
   4.1 Ensure the repository does not contain a LICENSE.txt file.
   [Yes: Follows] the licensing requirements.
   4.2 Ensure the repository does not contain any 3rd party (non-GPL) code.
   [Yes: Follows] the guidelines for 3rd party assets/code.
5. Documentation checks
   5.1 Ensure the project page contains detailed information.
   [No: Please provide more information regarding your project.]
   5.2 Ensure the repository contains a detailed README.txt.
   [Yes: Follows] the guidelines for in-project documentation and/or the README Template.
   5.3 Ensure the code contains a well-balanced amount of inline-comments.
   [Yes].
6. Coding standards and style
   6.1 Run an automated review and ensure there are no major issues.
   [Yes: Automated review is passed..]
deepchandra84’s picture

deepchandra84 commented
Issue summary: View changes
Status: Needs work » Needs review
StatusFileSize
new javascript_disabled.png25.48 KB
deepchandra84’s picture

deepchandra84 commented

Updated sandbox project page with GIT repository and other details.

Project Page
Check JS sandbox project - https://www.drupal.org/sandbox/deepchandra84/2742711

Git Repository
http://git.drupal.org/sandbox/deepchandra84/2742711.git

deepchandra84’s picture

deepchandra84 commented
Issue summary: View changes
gisle’s picture

gisle
he/him
Primary language Norwegian Bokmål
Location Norway
commented
Issue summary: View changes

Added Git clone command and link to PAreview to summary.

naveenvalecha’s picture

naveenvalecha
He/Him
commented

@ashwin.shaharkar: look like you forgot to change the status. Is this now RTBC after your review or are there application blockers left and this should be "needs work"?

naveenvalecha’s picture

naveenvalecha
He/Him
commented
Issue tags: +PAreview: single application approval
ashwinsh’s picture

ashwinsh
He/Him
Primary language English
Location Pune
commented
Status: Needs review » Needs work
naveenvalecha’s picture

naveenvalecha
He/Him
commented

@ashwin.shaharkar,
what are the blockers above , the review contains only preview template.

ashwinsh’s picture

ashwinsh
He/Him
Primary language English
Location Pune
commented
Status: Needs work » Needs review

Hi @naveenvalecha,

I edited my comment, please ignore previous one. I see that now 'Git repository url and information regarding project' is added.

Thank you,

scott_euser’s picture

scott_euser commented

Hi Deepchandra84,

Thanks for your contribution.

Generally the module looks good and is simple and efficient.

A few notes:

  • Can you instead add the markup to $page_top in the html.tpl.php rather than in the <head></head>. As the site administrator can potentially add html elements, it could result in invalid html markup pre-html5.
  • You have a small typo in the hook_help(), should be "display" instead of "dispaly".
  • "All sites uses javascript" is a bold statement. Many sites use javascript, yes.
  • As far as I can see, the best practice is to keep permission machine names all lowercase, so "administer js check" instead of "administer JS Check".

What would be great to see (but not a requirement):

  • If the message could only be shown if one or more javascript files are loaded on the page. Ie, if you have a site which uses javascript for some functionalities only, perhaps it is more appropriate to only show the message on those pages. Perhaps you can determine if any javascript is on the page with hook_js_alter if it fires early enough.

Hope that helps!
Scott

scott_euser’s picture

scott_euser commented
Status: Needs review » Needs work
deepchandra84’s picture

deepchandra84 commented

Thanks @scott_euser for your review comments. It is really helpful to make it better.

I will make the changes and will commit the updated code.

deepchandra84’s picture

deepchandra84 commented
Status: Needs work » Needs review

Code has been updated as per comments from scott_euser.
Changing status to Needs Review.

Thanks,
Deep

scott_euser’s picture

scott_euser commented
Status: Needs review » Needs work

Thanks for that update. Could you please respond to the other 3 points if you don't intend to change.

Additionally, after further review, could you please use check_plain() on plain text output or filter_xss() for the html output. I realise the content is intended to be added by admins but since you register a permission it is possible that a not as trusted user could be granted permission to edit the check_js_message. Better to be safe to avoid any potential for XSS.

Thanks!
Scott

deepchandra84’s picture

deepchandra84 commented

Hi scott_euser,

In my last commit, I have already updated the code with below 4 points:

  • Can you instead add the markup to $page_top in the html.tpl.php rather than in the <head></head>. As the site administrator can potentially add html elements, it could result in invalid html markup pre-html5. - DONE
  • You have a small typo in the hook_help(), should be "display" instead of "dispaly". - DONE
  • "All sites uses javascript" is a bold statement. Many sites use javascript, yes. - DONE
  • As far as I can see, the best practice is to keep permission machine names all lowercase, so "administer js check" instead of "administer JS Check". - DONE

Thanks

deepchandra84’s picture

deepchandra84 commented
Status: Needs work » Needs review

Hello scott_euser,
I have added check_plain as suggested by you and committed the code.
Changing the status to needs review.

Thanks

scott_euser’s picture

scott_euser commented
Status: Needs review » Needs work

Hi Deepchandra84,

Are you sure you've pushed those commits? I see the page_top and the check_plain now but can still find 'dispaly' and 'All sites uses javascript' (although I wouldn't consider those critical of course).

For check_plain, that works when the text is plain text. You need to use filter_xss if the editor selects a format other than plain text or any html markup would be escaped. If you can solve at least this one, I'm happy to move this into the final review steps.

Thanks,
Scott

deepchandra84’s picture

deepchandra84 commented
Status: Needs work » Needs review

Hello Scott_euser,

Yes, sorry, it's my bad. 'dispaly' and 'All sites uses javascript' code changes were missed in my last commit.
Now I have committed those changes along with filter_xss change.
Changing the status to needs review.

Thanks,
Deep

scott_euser’s picture

scott_euser commented

Hi Deepchandra84,

Thanks for that. Looks pretty good. I see you're wanting to match the html tags based on the filter the visitor chooses / has access to. As the allowed html tags can be changed, hardcoding the list of allowed tags wouldn't work in all cases. Additionally, admins could create new text formats not in your list which would end up rendering as plain_text in your current setup.

Instead of your switch you can just do this:
$check_js_formatted_text = check_markup($check_js_content, $check_js_format);
You can read more about that here (including a security warning) or view the full list of sanitising functions here.

Thanks,
Scott

scott_euser’s picture

scott_euser commented
Status: Needs review » Needs work
deepchandra84’s picture

deepchandra84 commented
Status: Needs work » Needs review

Hello Scott,

Yes, I agree. Using check_markup is even better. I have updated the code with check_markup in place of switch case statement.

Thanks
Deep

scott_euser’s picture

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

Great, looks good to me! Changing to reviewed and tested by the community, ready for final approval.

deepchandra84’s picture

deepchandra84 commented

Thanks a lot scott_euser for your review. It was really helpful and great learning :)

scott_euser’s picture

scott_euser commented

Happy to help, hope I was too nitpicky and annoying :)

mlncn’s picture

mlncn commented
Status: Reviewed & tested by the community » Fixed

Thanks for your contribution, deepchandra84! You are now a vetted Git user. You can promote this to a full project.

When you create new projects (typically as a sandbox to start) you can then promote them to 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— in particular my long delay in approving after all reviews passed. We know it's broken and are trying to fix it.

Status: Fixed » Closed (fixed)

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