@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.

Powered by GitHub Issue Mirror