I tested this successfully, but I would not consider this a bug. I agree that hourly records are a bit excessive but this a known behaviour for a long time, see for example here: http://www.inmotionhosting.com/support/edu/joomla-3/banners/tracking

Also on an existing installation with activated tracking you would get mixed (hourly, daily) records which isn´t desirable. A neat solution could be another parameter - there aren´t that many for this component - that let´s you switch to daily records.

it is a bug if we call it daily when it is in fact hourly

On 27 January 2016 at 12:19, waader notifications@github.com wrote:

I tested this successfully, but I would not consider this a bug. I agree
that hourly records are a bit excessive but this a known behaviour for a
long time, see for example here:
http://www.inmotionhosting.com/support/edu/joomla-3/banners/tracking

Also on an existing installation with activated tracking you would get
mixed (hourly, daily) records which isn´t desirable. A neat solution could
be another parameter - there aren´t that many for this component - that

let´s you switch to daily records.

This comment was created with the J!Tracker Application
https://github.com/joomla/jissues at issues.joomla.org/joomla-cms/8825
https://issues.joomla.org/tracker/joomla-cms/8825.

—
Reply to this email directly or view it on GitHub
#8825 (comment).

Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
http://brian.teeman.net/

You probably mean the description in the mentioned help text. As I get no help text at all I searched via google and found that this behaviour is known for a long time. So in this case I would say, correct the help text. Anyway.

To me the code is wrong not the text

On 27 January 2016 at 12:42, waader notifications@github.com wrote:

You probably mean the description in the mentioned help text. As I get no
help text at all I searched via google and found that this behaviour is
known for a long time. So in this case I would say, correct the help text.
Anyway.

—
Reply to this email directly or view it on GitHub
#8825 (comment).

Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
http://brian.teeman.net/

I have tested this item successfully on 139d644

Does what it says but I see no issue here.

It would be a nice addition if you could choose between Hourly or Daily through the settings.
If you just change the code, existing environments will start working differently.
When you create a configuration option for it and keep the default the same you won't change existing websites.

This PR has received new commits.

CC: @waader

just a rebase to current staging.

This PR has received new commits.

CC: @waader

@brianteeman Plese check new language strings

Lol I was just commenting on the strings when I got the email :)

This PR has received new commits.

CC: @waader

@conconnl @waader Please test now

my seccond comment still applys ;)

this can't work because we set it to 'Y-m-d H' in line 81 and override it everytime with 'Y-m-d' in line 84.

I think moving the line 84 above the if can fix the issue:

$trackDate = JFactory::getDate()->format('Y-m-d');

 if ($track_frequency == 'hourly')
 {
    $trackDate = JFactory::getDate()->format('Y-m-d H');
 }

This PR has received new commits.

CC: @waader

This PR has received new commits.

CC: @waader

@zero-24 Thans for comments, PR updated

Looks better thanks ?

@wojsmol I Tested this correctly, but I'm seeing one inconsistency.

The text states: "These settings apply for all clients unless they are changed for a specific client. "
But the "Track Frequency" is not configurable on client level.
Maybe you can add this to the Client part and then everything is perfect.

This PR has received new commits.

CC: @waader

This PR has received new commits.

CC: @waader

@wojsmol The track frequency at client level does not work, it keeps jumping back to hourly.
Instead of the selected setting.

This PR has received new commits.

CC: @waader

This PR has received new commits.

CC: @waader

After applying the patch run the database fix in Extensions> Manage> Database.

I have tested this item ✅ successfully on ca1e68b

@wojsmol it works perfectly now

This PR has received new commits.

CC: @conconnl, @waader

@waader @alikon Plese test in postgresql and sqlazure if passible.

I have tested this item ✅ successfully on ec75c7a

After I have installed the patch and fixed the database I'm able to see the track frequency option.
On the client level of the banner I checked the daily option and it doesn't switch to hourly. I have tested with a mysql database.

So in my opinion it works correctly.

@icampus

I have tested this item ✅ successfully on ec75c7a

I applied the patch and got the track frequency option which works correctly.
I had done the database fix and can change between daily and hour now.
Using MySQL.
Tested@icampus

@waader @alikon requesting if you can test in postgresql and sqlazure if possible. Thanks.

rebase after #11619

I have tested this item ✅ successfully on 7428d3b

Installed the patch and could set the Track Frequency option to Hourly or Daily. I set the option to Hourly. In the Banner edit form I can set the Track Frequency to Hourly or Daily. In the Client edit form I can also set the Track Frequency to Hourly or Daily.

I have tested this item ✅ successfully on 7428d3b

Tested both options (hourly and daily) and it works like expected after the patch.

I have tested this item ? unsuccessfully on 7428d3b

Tested this one with PostgreSQL. Getting several errors, not only related to this issue. Needs a more thorough review.

If needed I can test more on PostgreSQL.

@slibbe Could I ask you to provide errors for PostgreSQL associated with this PR?

@slibbe can you report on a new issue what error you get using banner with postgresql if they are not related to this pr

@alikon Can you test this PR on postgresql?

@wojsmol If you could sync this patch I will test on postgresql and mssql.

@waader PR is rebased to staging

With postgres I get a syntax error for type timestamp with this query:

UPDATE #__banner_tracks SET "count" = ("count" + 1) WHERE track_type=1 AND banner_id=4 AND track_date='2017-04-10 09'

@waader Do you have any idea how to solve this problem? I have no experience with postgres.

cc @alikon see error reported by @waader any idea?

...maybe you could use CAST() ...

@alikon Plese send PR with appropriate changes.

@alikon Can you help here?

gently Reminder @alikon.

or @csthomas maybe?

the issue is to use a date to compare a timestamp field as reported 1 year and plus ago by @waader #8825 (comment)
see here https://github.com/joomla/joomla-cms/pull/8825/files#diff-4140a4b988c67c2915e88362dd9815ceR79 , so provide the correct data type or compare the same data type before issuing a comparison, this should be a normal rule not a postgresql rule only

I only did a code review.

IMO all lines $trackDate = JFactory::getDate()->format('Y-m-d H'); should be replaced by something like $trackDate = JFactory::getDate()->format('Y-m-d H:00:00'); and the same for $trackDate = JFactory::getDate()->format('Y-m-d'); should be replaced by $trackDate = JFactory::getDate()->format('Y-m-d 00:00:00');.

Then all values should be OK.

Another issue is that there is no time zone and everything will be calculated in UTC.

@wojsmol should this Pull Request test again or is it waiting for something else?

@franz-wohlkoenig PR is ready for testing.
@brianteeman Please accept language strings.

@wojsmol will test.

@wojsmol I requested changes 16 days ago

@brianteeman Please see 5ff5f0e ?

@brianteeman Fixed in latest commit.

@alikon AFTER removed in 4e3bc26

@wojsmol thanks

@wojsmol to be sure what to test can you give please Test Instructions?

I have tested this item ? unsuccessfully on 4e3bc26

When Track frequency is set in the banner details ("hourly" or "daily"), it always get the Track Frequency in the global configuration.

Fixes to read the track_frequency parameter of the component (banner and client):
banner_patch.txt
banners_patch.txt

This is the oldest PR we have and sadly it started with a bug fix and is now a new feature.

Our policy says no new features in patch levels and no new feature in combat version 3.10. I don't know why this PR hasn't merged before but now I would like to ask you @wojsmol to rebase this PR on J4.

This would be really great, thx. In the meantime I close this PR.