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

Fix JS error on Performance page #8456

Merged
merged 2 commits into from Oct 31, 2017

Conversation

Projects
None yet
2 participants
@Quetzacoalt91
Member

Quetzacoalt91 commented Oct 31, 2017

Questions Answers
Branch? develop
Description? When the additional cache is disabled, we cannot send anymore the performance form. This PR fixes a JS error and the form submission.
Type? bug fix
Category? BO
BC breaks? Nope
Deprecations? Nope
Fixed ticket? /
How to test? Try to send the form without cache server selected.

This change is Reviewable

@Quetzacoalt91 Quetzacoalt91 requested a review from mickaelandrieu Oct 31, 2017

@Quetzacoalt91 Quetzacoalt91 added this to the 1.7.3.0 milestone Oct 31, 2017

var memcacheServersListBlock = document.getElementById('servers-list');
var newServerBtn = document.getElementById('new-server-btn');
var isMemcache = cacheSelected === "CacheMemcache" || cacheSelected === "CacheMemcached";
var isMemcache = cacheSelected && (cacheSelected.value === "CacheMemcache" || cacheSelected.value === "CacheMemcached");

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Oct 31, 2017

Contributor

are you sure? I don't see the value of this refactoring

@mickaelandrieu

mickaelandrieu Oct 31, 2017

Contributor

are you sure? I don't see the value of this refactoring

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Oct 31, 2017

Contributor

still, if it's ok for you I merge :) Thanks!

@mickaelandrieu

mickaelandrieu Oct 31, 2017

Contributor

still, if it's ok for you I merge :) Thanks!

This comment has been minimized.

@Quetzacoalt91

Quetzacoalt91 Oct 31, 2017

Member

This is more a bug fix than only a refacto.

Because no option was checked on my form, document.querySelector('input[name="form[caching][caching_system]"]:checked') returned null. Trying to get the value from it threw an error on the JS console.

@Quetzacoalt91

Quetzacoalt91 Oct 31, 2017

Member

This is more a bug fix than only a refacto.

Because no option was checked on my form, document.querySelector('input[name="form[caching][caching_system]"]:checked') returned null. Trying to get the value from it threw an error on the JS console.

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Oct 31, 2017

Contributor

Ok, would you mind to fix the same issue here too ? https://github.com/Quetzacoalt91/PrestaShop/blob/c24f5dea384b45b6fb6c86410f40db7efc120b87/admin-dev/themes/default/js/bundle/admin_parameters/performancePageUI.js#L28 (some duplication, so probably the same bug?) and then ping me and I merge :)

Good job!

@mickaelandrieu

mickaelandrieu Oct 31, 2017

Contributor

Ok, would you mind to fix the same issue here too ? https://github.com/Quetzacoalt91/PrestaShop/blob/c24f5dea384b45b6fb6c86410f40db7efc120b87/admin-dev/themes/default/js/bundle/admin_parameters/performancePageUI.js#L28 (some duplication, so probably the same bug?) and then ping me and I merge :)

Good job!

This comment has been minimized.

@Quetzacoalt91

Quetzacoalt91 Oct 31, 2017

Member

I just commited that change. Thanks

@Quetzacoalt91

Quetzacoalt91 Oct 31, 2017

Member

I just commited that change. Thanks

@mickaelandrieu mickaelandrieu merged commit 3cf897a into PrestaShop:develop Oct 31, 2017

2 checks passed

codacy/pr Good work! A positive pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Oct 31, 2017

Contributor

Thanks mate

Contributor

mickaelandrieu commented Oct 31, 2017

Thanks mate

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