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

Don't display the system step if mandatory requirements are OK #9366

Merged
merged 1 commit into from Nov 29, 2018

Conversation

Projects
None yet
7 participants
@rGaillard
Copy link
Contributor

rGaillard commented Jul 23, 2018

Questions Answers
Branch? devleop
Description? Since the 1.7.0 version (71717b8), the system step was displayed every times on the installer.
Type? bug fix
Category? IN
BC breaks? no
Deprecations? no
How to test? In install wizard, System compatibility step is ignored when green.

This change is Reviewable

@Quetzacoalt91

This comment has been minimized.

Copy link
Member

Quetzacoalt91 commented Jul 23, 2018

@rGaillard Can you please make the PR targeting the develop branch?

If the bug was introduced in 1.7.0.0, we can wait for the next minor release.

@eternoendless eternoendless changed the title IN: Don't display the system step if mandatory requirements are OK Don't display the system step if mandatory requirements are OK Jul 23, 2018

@kpodemski

This comment has been minimized.

Copy link
Contributor

kpodemski commented Jul 27, 2018

@Quetzacoalt91

it can wait but... why? what is the point of waiting? guys, 1.x.y.z, Z is bugfix version, always, this is bugfix, why wait to Y for it? it's not a new feature... come on...

@mickaelandrieu

This comment has been minimized.

Copy link
Contributor

mickaelandrieu commented Jul 31, 2018

@kpodemski release new releases are really time-consuming tasks, you can't even imagine. So we can release a hundred iterations of 1.7.4.y "or" we can work on 1.7.5.

Our resources are limited, this doesn't mean we won't accept the patch, this means it will be part of 1.7.5.x release.

@Quetzacoalt91

This comment has been minimized.

Copy link
Member

Quetzacoalt91 commented Aug 6, 2018

I was in vacation so I couldn't answer you @kpodemski. Fortunately the team did it instead, and even better. :)

@rGaillard, let us know when it's done.

@rGaillard rGaillard changed the base branch from 1.7.4.x to develop Aug 6, 2018

@@ -242,6 +242,12 @@ public function setCurrentStep($step)
if (self::$steps->current()->getControllerInstance()->validate()) {
self::$steps->next();
}
// Don't display system step if mandatory requirements is valid
if (self::$steps->current()->getName() == 'system' && self::$steps->current()->getControllerInstance()->validate()) {

This comment has been minimized.

@Quetzacoalt91

Quetzacoalt91 Sep 3, 2018

Member

There is actually something I do not understand. How adding the same check for a specific step will solve the described issue?
There's probably an issue in the first validate() call which prevent the step to be properly validated. What do you think?

This comment has been minimized.

@Quetzacoalt91

Quetzacoalt91 Nov 28, 2018

Member

Had to find it by myself, but the code can be previously found in 71717b8#diff-314f7a40b328a8f143131097e1a37743L325

@Quetzacoalt91 Quetzacoalt91 force-pushed the rGaillard:install_system_step branch 2 times, most recently from c442c45 to 3bb6c69 Nov 28, 2018

@ntiepresta ntiepresta self-assigned this Nov 29, 2018

@ntiepresta ntiepresta added QA ✔️ and removed waiting for QA labels Nov 29, 2018

@ntiepresta ntiepresta removed their assignment Nov 29, 2018

@Quetzacoalt91 Quetzacoalt91 merged commit db1821d into PrestaShop:develop Nov 29, 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
@Quetzacoalt91

This comment has been minimized.

Copy link
Member

Quetzacoalt91 commented Nov 29, 2018

Thank you @rGaillard

@eternoendless eternoendless added this to the 1.7.6.0 milestone Dec 7, 2018

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