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
Modify IdSite dimension so it is visible in customreports UI for use w/ CustomReports. #12693
Conversation
plugins/CoreHome/Columns/IdSite.php
Outdated
try { | ||
$siteName = Site::getNameFor($value); | ||
return $siteName . ' (ID ' . $value . ')'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really need the ID in there? I think it would be much better without.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was requested by @mattab (in slack), can you answer this Matt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually would it be possible to have it as a metadata? If not, we can remove it as it will look better/simpler without.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only way I can think of to add it as metadata would be to:
- create a new method in base
Dimension
class likedecorateRow($row)
that accepts the row object. (doesn't have to be @api) - in CustomReports, call this method before calling
formatValue()
. - then in IdSite, add a
decorateRow()
method to set the idsite as row metadata.
If you (@mattab / @tsteur) approve of this approach, then would take me ~30 mins.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@diosmosis lets not do this for now. Dimensions should not have any information about rows etc and I would prefer we don't hack anything in there now just to have something there we don't even really need right now etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, no worries.
plugins/CoreHome/Columns/IdSite.php
Outdated
protected $type = self::TYPE_TEXT; | ||
|
||
public function getDbColumnJoin() | ||
{ | ||
return new SiteNameJoin(); | ||
return new SiteIdJoin(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Considering it is a idsite already does it need to be joined? or is it there to ignore removed sites?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assumed it was necessary (I'm not familiar with this code). SiteNameJoin joins on idsite, but selects the name, figured I'd need another join, but I guess this join is just for extra data during aggregation?
I guess we can select for sites that aren't deleted, I'm not sure if it makes sense. If a rollup has a deleted site, then it should be fixed in the rollup, not ignored, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, when a site is deleted, it should be also removed from the roll-up. And when using this dimension for an individual site (not for a roll-up), and this site is deleted, this report would not be archived anymore. It would be only a problem if for some reason the site was not removed from the roll-up automatically.
The join is not needed but can be used when resolving extra data like idaction_url
wants to be resolved to log_action.name
.
This method to get a join getDbColumnJoin()
is also used by GDPR features (in a PR) to fetch all data stored for a visitor / data subject.
This join is ignored though so we can probably just remove the join. It would be only needed if we directly wanted to get the name of the site during archiving.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha.
plugins/CoreHome/Columns/IdSite.php
Outdated
return $siteName . ' (ID ' . $value . ')'; | ||
} catch (\Exception $ex) { | ||
return parent::formatValue($value, $idSite, $formatter); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When a site was deleted and name cannot be resolved, does it make sense to format like "Site ID: XYZ"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, can do this.
f1e49d1
to
409793c
Compare
…te ID to formatted label value.
409793c
to
ec84218
Compare
Updated the PR |
Haven't tested it but looks good to me 👍 |
…te ID to formatted label value. (matomo-org#12693)
Changes:
Fixes #12409
CC @mattab / @tsteur