Fix/improve behaviour of update check visualization #21290
Conversation
cddf824 to
04f17a7
Compare
|
Yet to run it locally but code changes look good from my perspective. |
core/UpdateCheck.php
Outdated
| Option::set(self::LAST_TIME_CHECKED, $lastTimeChecked, $autoLoad = 1); | ||
| } | ||
|
|
||
| Option::set(self::LAST_CHECK_FAILED, $hasLastCheckFailed, $autoLoad = 1); | ||
| Option::set(self::LATEST_VERSION, $latestVersion); |
There was a problem hiding this comment.
Does it make sense to update the version if the last check failed? Shouldn't we maybe keep the previous record in that case?
There was a problem hiding this comment.
Good point, it should not be worth setting the version if the check failed.
While it would prevent having "not the most up-to-date version" displayed it would also hide a previously available update if the request failed.
I also removed the autoLoad flag from the new option. After some previous code changes it is only ever read in the manual update action, no need to have it available for reading otherwise.
| {% set test_latest_version_available="4.0.0" %} | ||
| {% set test_piwikUrl='https://demo.matomo.org/' %} | ||
| {% set test_lastUpdateCheckFailed = false %} | ||
| {% set test_latest_version_available = '6.0.0' %} | ||
| {% set test_piwikUrl = 'https://demo.matomo.org/' %} |
There was a problem hiding this comment.
I guess those variables are only for testing purpose and in order to use them the test_ prefix needs to be removed? Not sure if we actually want to keep such things. If we want to provide an easy way to allow to overwrite those variables for testing purpose we should rather use an event or DI to allow that.
There was a problem hiding this comment.
I removed them. They were existing for 10 years, but with the new UI tests, that are setting the options to specific values, there should be no benefit to keeping them 👍
Co-authored-by: Michal Kleiner <michal@innocraft.com>
Description:
Additional work was done to get the manual check working. The parsing of the result was not yet updated for the new vue component structure and flagged every response as "no update available".
Fixes #21259.
Review