[#32665] Feature jquery keepalive by Chris-Jones-Gill · Pull Request #2494 · joomla/joomla-cms

Chris-Jones-Gill · 2013-11-11T17:52:05Z

Change the keepalive function to use jQuery instead of MooTools.

Tracker http://joomlacode.org/gf/project/joomla/tracker/?action=TrackerItemEdit&tracker_item_id=32665&start=600

beat · 2013-11-11T18:48:24Z

.gitignore

This file should not be part of the PR imho.

Ooops, it should not have been. How do I remove it?

wilsonge · 2013-11-15T01:50:33Z

You need to fix the unit tests for this :) To do so just remove the JHtml framework part of the array out here: https://github.com/KISS-Web-Design/joomla-cms/blob/feature-jquery-keepalive/tests/unit/suites/libraries/cms/html/JHtmlBehaviorTest.php#L640

If you being really good you might even add to the test to make sure jQuery is present

piotr-cz · 2013-11-15T08:39:17Z

What about moving the keepAlive function to the jQuery().ready so it doesn't leak to global scope?

Also I'd like to see newlines at the beginning and end of script, otherwise rendered page source code becomes unreadable when there are few scripts placed one after another.

// Attach keep alive to document
JFactory::getDocument()->addScriptDeclaration('
    jQuery(document).ready(function() {
        window.setInterval(
            function(){ new jQuery.ajax({ type: "GET" }); },
            ' . $refreshTime . '
        );
    });
');

Chris-Jones-Gill · 2013-11-15T10:08:35Z

@wilsonge

Thanks, didn't notice the failing test (my bad). I will fix it and update this PR.

@piotr-cz I will add the newlines in, but the jQuery().ready is not the recommended syntax - according to the jQuery API.
=== http://api.jquery.com/ready/ ===
All three of the following syntaxes are equivalent:

$( document ).ready( handler )
$().ready( handler ) (this is not recommended)

$( handler )

Chris.

… no longer required.

…h other scripts.

wilsonge · 2013-11-15T13:29:00Z

tests/unit/suites/libraries/cms/html/JHtmlBehaviorTest.php

Nice :) You haven't defined $document so the test is failing and also this is in the switcher test not the keepalive test (although jquery is being included in this function - so you can leave this set of lines if you want :) )

D'oh! Shouldn't try to rush things. Hopefully the latest commit corrects my mistakes. If not I will wait until I've had a good nights sleep before fixing.
Thanks for the feedback.

Both jQuery tests still failing which is odd - beginning to wonder if we'd need to mock it in some way as something very odd is going on there.

I suggest taking a look at how JHtmlJquery tests are set up and copying
that effort should fix your issues

On Friday, November 15, 2013, George Wilson wrote:

In tests/unit/suites/libraries/cms/html/JHtmlBehaviorTest.php:

@@ -233,6 +233,12 @@ public function testSwitcher()

JFactory::$application = $mock;

$this->assertArrayHasKey(

'/media/jui/js/jquery.min.js',

$document->_scripts,

Both jQuery tests still failing which is odd - beginning to wonder if we'd
need to mock it in some way as something very odd is going on there.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/2494/files#r7705190
.

… instance BEFORE referencing it. D'oh! Added the jQuery test to testSwitcher by mistake previously, but as it is a valid test it has been corrected and left in.

…live because they are failing and I don't understand why - as the jQuery test is not part of the original test structure, removing what I added (hoping to improve the test, but failing) is probably the best idea for now. The test for jQuery presence could be added at a later date by someone who understands the test structure and phpUnit better than I.

betweenbrain · 2013-12-11T02:26:36Z

From what I can tell, the code looks good. We just need some test instructions and good tests. @KISS-Web-Design can you provide instructions on testing this?

Chris-Jones-Gill · 2013-12-11T02:34:31Z

No problem, I'll write something up and post it here and on the tracker.

Chris.

On Wednesday, 11 December 2013, Matt Thomas wrote:

From what I can tell, the code looks good. We just need some test
instructions and good tests. @KISS-Web-Designhttps://github.com/KISS-Web-Designcan you provide instructions on testing this?

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/2494#issuecomment-30288817
.

Chris Jones-Gill
KISS Web Design
Company number: 7486736
VAT Registration: 107800539

betweenbrain · 2013-12-11T03:22:20Z

No problem, I'll write something up and post it here and on the tracker.

Awesome, thanks.

Chris-Jones-Gill · 2013-12-11T20:50:10Z

Test Instructions

You will need to be comfortable using the web inspector tools of your preferred browser, I use Chrome Inspector and Firebug (on Firefox).

1. Before applying the patch ensure that the MooTools keepAlive function is loaded.

visit yoursitename/index.php/component/users/?view=login

Check the <head> for the following code

function keepAlive() {  var myAjax = new Request({method: "get", url: "index.php"}).send();} window.addEvent("domready", function(){ keepAlive.periodical(840000); });

It will look something like this

Wait for at least 15 minutes, then use the inspector to check that the site has been reloaded in the background - evidenced by a GET of the page.

It will look something like this

2. Apply the patch, and reload the site page.

Or navigate back to yoursitename/index.php/component/users/?view=login

Check the <head> for the following code

            jQuery(document).ready(function() {
                window.setInterval(
                    function(){ new jQuery.ajax({ type: "GET" }); },
                    840000
                );
            });

It will look something like this

Wait for at least 15 minutes, then use the inspector to check that the site has been reloaded in the background - evidenced by a GET of the page.

It will look something like this

3. Respond here

With a +1, success, or other positive test result affirmation.
OR
With a -1, fails, or other negative test result affirmation - along with a good description of your environment, which part doesn't work (with screenshots and/or source copy/paste), and any other helpful information.

Thanks for testing.

Chris.

Bakual · 2013-12-12T07:39:32Z

Thanks for the instructions. Just a note: The test results should be posted in the tracker. Of course you can also add them here, but the tracker is currently still the point of thruth.

Chris-Jones-Gill · 2013-12-12T13:54:04Z

@Bakual , I have also posted the same instructions on the tracker - but there are some people that don't have tracker accounts (or refuse to use it), so I would rather have test results posted here and there than exclude people.

I don't mind doing a copy/paste from here to the tracker to keep all the results together.

Bakual · 2013-12-12T14:18:22Z

@KISS-Web-Design agreed :)

mbabker · 2014-04-08T23:31:24Z

Closed in favor of #3376

Chris-Jones-Gill added 3 commits November 11, 2013 16:14

Update gitignore

4091df3

Change keepAlive to use jQuery instead of MooTools framework

a8db650

corrected comment typos

d6ee3d2

beat reviewed Nov 11, 2013
View reviewed changes

reset gitignore to default project state

3d929d7

Chris-Jones-Gill added 3 commits November 15, 2013 10:09

Remove JHtmlBehavior::framework from the keepalive unit test as it is…

301f9ee

… no longer required.

Add test to see if jQuery is loaded before testing keepalive behaviour

79d5e14

Refactor the keepalive js so that it is easier to read when added wit…

3e80bff

…h other scripts.

wilsonge reviewed Nov 15, 2013
View reviewed changes

Chris-Jones-Gill added 3 commits November 15, 2013 15:30

Corrected the test for keepalive and switcher by getting the document…

271102a

… instance BEFORE referencing it. D'oh! Added the jQuery test to testSwitcher by mistake previously, but as it is a valid test it has been corrected and left in.

Initialise the Bootstrap JS framework before testing for jQuery

df9eaca

Chris-Jones-Gill mentioned this pull request Mar 29, 2014

[Remove Mootools] Behavior Keepalive without any library dependency. #3376

Merged

Chris-Jones-Gill changed the title ~~Feature jquery keepalive~~ [#32665] Feature jquery keepalive Apr 8, 2014

Uh oh!

Conversation

Chris-Jones-Gill commented Nov 11, 2013

Uh oh!

beat Nov 11, 2013

Choose a reason for hiding this comment

Uh oh!

Chris-Jones-Gill Nov 11, 2013

Choose a reason for hiding this comment

Uh oh!

wilsonge commented Nov 15, 2013

Uh oh!

piotr-cz commented Nov 15, 2013

Uh oh!

Chris-Jones-Gill commented Nov 15, 2013

$( handler )

Uh oh!

wilsonge Nov 15, 2013

Choose a reason for hiding this comment

Uh oh!

Chris-Jones-Gill Nov 15, 2013

Choose a reason for hiding this comment

Uh oh!

wilsonge Nov 15, 2013

Choose a reason for hiding this comment

Uh oh!

mbabker Nov 15, 2013

Choose a reason for hiding this comment

Uh oh!

betweenbrain commented Dec 11, 2013

Uh oh!

Chris-Jones-Gill commented Dec 11, 2013

Uh oh!

betweenbrain commented Dec 11, 2013

Uh oh!

Chris-Jones-Gill commented Dec 11, 2013

Test Instructions

1. Before applying the patch ensure that the MooTools keepAlive function is loaded.

2. Apply the patch, and reload the site page.

3. Respond here

Uh oh!

Bakual commented Dec 12, 2013

Uh oh!

Chris-Jones-Gill commented Dec 12, 2013

Uh oh!

Bakual commented Dec 12, 2013

Uh oh!

mbabker commented Apr 8, 2014

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants