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

Regression with DatePicker and checkboxes on stock page (catalogue > stock) #9133

Merged
merged 5 commits into from Jun 4, 2018

Conversation

Projects
None yet
6 participants
@PierreRambaud
Contributor

PierreRambaud commented May 28, 2018

Questions Answers
Branch? 1.7.4.x
Description? DatePicker field not working due to wrong css selector. Wrong usage of v-model in PsCheckbox.vue (https://fr.vuejs.org/v2/guide/forms.html#Checkbox)
Type? bug fix
Category? BO
BC breaks? no
Deprecations? no
Fixed ticket? Partial http://forge.prestashop.com/browse/BOOM-5620
How to test? Try to click on datepicker field, for example in product page, Available quantity field.

This change is Reviewable

@prestonBot prestonBot added the 1.7.4.x label May 28, 2018

@PierreRambaud PierreRambaud changed the title from Regressions with DatePicker to Regression with DatePicker May 28, 2018

@PierreRambaud PierreRambaud added the Bug label May 28, 2018

@PierreRambaud

This comment has been minimized.

Contributor

PierreRambaud commented May 29, 2018

Same problem as @mickaelandrieu on #9137
@fatmaBouchekoua @khouloudbelguith Can you check please :)

@PierreRambaud PierreRambaud changed the title from Regression with DatePicker to Regression with DatePicker and checkbos on stock page (catalogue > stock) May 29, 2018

@fatmaBouchekoua

This comment has been minimized.

Contributor

fatmaBouchekoua commented May 30, 2018

Hello @PierreRambaud ,
I'm checking it. That's weird because it works perfectly on local.

@PierreRambaud

This comment has been minimized.

Contributor

PierreRambaud commented May 30, 2018

@fatmaBouchekoua Could you please send me the command you used to run this test? :)

@PierreRambaud

This comment has been minimized.

Contributor

PierreRambaud commented May 30, 2018

@fatmaBouchekoua I can reproduce the bug, while running E2E tests, I see the green message coming and disappeared one second after, and just after the check to verify the appearance of the green validation is executed.

@fatmaBouchekoua

This comment has been minimized.

Contributor

fatmaBouchekoua commented May 30, 2018

Hello @PierreRambaud ,
To run this test I use this command : npm test -- --URL=URLFrontOffice
That's weird, this is working for me on local shop and on Travis https://travis-ci.org/fatmaBouchekoua/PrestaShop/jobs/385647686
I will try to reproduce it on this branch even if this PR doen't have an impact on the product page.

@PierreRambaud

This comment has been minimized.

Contributor

PierreRambaud commented May 30, 2018

@fatmaBouchekoua Hey, I found what happened, a problem with package-lock.json file, it's now fix we will release a patch on 1.7.4.x and develop :)

@eternoendless

Please commit built assets in a separate commit, it will make rebasing easier

new NavBar();
new Header();
$(() => {
initDatePickers();

This comment has been minimized.

@eternoendless

eternoendless May 30, 2018

Member

This probably should be done only in pages containing datepickers, shouldn't it?

This comment has been minimized.

@PierreRambaud

PierreRambaud May 30, 2018

Contributor

It's a theme feature to have a DatePicker, it's not really specific to a single page.
When this selector isn't found, nothing happened, and it cost maybe 0.0005ms (:trollface:).
If we decide to move this only in pages containing datepickers, we maybe need to do the same for product-page/index, ./clickable-dropdown, and ./translation-page/index that are in theme.js. Wdyt?

This comment has been minimized.

@mickaelandrieu

mickaelandrieu May 30, 2018

Contributor

We need to check that adding it globally doesn't conflict with datepickers management in product page.

This comment has been minimized.

@PierreRambaud

PierreRambaud May 30, 2018

Contributor

@mickaelandrieu It's a fix because no datepickers working on product page =)

This comment has been minimized.

@eternoendless

eternoendless Jun 1, 2018

Member

Ok but we will have to do it eventually when we'll split the files per page sometime in the future

@PierreRambaud PierreRambaud changed the title from Regression with DatePicker and checkbos on stock page (catalogue > stock) to Regression with DatePicker and checkboxes on stock page (catalogue > stock) May 30, 2018

PierreRambaud added some commits May 30, 2018

Regression with DatePicker and checkboxes on stock page
DatePicker field not working due to wrong css selector.
And because it can be on every page, run datepicker function
directly on theme.js. Fix problem where we can't check checkboxes
on the stock page.
@PierreRambaud

This comment has been minimized.

Contributor

PierreRambaud commented May 30, 2018

@eternoendless I rebase everything in two separate commits :) Sorry for the delay

@marionf marionf added this to the 1.7.4.0 milestone May 31, 2018

new NavBar();
new Header();
$(() => {
initDatePickers();

This comment has been minimized.

@eternoendless

eternoendless Jun 1, 2018

Member

Ok but we will have to do it eventually when we'll split the files per page sometime in the future

@@ -47,6 +42,13 @@ import './product-page/index';
import './translation-page/index';
import Header from './header.js';
import initDatePickers from './app/utils/datepicker';
const $ = global.$;

This comment has been minimized.

@eternoendless

eternoendless Jun 1, 2018

Member

You may need to use window instead of global

This comment has been minimized.

@PierreRambaud

PierreRambaud Jun 1, 2018

Contributor

Should I need to replace every occurrences of global in other files?

This comment has been minimized.

@eternoendless

eternoendless Jun 4, 2018

Member

No, it will be automagically converted to window by webpack. It's just to avoid linter warnings.

@marionf

This comment has been minimized.

Contributor

marionf commented Jun 4, 2018

Hello @PierreRambaud

It's ok for checkbox
It's ok for datepicker in quantities tab for a standard product

It's not ok for datepicker in combination tab
capture du 2018-06-04 09-23-17

It's not ok for datepicker in stock movement page
capture du 2018-06-04 09-24-13

@eternoendless

This comment has been minimized.

Member

eternoendless commented Jun 4, 2018

Review status: all files reviewed at latest revision, all discussions resolved.


admin-dev/themes/new-theme/js/app/widgets/ps-checkbox.vue, line 49 at r4 (raw file):

      onClick() {
        this.checked = !this.checked;
      },

Doesn't removing this prevent the model from being updated?


Comments from Reviewable

@PierreRambaud

This comment has been minimized.

Contributor

PierreRambaud commented Jun 4, 2018

Review status: 14 of 15 files reviewed at latest revision, 1 unresolved discussion.


admin-dev/themes/new-theme/js/app/widgets/ps-checkbox.vue, line 49 at r4 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

Doesn't removing this prevent the model from being updated?

No, in fact, model and item aren't used here (only to emit checked event to parent.
And it's against the VueJs philosophy https://fr.vuejs.org/v2/guide/forms.html#Checkbox by setting a v-model on the input, the checked value is already toggled.


Comments from Reviewable

@marionf marionf added the QA ✔️ label Jun 4, 2018

@eternoendless

This comment has been minimized.

Member

eternoendless commented Jun 4, 2018

:lgtm:


Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.


admin-dev/themes/new-theme/js/app/widgets/ps-checkbox.vue, line 49 at r4 (raw file):

Previously, PierreRambaud (GoT) wrote…

No, in fact, model and item aren't used here (only to emit checked event to parent.
And it's against the VueJs philosophy https://fr.vuejs.org/v2/guide/forms.html#Checkbox by setting a v-model on the input, the checked value is already toggled.

magic.gif


Comments from Reviewable

@eternoendless

This comment has been minimized.

Member

eternoendless commented Jun 4, 2018

Thank you @PierreRambaud

@eternoendless eternoendless merged commit d54c7a2 into PrestaShop:1.7.4.x Jun 4, 2018

2 of 3 checks passed

Codacy/PR Quality Review Not up to standards. This pull request quality could be better.
Details
code-review/reviewable 15 files reviewed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@PierreRambaud PierreRambaud deleted the PierreRambaud:fix/datepicker branch Jun 4, 2018

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