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

Fixes new webservice list behavior in multishop context and fixes warning message display #10781

Merged
merged 5 commits into from Oct 3, 2018

Conversation

Projects
None yet
8 participants
@tomas862
Contributor

tomas862 commented Sep 30, 2018

Questions Answers
Branch? develop
Description? The new webservie list which for now can be reached only via admin-dev/index.php/configure/advanced/webservice had missing multi-shop functionality. After the fix, the list will work the same as the legacy list in multishop context. Also improved a warning messages appearance above the list due to before messages sometimes duplicated itself.
Type? bug fix
Category? BO
BC breaks? no
Deprecations? no
Fixed ticket?
How to test? Create some webservice keys in shop 1 then create some keys in shop2. Then try to switch between shop contexts, try also go to "All shops" link - both legacy list and the new list located in admin-dev/index.php/configure/advanced/webservice should display the same amount of data in different shop contexts

This change is Reviewable

@matks

This comment has been minimized.

Contributor

matks commented Oct 1, 2018

This is nice @tomas862 👍

@matks matks added the waiting for QA label Oct 1, 2018

@matks matks added this to the 1.7.6.0 milestone Oct 1, 2018

$qb = $this->getQueryBuilder($searchCriteria->getFilters());
$qb->select('COUNT(wa.`id_webservice_account`)');
$qb = $this->getQueryBuilder($searchCriteria->getFilters())
->select('SQL_CALC_FOUND_ROWS')

This comment has been minimized.

@PierreRambaud

PierreRambaud Oct 1, 2018

Contributor

Why adding two columns in select result? and what is the gain of using SQL_CALC_FOUND_ROWS and FOUND_ROWS() instead of COUNT here? @jocel1 any idea?

This comment has been minimized.

@jocel1

jocel1 Oct 1, 2018

Contributor

I strongly recommend using COUNT when possible

This comment has been minimized.

@sarjon

sarjon Oct 1, 2018

Member

i think its better to use 2 queries, we already discussed it in another PR.

This comment has been minimized.

@PierreRambaud

PierreRambaud Oct 1, 2018

Contributor

I don't understand why it's a good idea to have 2 queries instead of one, when having two queries can have performance issues :/

This comment has been minimized.

@tomas862

tomas862 Oct 1, 2018

Contributor

Thanks for the insights 👍 - I managed to make it in just a single sql by separating group by and instead of plain COUNT I used the combination of COUNT and DISTINCT - and indeed, from reading some benchmarks, FOUND_ROWS is slower then COUNT

@@ -27,6 +27,19 @@
{% trans_default_domain "Admin.Advparameters.Feature" %}
{% block content %}
{% if configurationWarnings is not empty %}
<div class="row">

This comment has been minimized.

@PierreRambaud

PierreRambaud Oct 1, 2018

Contributor

add this into block maybe?

This comment has been minimized.

@tomas862

tomas862 Oct 1, 2018

Contributor

sure 🙂

@PierreRambaud

little change and information required :)

@marionf marionf added QA ✔️ and removed waiting for QA labels Oct 3, 2018

@Quetzacoalt91 Quetzacoalt91 merged commit 445d1f9 into PrestaShop:develop Oct 3, 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.

Member

Quetzacoalt91 commented Oct 3, 2018

Thank you @tomas862

@tomas862 tomas862 deleted the tomas862:bug_fixes/webservice_list_multishop branch Oct 4, 2018

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