Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Automatically send batch request #91

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

RFreij
Copy link

@RFreij RFreij commented Sep 30, 2020

Hi, first of all thank you @alberto-bottarini for creating the batch functionality.

In the discussion of #83 @jorgeborges said he would prefer the batch to be sent automatically after the 20 enqueued url's have been reached. Personally I like this approach so this is why i'm creating this pull request.

I've also added a hasEnqueuedUrls method. This method allows us to determine if any enqueued url's are still available before sending the response back to the client. If any are available then send the enqueued hits. We're using Laravel so I created a middleware to do exactly that for each response.

This way we don't have to worry about the limit.

It would be nice to have at least the hasEnqueuedUrls method added. I understand the philosophy behind the exception approach as well, if you decide to keep it that way than i will create something myself.

I coudn't get phpunit to work, i got the following error:

image

I'll check them again if they do not pass.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 5814773 on bsbip:automatic-queue into 6136c2f on theiconic:master.

@alberto-bottarini
Copy link
Contributor

Thanks!

As I already said, I don't like this "magic" implementation.
I understand your needs but I would prefer control over automation.

We don't know any users needs. Maybe this automation could create some unwanted situation.

@jorgeborges
Copy link
Contributor

jorgeborges commented Oct 9, 2020

Thanks for the PR @RFreij.

To be honest, I don't have hard feelings on either approach. I would prefer the auto event flush just because that seems to make the library easier to use, but on the other hand, @alberto-bottarini did point out the rest of the library throws exceptions when it's used wrong (such as when not all required data for a hit is included in the Analytics object), so having it throw the Exception makes things consistent.

Another approach would be to make this configurable, by default it doesn't auto send the batch, but if you call a method called enableAutoBatchHitSend or the like, then that activates the auto flush.

I think we can leave this one up to the community to decide, I'll create an issue, @ all the people who has contributed to the library, and depending on the number of votes using 👍 or 👎 we'll decide.

@friedrichroell
Copy link

... or add a wrapper method called enqueOrSend<hit-type>() which also gets rid of any "magic".

PS: I want the hasEnqueuedUrls() method in this PR :)
PPS: last but not least, if this PR gets rejected there is still the possibility of extending the Analytics class:

<?php

use TheIconic\Tracking\GoogleAnalytics\Analytics;

class AnalyticsDriver extends Analytics
{
    protected $autoFlushEnabled = false;

    public function enableAutoFlush()
    {
        $this->autoFlushEnabled = true;

        return $this;
    }

    public function disableAutoFlush()
    {
        $this->autoFlushEnabled = false;

        return $this;
    }

    protected function enqueueHit($methodName)
    {
        if (count($this->enqueuedUrls) == 20 && $this->autoFlushEnabled) {
            $this->sendEnqueuedHits();
        }

        return parent::enqueueHit($methodName);
    }
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants