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

Impossible to change position of products #11475

Merged
merged 10 commits into from Nov 26, 2018

Conversation

Projects
None yet
5 participants
@PierreRambaud
Copy link
Contributor

PierreRambaud commented Nov 23, 2018

Questions Answers
Branch? 1.7.5.x
Description? Unable to change position when position start at 0. Reorder button appears with strange condition.
Type? bug fix
Category? BO
BC breaks? no
Deprecations? no
Fixed ticket? Fixes #11152.
How to test? Create new category, two products and try to reorder. Or just reorder after a fresh install.

This change is Reviewable

@PierreRambaud PierreRambaud force-pushed the PierreRambaud:fix/position-products branch from b155fc6 to 32f5a65 Nov 23, 2018

@PierreRambaud PierreRambaud force-pushed the PierreRambaud:fix/position-products branch from 32f5a65 to f0e37ff Nov 23, 2018

@jolelievre
Copy link
Contributor

jolelievre left a comment

nice catch @PierreRambaud

* We need to force the sort order when the order by
* is set to position_ordering
*/
if ('position_ordering' === $filters['orderBy']) {

This comment has been minimized.

@jolelievre

jolelievre Nov 23, 2018

Contributor

ok but I think it should be done in cleanFiltersForPositionOrdering

This comment has been minimized.

@PierreRambaud

PierreRambaud Nov 23, 2018

Contributor

I tought about it but this function is called after the first usage of $orderBy.
ProductController::150 => getCatalogPorduct
and the clean is at 191 :(

This comment has been minimized.

@jolelievre

jolelievre Nov 23, 2018

Contributor

you're right, this controller is a nightmare..
I'm thinking of better ways to manage this, but none seems acceptable..

This comment has been minimized.

@PierreRambaud

PierreRambaud Nov 23, 2018

Contributor

We will have to change everything in the future, we need to refactor how the old migrated controllers have been done :/

This comment has been minimized.

@jolelievre

jolelievre Nov 23, 2018

Contributor

yep @mickaelandrieu has prepared an issue about this, to plan how we will attack this controller!

Show resolved Hide resolved src/PrestaShopBundle/Controller/Admin/ProductController.php
'total' => $totalCount,
]
)
);
}
return $vars;

This comment has been minimized.

@jolelievre

jolelievre Nov 23, 2018

Contributor

this is not clear at all, sometimes we return a response, sometime it's an array..
we should always return the same thing, the convention chosen for new migrated controllers is to always explicitly return a Response via render
could you please remove the @Template association and use render instead?
it may not be necessary on all actions (although it would be nice), but this one especially is really messy

This comment has been minimized.

@PierreRambaud

PierreRambaud Nov 23, 2018

Contributor

I just add line break here :P It's from my cleanup commit.

This comment has been minimized.

@jolelievre

jolelievre Nov 23, 2018

Contributor

I know you didn't implement it, I was just thinking it could be the occasion to improve it
But it can be done later

This comment has been minimized.

@PierreRambaud

PierreRambaud Nov 23, 2018

Contributor

Too risky to change something and or refactor everything before the release :(

This comment has been minimized.

@PierreRambaud

PierreRambaud Nov 23, 2018

Contributor

But I thought about it ^^

This comment has been minimized.

@jolelievre

jolelievre Nov 23, 2018

Contributor

you're right!

Show resolved Hide resolved src/PrestaShopBundle/Controller/Admin/ProductController.php Outdated
@jolelievre
Copy link
Contributor

jolelievre left a comment

unless you're motivated to change the listAction method (but I would understand if you're not ^^) it's fine for me

@marionf marionf self-assigned this Nov 26, 2018

@marionf marionf added QA ✔️ and removed waiting for QA labels Nov 26, 2018

@marionf marionf removed their assignment Nov 26, 2018

@Quetzacoalt91 Quetzacoalt91 merged commit 47ff908 into PrestaShop:1.7.5.x Nov 26, 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 26, 2018

Thank you @PierreRambaud

@PierreRambaud PierreRambaud deleted the PierreRambaud:fix/position-products branch Nov 26, 2018

@jolelievre jolelievre changed the title impossible to change position of products Impossible to change position of products Dec 7, 2018

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