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
Correctly set date in page title when period changed via period selector #12222
Correctly set date in page title when period changed via period selector #12222
Conversation
Huh, okay - I got my branch updated, no conflicts when merging upstream/master (as outlined in your Contributing to Piwik Core guide). Shouldn't I be merging upstream/3.x-dev instead though? Quite the conflict there! :) It looks like I should be working in the AngularJS PeriodSelectorController now. |
Actually we use |
94488ff
to
182e608
Compare
Rebased on I went ahead and and refactored the JS logic to its own function |
182e608
to
724c13b
Compare
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.
Thanks for the PR! Requested a couple changes, happy to answer any questions if you have them.
core/Plugin/Controller.php
Outdated
} | ||
|
||
return $period->getLocalizedLongString(); | ||
} |
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 think if we can avoid having this logic in two places, it would be better. From looking at the Controller::setGeneralVariablesView()
method, it looks like the reason it's difficult to pull this out, is because each variable (or nearly each) is set as a property on the view.
Maybe we can create a getCurrentPeriodInfo()
method (w/ or w/o a better name) that returns an array with all the information needed in setGeneralVariablesView()
. In setGeneralVariablesView()
, you'd set the result as properties on $view
, and in CoreHome:: getDateString()
you'd just use the localized long string. Can you do something like that here?
Might look something like:
class Controller
{
protected function getCurrentPeriodInfo()
{
$dateStart = // ...
$dateEnd = // ...
// ...
$prettyDateLong = // ...
return [
'dateStart' => $dateStart,
'dateEnd' => $dateEnd,
// ...
'prettyDateLong' => $prettyDateLong,
];
}
protected function setGeneralVariablesView($view)
{
// ...
$dateInfo = $this->getCurrentPeriodInfo();
foreach ($dateInfo as $name => $value) {
$view->$name = $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.
Sounds good to me 👍 I used the name getCurrentDatePeriodVars()
(can change if advised).
I struggled a bit to extract the side-effects that are tangled up with this "date and period" logic: specifically, the calls to Period::checkDateFormat($rawDate)
which throws if date is invalid, and $this->setDate($validDate)
, which modifies a property on $this
.
I decided to leave these side-effects in the getCurrentDatePeriodVars()
method. For the former, this validation seems important. For the latter, there would be a fair amount of logic left duplicated. Let me know if you have any input on this!
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 see what you mean about setDate()
, those side effects could result in confusion in the future. setDate()
sets the $this->strDate
& $this->date
properties on the controller, but looks like they aren't used in getCurrentDatePeriodVars()
, so we could potentially move the setDate()
call to after $dateVars = $this->getCurrentDatePeriodVars();
.
Something like:
// in getCurrentDatePeriodVars() return the $validDate variable too...
$validDate = null;
if ($periodStr != 'range') {
// ...
$validDate = ...
} else {
// ...
}
// ...
return [
// ...
'validDate' => $validDate,
];
then in setGeneralVariablesView()
:
$dateVars = $this->getCurrentDatePeriodVars();
if (Date::factory($this->strDate)->toString() != $validDate->toString()) {
// ... comment ...
$this->setDate($validDate);
}
then the side effect will be limited to the setGeneralVariablesView() method. Think it'll work?
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.
Yep, I think that will work just fine!
plugins/CoreHome/Controller.php
Outdated
$date = Common::getRequestVar('date', null, 'string'); | ||
$period = Common::getRequestVar('period', null, 'string'); | ||
|
||
$longPrettyString = $this->getLongPrettyDate($date, $period); |
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.
The getLongPrettyDate()
method doesn't take parameters, so no need to pass these two in.
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.
🤦♂️
@@ -62,6 +66,22 @@ | |||
return false; | |||
} | |||
} | |||
|
|||
function updateDateInTitle( date, period ) { | |||
piwikApi = piwikApi || $injector.get('piwikApi'); |
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'm guessing you're using the $injector
here to avoid a dependency cycle? Would be nice if we could avoid the cycle, but I can't think of a simple way to do it...
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.
Correct. I couldn't see a simple solution around it either :/
|
||
function PeriodSelectorController(piwik, $location, piwikPeriods) { | ||
function PeriodSelectorController(piwik, $location, piwikPeriods, piwikApi) { |
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.
Don't need piwikApi
here, correct?
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.
Hm, yes I think you are correct. I don't remember why I added this 🤷♂️
724c13b
to
f8f4c95
Compare
@spacenate left a comment on this thread: #12222 (comment) (which may be hidden by github) Everything else looks good, so once that's dealt w/, I'll test it and if it's good to go, merge this PR. |
plugins/CoreHome/Controller.php
Outdated
@@ -300,4 +300,11 @@ public function saveViewDataTableParameters() | |||
|
|||
ViewDataTableManager::saveViewDataTableParameters($login, $reportId, $parameters); | |||
} | |||
|
|||
public function getDateString() |
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 would need to be an API method, and would also need to check for permissions.
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.
Like Piwik::checkUserHasSomeViewAccess();
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.
Hi @tsteur, good catch on the missing function-level access control! I can add this permission check.
What does it mean, this needs to be an API method? Does it need to get documented somewhere, or some code changed?
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.
It would be documented kind of automatically once it is an API method. As suggested below, I would maybe remove all changes in PHP and instead only add the date in JS to the title (always). This would remove a lot of problems and complexity plus ensure consistent behaviour.
|
||
function updateDateInTitle( date, period ) { | ||
piwikApi = piwikApi || $injector.get('piwikApi'); | ||
piwikApi.fetch( |
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 wonder if we could keep it simple and if the date was changed in JS, simply show the same date format in the title as the date picker does? It wouldn't be hugely formatted, but maybe good enough.
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.
It would simplify a few things in this PR for sure. Controller::getLongPrettyDate()
would not be needed (I think), so all changes to core/plugin/Controller.php
and plugins/CoreHome/Controller.php
could be reverted.
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.
A non-essential XHR would also be avoided (tiny perf gain for the front-end app)
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.
@spacenate Sounds good, would be great if you can make these changes 👍
} | ||
).then(function (response) { | ||
if (response && response.date) { | ||
document.title = piwik.siteName + ' - ' + response.date + ' - ' + _pk_translate('CoreHome_WebAnalyticsReports') + ' - Piwik'; |
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.
We cannot hardcode "Piwik" here (which should be "Matomo" btw), because when for example a custom logo is used, we do not show "Matomo" in the title.
I don't know how this can be solved as it'll be hard to detect which part of the title needs to be replaced with a title. And the title may be different to the title specified here.
I wouldn't even mind if we just removed the date from the title in general.
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.
Thinking about it... I reckon if we still wanted to have the date in the page title, I would say lets only set the date in JS and not in PHP/Twig at all. This will be easiest and allows us to change the title etc. and have logic only in one place. So JS could append the date to whatever title is currently set, and when date is changed, remove the last part and update it again. Would make it a lot simpler and more consistent.
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 really like the idea of de-duplicating this logic. It would be simple to cache the title that was generated on the back-end, so no tomfoolery is needed when updating the title again (just append the new date string to the cached title). Shall I make this change in this PR?
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 PR or another PR 👍
989e38e
to
64f4e15
Compare
@@ -8,7 +8,7 @@ | |||
<![endif]--> | |||
{% endblock %} | |||
|
|||
{% set title %}{{ siteName|raw }}{% if prettyDateLong is defined %} - {{ prettyDateLong }}{% endif %} - {{ 'CoreHome_WebAnalyticsReports'|translate }}{% endset %} | |||
{% set title %}{{ siteName|raw }} - {{ 'CoreHome_WebAnalyticsReports'|translate }}{% endset %} |
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.
prettyDateLong
does not seem to be used in any other view templates. Should I remove it from core/Plugin/Controller.php:L(633)
?
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.
Other plugins might make use of it, so better to keep it IMO. Though you could leave a note in Controller.php that it's not used by core.
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 have squashed my commits to make the changes in this PR more clear. Sorry for the delay, and please let me know if there are any other changes you would like to see here! |
Oh rats - I think I see why the delay. I forgot to remove "WIP" from the PR title 🤦♂️ |
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.
Tested the code, works well. Left two comments, kudos @spacenate on fixing this with such a minimal change 👍 , apologies for letting this PR slip :)
@@ -8,7 +8,7 @@ | |||
<![endif]--> | |||
{% endblock %} | |||
|
|||
{% set title %}{{ siteName|raw }}{% if prettyDateLong is defined %} - {{ prettyDateLong }}{% endif %} - {{ 'CoreHome_WebAnalyticsReports'|translate }}{% endset %} | |||
{% set title %}{{ siteName|raw }} - {{ 'CoreHome_WebAnalyticsReports'|translate }}{% endset %} |
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.
Other plugins might make use of it, so better to keep it IMO. Though you could leave a note in Controller.php that it's not used by core.
@@ -185,6 +185,8 @@ | |||
} | |||
|
|||
function propagateNewUrlParams(date, period) { | |||
piwik.updateDateInTitle(date, period); |
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.
Noticed this isn't really necessary, could probably remove it. Below since $location.search()
is called, updatePeriodParamsFromUrl()
ends up being called in piwikService
. It would be useful outside of an angular context, but that only happens in the embedded pages, and since those are shown in iframes, the document title isn't really that useful
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! I wasn't certain about if or when angular might not be rendering the page, so I added this here to be safe. It sounds like it is unnecessary, so I will go ahead and remove it.
…CoreHome javascript to update page title with date string.
64f4e15
to
0cf6675
Compare
Thanks again for the contribution @spacenate! |
Very useful improvement @spacenate 👍 (fyi created this follow up issue: Page title in Tag Manager, Help page, Admin pages show the date but there is no calendar in the page #13353 ) |
…CoreHome javascript to update page title with date string. (matomo-org#12222) Correctly set date in page title when period changed via period selector
Fixes #12162
Hello! In this change:
getLongPrettyDate
toPiwik\Plugin\Controller
.getDateString
toPiwik\Plugins\CoreHome\Controller
.In calendar.js, the logic to fetch new string/update page title has been appended to(done)propagateNewUrlParams
, but in retrospect this would be better in its own function, perhaps still invoked frompropagateNewUrlParams
?In
Piwik\Plugin\Controller
, the logic insidegetLongPrettyDate
is currently is copy/pasted fromsetGeneralVariablesView
. I tried to tease apart the duplicated logic, but it started to become a time sink so I wanted to seek feedback on this approach before spending further time de-duplicating this logic.Your thoughts?