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

Check at installation if PrestaShop version is the latest #9401

Merged
merged 16 commits into from Aug 30, 2018

Conversation

Projects
None yet
7 participants
@matks
Contributor

matks commented Aug 2, 2018

Questions Answers
Branch? develop
Description? Check at installation if PrestaShop version is the latest. If not, show the user a form to allow him to download and install the latest version.
Type? new feature
Category? IN
BC breaks? no
Deprecations? no
Fixed ticket? http://forge.prestashop.com/browse/FF-173
How to test? See below

How to test

It requires a specific zip archive to test: it must be a zip archive built using this PR but with a version number lower than the current PS version.

Ask @matks (Mathieu Ferment) to build a fake v1.7.4.1 for your tests.

Then, you can install this fake install. The form will appear and ask you whether you wish to install the latest version instead of the 1.7.4.1:

  • perform a regular 1.7.4.1 install by saying "No, thanks"
  • download & install the latest (should be 1.7.4.2) by saying "Yes, please!"

Pull Request summary

This PR does 3 things:

  1. Modify compile process for releases

The PR modifies the behavior of tools/build/Library/InstallUnpacker/compile.php and related files to allow:

  • injection of the release number into the built index.php using a placeholder
  • injection of php classes into the built index.php using to keep a single file
  1. Introduce a form at the install step to allow users to install the latest version

This basically introduces a 2nd page for the install step. It is being used only if the Installer is able to detect its version is not the latest stable available.

  1. If the user chooses to update its install version, replace current install by the latest one

This means:

  • to download the zip archive of the latest version
  • to unzip the archive
  • to replace current install files by the latest install files

If this process cannot be carried out, user is able to resume its regular installation. If possible, the reason why it could not be carried out is displayed as an error message before allowing the user to resume.

Known limitations and possible improvements

There are multiple @todo statements in the code as I can see multiple improvements but did not want to make this PR too big and too complex.
The most relevant ones:

  • php classes contain the Prestashop license header. When a new release is built, php classes are being copied into final build file index.php along with the license header who then appears multiple times in the final file.
  • downloaded zip archive is installed right away, without checking md5 checksums
  • no unit tests

Please tell me if you think these are vital for this PR and I will implement them.


This change is Reviewable

@matks

This comment has been minimized.

Show comment
Hide comment
@matks

matks Aug 2, 2018

Contributor

This is what the form looks like:
capture d ecran 2018-08-02 a 17 22 35

Contributor

matks commented Aug 2, 2018

This is what the form looks like:
capture d ecran 2018-08-02 a 17 22 35

@mickaelandrieu

I like the idea, but the code may be improved a little bit (using PHPStorm, you can extract function from portion of code in mostly 1 click)

@matks

This comment has been minimized.

Show comment
Hide comment
@matks

matks Aug 6, 2018

Contributor
(using PHPStorm, you can extract function from portion of code in mostly 1 click)

@mickaelandrieu Euh ... yes indeed. I do not see why you are saying this 😅 ?

Contributor

matks commented Aug 6, 2018

(using PHPStorm, you can extract function from portion of code in mostly 1 click)

@mickaelandrieu Euh ... yes indeed. I do not see why you are saying this 😅 ?

@PierreRambaud

This comment has been minimized.

Show comment
Hide comment
@PierreRambaud

PierreRambaud Aug 7, 2018

Contributor

@matks maybe trying to trolling me , but I can do the same with emacs (https://github.com/keelerm84/php-refactor-mode.el) :trollface:

Contributor

PierreRambaud commented Aug 7, 2018

@matks maybe trying to trolling me , but I can do the same with emacs (https://github.com/keelerm84/php-refactor-mode.el) :trollface:

@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Aug 7, 2018

Contributor

@mickaelandrieu Euh ... yes indeed. I do not see why you are saying this sweat_smile ?

Just in case, to show you it's really easy to do it 👼

Contributor

mickaelandrieu commented Aug 7, 2018

@mickaelandrieu Euh ... yes indeed. I do not see why you are saying this sweat_smile ?

Just in case, to show you it's really easy to do it 👼

@matks

This comment has been minimized.

Show comment
Hide comment
@matks

matks Aug 21, 2018

Contributor

Requested changes have been either answered or implemented in 51629e3

Please re-review and, if relevant, approve 😄 @Quetzacoalt91 @PierreRambaud ?

Contributor

matks commented Aug 21, 2018

Requested changes have been either answered or implemented in 51629e3

Please re-review and, if relevant, approve 😄 @Quetzacoalt91 @PierreRambaud ?

@matks

This comment has been minimized.

Show comment
Hide comment
@matks

matks Aug 22, 2018

Contributor

@mickaelandrieu Your requested change has been applied in 5ee798f

Please re-review and, if relevant, approve 😉

Contributor

matks commented Aug 22, 2018

@mickaelandrieu Your requested change has been applied in 5ee798f

Please re-review and, if relevant, approve 😉

@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Aug 22, 2018

Contributor

Thanks @matks !

Contributor

mickaelandrieu commented Aug 22, 2018

Thanks @matks !

@marionf marionf self-assigned this Aug 22, 2018

@marionf marionf removed their assignment Aug 22, 2018

@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Aug 22, 2018

Contributor

Hello @marionf, why is it in "waiting for author" state? I may have missed one of your comments ^^

For me, we only needs your validation here 👍

Edit: ping @matks, you may change your pull request description that say "DO NOT MERGE: specs have changed warning".. is it still true?

Contributor

mickaelandrieu commented Aug 22, 2018

Hello @marionf, why is it in "waiting for author" state? I may have missed one of your comments ^^

For me, we only needs your validation here 👍

Edit: ping @matks, you may change your pull request description that say "DO NOT MERGE: specs have changed warning".. is it still true?

@marionf

This comment has been minimized.

Show comment
Hide comment
@marionf

marionf Aug 23, 2018

Contributor

@mickaelandrieu
@matks add a comment here
We need to display a spinner while the page is loading because the loading time depends of addons API and that can take a while

Contributor

marionf commented Aug 23, 2018

@mickaelandrieu
@matks add a comment here
We need to display a spinner while the page is loading because the loading time depends of addons API and that can take a while

@matks

This comment has been minimized.

Show comment
Hide comment
@matks

matks Aug 29, 2018

Contributor

@PierreRambaud Requested changes have been applied in 7fb4cbe

Contributor

matks commented Aug 29, 2018

@PierreRambaud Requested changes have been applied in 7fb4cbe

@PierreRambaud

This comment has been minimized.

Show comment
Hide comment
@PierreRambaud

PierreRambaud Aug 29, 2018

Contributor

@mickaelandrieu @Quetzacoalt91 27 files, If someone can have a check too?

Contributor

PierreRambaud commented Aug 29, 2018

@mickaelandrieu @Quetzacoalt91 27 files, If someone can have a check too?

@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Aug 29, 2018

Contributor

@PierreRambaud too many changes now so we need again to ask QA...

Next time if QA is ok, don't review it again: it's better to do another pull request to improve a behavior than asking QA twice on the same contribution imo :)

Contributor

mickaelandrieu commented Aug 29, 2018

@PierreRambaud too many changes now so we need again to ask QA...

Next time if QA is ok, don't review it again: it's better to do another pull request to improve a behavior than asking QA twice on the same contribution imo :)

@marionf marionf assigned marionf and unassigned marionf Aug 30, 2018

@marionf marionf added QA ✔️ and removed waiting for QA labels Aug 30, 2018

@PierreRambaud

This comment has been minimized.

Show comment
Hide comment
@PierreRambaud

PierreRambaud Aug 30, 2018

Contributor

@mickaelandrieu We logically need QA after each validated modifications, even for build assets. What happened if someone forget to run npm install or keep building in dev mode.
More, many changes occurred from the first review. When there are 27 files and +3,289 −164, it maybe better (but not mandatory) to ask for another check :)

Contributor

PierreRambaud commented Aug 30, 2018

@mickaelandrieu We logically need QA after each validated modifications, even for build assets. What happened if someone forget to run npm install or keep building in dev mode.
More, many changes occurred from the first review. When there are 27 files and +3,289 −164, it maybe better (but not mandatory) to ask for another check :)

@PierreRambaud PierreRambaud merged commit 2d44ffa into PrestaShop:develop Aug 30, 2018

2 checks passed

Codacy/PR Quality Review Up to standards. A positive pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@PierreRambaud

This comment has been minimized.

Show comment
Hide comment
@PierreRambaud

PierreRambaud Aug 30, 2018

Contributor

Btw Thanks @matks ;)

Contributor

PierreRambaud commented Aug 30, 2018

Btw Thanks @matks ;)

@matks matks deleted the matks:FF-173_preinstall-check-to-get-latest-version branch Aug 30, 2018

@colinegin

This comment has been minimized.

Show comment
Hide comment
@colinegin

colinegin commented Aug 30, 2018

Fixes #10209

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment