@diosmosis opened this Pull Request on April 3rd 2020 Member

Fix #14094

@mattab commented on April 8th 2020 Member

Test case:

  • If custom reports are using one of these dimensions we now remove, can you test it and check what will happen? will the core:archive fail, or will the "bogus" custom report be skipped and disabled automatically or so?
@diosmosis commented on April 8th 2020 Member

@mattab 👍 if possible will try to make it work in that case

@diosmosis commented on April 8th 2020 Member

@mattab it looks like if one of those dimensions is used, it is ignored and effectively removed from the list. I was hoping to be able to add the dimension back as a view on the seconds one, but it doesn't seem like it's possible to have a dimension that is a "view" of another. I also noticed w/o it, the days dimension disappears and only the seconds one is there. This seems like it would be a problem for most users since seconds is harder to use when they want days. @tsteur can you think of a way around this? Is there a way to add a dimension class w/o it being added to a table but still having an associated column so customreports can aggregate on it?

@tsteur commented on April 8th 2020 Member

@diosmosis Custom reports uses as unique ID the dimension id from Dimension::getId() which is by default the PluginName.DimensionClassName. Eg Goals.DaysToConversion.

So it could help to recreate the old dimension classes. The SQL segment in that class would be something like

class DaysSince...Dimension {
    protected $type = self::TYPE_NUMBER;
    protected $sqlSegment = 'ROUND(log_visit.days_since_last_visit / 86400)';
    protected $nameSingular = 'General_DaysSinceFirstVisit';
}

you could use the same class to add back the previous segments which you are currently adding in addSegments manually.

class VisitorDaysSinceFirst {
    protected $columnName = 'visitor_days_since_first';
    protected $segmentName = 'daysSinceFirstVisit';
    protected $type = self::TYPE_NUMBER;
    protected $sqlSegment = 'ROUND(log_visit.days_since_last_visit / 86400)';
    protected $nameSingular = 'General_DaysSinceFirstVisit';
}

I think this should the trick. The seconds since first XYZ we will later need to disable in Custom Reports since they would be generating too many rows and isn't really useful for Custom Reports and could cause significant performance problems

@mattab commented on April 8th 2020 Member

we will later need to disable in Custom Reports

Btw if we could deal with this in general that'd be great. For example a couple of times before we had the same issue as here: a user/customer used to have a plugin (for example Provider) and some custom reports used the dimension in that plugin (eg. Provider name) and then the plugin is deactivated and the archiving then fails/errors. The fix then is to delete custom reports in the DB that contain the dimension (which is not best user experience and requires support assistance)

@diosmosis commented on April 23rd 2020 Member

ready for review

@diosmosis commented on May 21st 2020 Member

build js

@diosmosis commented on May 25th 2020 Member

@tsteur applied review comments. There's still a failing test for the 1.5 update change. Let me know if you think that's a mistake on the tests or not.

@diosmosis commented on June 4th 2020 Member

Not sure if we need to check in getValuesFromVisitorIdCookie whether the value looks more like a timestamp?

Forgot about this, added placeholder values back in, seems like the simplest approach.

@diosmosis commented on June 4th 2020 Member

The currentVisitTs seems no longer used in general?

No it's not, looks like we can simplify things even more here. Don't need the old cookie values now since they are all unused.

@tsteur commented on June 24th 2020 Member

@diosmosis is there much left to do in this PR?

@diosmosis commented on June 24th 2020 Member

@tsteur this was ready for another review. I must've forgotten to ping you.

@tsteur commented on June 24th 2020 Member

Looks good @diosmosis

@diosmosis commented on June 25th 2020 Member

build js

@diosmosis commented on June 26th 2020 Member

@tsteur / @sgiehl I updated OmniFixture.sql to have a lower version so the update would run on it fully. This, strangely, causes the ip addresses displayed in the visitor log to change to 0.0.0.0. I looked in the SQL file, and that's the correct value. When I look in the db for the branch and in 4.x-dev, the ip address for the visits shown are 0.0.0.0. But for some reason on 4.x-dev, a completely different ip address is shown... because of the update being run I guess... not sure what the issue is, but I'm merging this. Can fix the test if anyone knows what's going on.

This Pull Request was closed on June 26th 2020
Powered by GitHub Issue Mirror