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

Modify IdSite dimension so it is visible in customreports UI for use w/ CustomReports. #12693

Merged
merged 1 commit into from Apr 23, 2018

Conversation

diosmosis
Copy link
Member

Changes:

  • Added name singular/plural properties to IdSite dimension so it appears in CustomReports list.
  • Add SiteIdJoin and use in IdSite dimension so it selects the idsite and not the site name.
  • Add formatValue() method to IdSite to get the site name & retain the site ID (which is appended to the site name).

image

Fixes #12409

CC @mattab / @tsteur

@diosmosis diosmosis added Enhancement For new feature suggestions that enhance Matomo's capabilities or add a new report, new API etc. not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. labels Apr 1, 2018
@diosmosis diosmosis added this to the 3.4.1 milestone Apr 1, 2018
@diosmosis diosmosis added the Needs Review PRs that need a code review label Apr 1, 2018
try {
$siteName = Site::getNameFor($value);
return $siteName . ' (ID ' . $value . ')';
Copy link
Member

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.

Copy link
Member Author

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?

Copy link
Member

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.

Copy link
Member Author

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:

  1. create a new method in base Dimension class like decorateRow($row) that accepts the row object. (doesn't have to be @api)
  2. in CustomReports, call this method before calling formatValue().
  3. 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.

Copy link
Member

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, no worries.

protected $type = self::TYPE_TEXT;

public function getDbColumnJoin()
{
return new SiteNameJoin();
return new SiteIdJoin();
Copy link
Member

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?

Copy link
Member Author

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?

Copy link
Member

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha.

return $siteName . ' (ID ' . $value . ')';
} catch (\Exception $ex) {
return parent::formatValue($value, $idSite, $formatter);
Copy link
Member

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"?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, can do this.

@diosmosis
Copy link
Member Author

Updated the PR

@tsteur
Copy link
Member

tsteur commented Apr 2, 2018

Haven't tested it but looks good to me 👍

@mattab mattab merged commit ebf902a into 3.x-dev Apr 23, 2018
@mattab mattab deleted the site-id-name-dimensions branch April 23, 2018 02:05
InfinityVoid pushed a commit to InfinityVoid/matomo that referenced this pull request Oct 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement For new feature suggestions that enhance Matomo's capabilities or add a new report, new API etc. Needs Review PRs that need a code review not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants