@mnapoli opened this Pull Request on March 14th 2015 Member

In #6441 we added support for updating over HTTPS but in some situations this will fail:

In every case except the first one the user has legitimate reasons to ignore the HTTPS error and update via HTTP.

To address that, the user will be shown this screen if downloading the archive over HTTPS fails:

Please note that if the system doesn't support HTTPS downloads, it will use HTTP automatically so this use case is not related to this PR.

Technical details:

  • it will show the screen for any error while downloading over HTTPS, e.g. timeout or whatever. This is because the error messages depend on the method used (curl, fopen…) and the OS/PHP version so it's impossible to know exactly what went wrong. That's why we mustn't assume there is a MITM attack and don't be too alarming in the error message. Feedback on the wordings welcome.
  • includes UI tests
  • I moved existing CoreUpdater UI tests from tests/UI to the plugin folder
  • to cover the scenario of HTTPS update with UI tests I had to do the following changes:
    • refactored the updater by extracting the code of the controller into a Updater class using dependency injection
    • added a way to provide DI configuration in test fixtures: I've extracted this commit into #7434 for easier code review and discussions.
    • for my use case, I mock the Updater and simulate that 1) there is a new version available, 2) updating over HTTPS fails, 3) updating over HTTP succeeds
  • unfortunately the Updater itself is not tested but it wasn't before anyway. To test it I would have to mock many things so I'm not even sure it would be very meaningful. Anyway it's too hard today because there are too many things that are static/not using DI.
  • unrelated but I improved the diffviewer a bit (pushed that to master)

FYI looking at the diff commit by commit would probably be easier to read. Also the first commit here belong to #7434, it's better to review it in the separate PR.

TODO before merging:

TODO after merging:

  • [ ] remove the screenshots that were moved in this plugin from the UI screenshots repository (1 and 2)
@mnapoli commented on March 14th 2015 Member
@mnapoli commented on March 15th 2015 Member

I have extracted the "DI config in UI fixtures" commit into a separate pull request: #7434 That should allow an easier code review. #7434 Needs to be merged first.

@mattab commented on March 16th 2015 Owner

Quick note about:

I moved existing CoreUpdater UI tests from tests/UI to the plugin folder

let's leave all "core plugins" screenshot tests in tests/UI/expected-screenshots (eg. in separate repo: https://github.com/piwik/piwik-ui-tests) - if we want to change it, let's open a new issue to discuss - but I suspect the piwik/piwik git repository size increase that would result, would be a blocker to making this change...

FYI why it's important:

  • moving only one core plugin tests to the plugin folder while all other tests are in the tests/UI adds some inconsistency.
  • the main reason we decided to leave the screenshot away from the main git repo piwik/piwik is because adding screenshots to the repo increases the size of the git repo significantly (especially when we start pushing new screenshots and history builds up). Unfortunately git repo size increase has a few disadvantages (slower CI builds, slower release process, slower deploy from git etc)
@mnapoli commented on March 16th 2015 Member

OK will do!

@mattab commented on March 16th 2015 Owner

Very nice to see new UI tests for all these use cases :+1:


The screenshot should maybe show the URL it is downloading from (the URL appears blank)


Let's see together the messaging and do small changes

Otherwise, looks good!

(we will need to test on production by releaseing b4 and then beta5 and testing upgrade from b4 to b5 using this new codebase)

@mnapoli commented on March 16th 2015 Member

I've moved the UI tests and the screenshots into the main tests directory (+ submodule).

The screenshot should maybe show the URL it is downloading from (the URL appears blank)

The URL is blank because the updater is mocked. If I show a URL here it will be a fake one so it could just confuse us in the future if the URL changes, that's why I'd rather leave it that way.

@mattab commented on March 16th 2015 Owner
This Pull Request was closed on March 16th 2015
Powered by GitHub Issue Mirror