Skip to content
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 "jumping" taxons while moving up or down #15272

Merged
merged 7 commits into from
Sep 14, 2023
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -74,8 +74,7 @@ sylius_admin_ajax_taxon_move_up:
path: /{id}/move/up
jakubtobiasz marked this conversation as resolved.
Show resolved Hide resolved
methods: [PUT]
defaults:
limit: 1
_controller: sylius.controller.taxon_move:upAction
_controller: sylius.controller.taxon_position::moveUpAction
_format: json
_sylius:
permission: true
@@ -84,26 +83,7 @@ sylius_admin_ajax_taxon_move_down:
path: /{id}/move/down
methods: [PUT]
defaults:
limit: 1
_controller: sylius.controller.taxon_move:downAction
_format: json
_sylius:
permission: true

sylius_admin_ajax_taxon_move_top:
path: /{id}/move/top
methods: [PUT]
defaults:
_controller: sylius.controller.taxon_move:upAction
_format: json
_sylius:
permission: true

sylius_admin_ajax_taxon_move_end:
path: /{id}/move/end
methods: [PUT]
defaults:
_controller: sylius.controller.taxon_move:downAction
_controller: sylius.controller.taxon_position::moveDownAction
_format: json
_sylius:
permission: true
2 changes: 0 additions & 2 deletions src/Sylius/Bundle/AdminBundle/Resources/private/js/app.js
Original file line number Diff line number Diff line change
@@ -57,8 +57,6 @@ $(document).ready(() => {

$('.sylius-taxon-move-up').taxonMove();
$('.sylius-taxon-move-down').taxonMove();
$('.sylius-taxon-move-top').taxonMove();
$('.sylius-taxon-move-end').taxonMove();

$('#sylius_shipping_method_calculator').handlePrototypes({
prototypePrefix: 'sylius_shipping_method_calculator_calculators',
Original file line number Diff line number Diff line change
@@ -34,18 +34,12 @@

<div class="divider"></div>

<div class="item sylius-taxon-move-top" data-url="{{ path('sylius_admin_ajax_taxon_move_top', { id: taxon.id }) }}">
<i class="angle double up icon grey"></i>{{ 'sylius.ui.move_top'|trans }}
</div>
<div class="item sylius-taxon-move-up" data-url="{{ path('sylius_admin_ajax_taxon_move_up', { id: taxon.id }) }}">
jakubtobiasz marked this conversation as resolved.
Show resolved Hide resolved
<i class="arrow up icon grey"></i>{{ 'sylius.ui.move_up'|trans }}
</div>
<div class="item sylius-taxon-move-down" data-url="{{ path('sylius_admin_ajax_taxon_move_down', { id: taxon.id }) }}">
<i class="arrow down icon grey"></i>{{ 'sylius.ui.move_down'|trans }}
</div>
<div class="item sylius-taxon-move-end" data-url="{{ path('sylius_admin_ajax_taxon_move_end', { id: taxon.id }) }}">
<i class="angle double down icon grey"></i>{{ 'sylius.ui.move_end'|trans }}
</div>

<input type="hidden" name="_method" value="DELETE">
<input type="hidden" name="_csrf_token" value="{{ csrf_token(taxon.id) }}" />

This file was deleted.

Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
<?php

/*
* This file is part of the Sylius package.
*
* (c) Paweł Jędrzejewski
jakubtobiasz marked this conversation as resolved.
Show resolved Hide resolved
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

declare(strict_types=1);

namespace Sylius\Bundle\TaxonomyBundle\Controller;

use Doctrine\Persistence\ObjectManager;
use Sylius\Component\Taxonomy\Model\TaxonInterface;
use Sylius\Component\Taxonomy\Positioner\TaxonPositionerInterface;
use Sylius\Component\Taxonomy\Repository\TaxonRepositoryInterface;
use Symfony\Component\HttpFoundation\JsonResponse;
use Symfony\Component\HttpFoundation\Response;
use Webmozart\Assert\Assert;

final class TaxonPositionController
{
public function __construct(
private TaxonRepositoryInterface $taxonRepository,
private TaxonPositionerInterface $taxonPositioner,
private ObjectManager $taxonManager,
) {
}

public function moveUpAction(int $id): Response
{
/** @var TaxonInterface|null $taxonToBeMoved */
jakubtobiasz marked this conversation as resolved.
Show resolved Hide resolved
$taxonToBeMoved = $this->taxonRepository->find($id);
jakubtobiasz marked this conversation as resolved.
Show resolved Hide resolved
Assert::notNull($taxonToBeMoved);

$this->taxonPositioner->moveUp($taxonToBeMoved);
$this->taxonManager->flush();

return new JsonResponse();
jakubtobiasz marked this conversation as resolved.
Show resolved Hide resolved
}

public function moveDownAction(int $id): Response
{
/** @var TaxonInterface|null $taxonToBeMoved */
jakubtobiasz marked this conversation as resolved.
Show resolved Hide resolved
$taxonToBeMoved = $this->taxonRepository->find($id);
Assert::notNull($taxonToBeMoved);

$this->taxonPositioner->moveDown($taxonToBeMoved);
$this->taxonManager->flush();

return new JsonResponse();
}
}
40 changes: 32 additions & 8 deletions src/Sylius/Bundle/TaxonomyBundle/Doctrine/ORM/TaxonRepository.php
Original file line number Diff line number Diff line change
@@ -169,29 +169,53 @@ protected function createTranslationBasedQueryBuilder(?string $locale): QueryBui

public function findOneAbove(TaxonInterface $taxon): ?TaxonInterface
jakubtobiasz marked this conversation as resolved.
Show resolved Hide resolved
{
return $this->createQueryBuilder('o')
->andWhere('o.parent = :parent')
->setParameter('parent', $taxon->getParent())
$query = $this->createQueryBuilder('o');
$taxonParent = $taxon->getParent();

if (null !== $taxonParent) {
$query
->andWhere('o.parent = :parent')
->setParameter('parent', $taxonParent)
;
} else {
$query
->andWhere('o.parent IS NULL')
;
}

return $query
->andWhere('o.position < :taxonPosition')
->setParameter('taxonPosition', $taxon->getPosition())
->addOrderBy('o.position', 'DESC')
->setMaxResults(1)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it necessary in presence of getOneOrNullResult? Which will throw exception like so

        if (count($result) > 1) {
            throw new NonUniqueResultException();
        }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to eliminate a need to handle the exception.

->getQuery()
->getOneOrNullResult()
;
;
}

public function findOneBelow(TaxonInterface $taxon): ?TaxonInterface
jakubtobiasz marked this conversation as resolved.
Show resolved Hide resolved
{
return $this->createQueryBuilder('o')
->andWhere('o.parent = :parent')
->setParameter('parent', $taxon->getParent())
$query = $this->createQueryBuilder('o');
$taxonParent = $taxon->getParent();

if (null !== $taxonParent) {
$query
->andWhere('o.parent = :parent')
->setParameter('parent', $taxonParent)
;
} else {
$query
->andWhere('o.parent IS NULL')
;
}

return $query
->andWhere('o.position > :taxonPosition')
->setParameter('taxonPosition', $taxon->getPosition())
->addOrderBy('o.position', 'ASC')
->setMaxResults(1)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to eliminate a need to handle the exception.

->getQuery()
->getOneOrNullResult()
;
;
}
}
Original file line number Diff line number Diff line change
@@ -26,8 +26,10 @@
<argument type="service" id="sylius.factory.taxon" />
</service>

<service id="sylius.controller.taxon_move" class="Sylius\Bundle\TaxonomyBundle\Controller\TaxonMoveController">
<service id="sylius.controller.taxon_position" class="Sylius\Bundle\TaxonomyBundle\Controller\TaxonPositionController">
<argument type="service" id="sylius.repository.taxon" />
<argument type="service" id="Sylius\Component\Taxonomy\Positioner\TaxonPositionerInterface" />
<argument type="service" id="doctrine.orm.entity_manager" />
jakubtobiasz marked this conversation as resolved.
Show resolved Hide resolved
</service>

<service id="sylius.custom_factory.taxon" class="Sylius\Component\Taxonomy\Factory\TaxonFactory" decorates="sylius.factory.taxon" decoration-priority="256" public="false">
@@ -37,5 +39,9 @@

<service id="sylius.generator.taxon_slug" class="Sylius\Component\Taxonomy\Generator\TaxonSlugGenerator" />
<service id="Sylius\Component\Taxonomy\Generator\TaxonSlugGeneratorInterface" alias="sylius.generator.taxon_slug" />

<service id="Sylius\Component\Taxonomy\Positioner\TaxonPositionerInterface" class="Sylius\Component\Taxonomy\Positioner\TaxonPositioner">
<argument type="service" id="sylius.repository.taxon" />
</service>
</services>
</container>
Original file line number Diff line number Diff line change
@@ -515,8 +515,6 @@ sylius:
most_expensive_first: 'Most expensive first'
move_down: 'Move down'
move_up: 'Move up'
move_top: 'Move to top'
move_end: 'Move to end'
my_account: 'My account'
n_a: 'N/A'
name: 'Name'
Original file line number Diff line number Diff line change
@@ -496,8 +496,6 @@ sylius:
most_expensive_first: 'Найдорожчий перший'
move_down: 'Перемістити нижче'
move_up: 'Перемістити вище'
move_top: 'Перемістити на початок'
move_end: 'Перемістити в кінець'
my_account: 'Мій обліковий запис'
n_a: 'Н/Д'
name: 'Назва регіону'
52 changes: 52 additions & 0 deletions src/Sylius/Component/Taxonomy/Positioner/TaxonPositioner.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
<?php

/*
* This file is part of the Sylius package.
*
* (c) Sylius Sp. z o.o.
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

declare(strict_types=1);

namespace Sylius\Component\Taxonomy\Positioner;

use Sylius\Component\Taxonomy\Model\TaxonInterface;
use Sylius\Component\Taxonomy\Repository\TaxonRepositoryInterface;

final class TaxonPositioner implements TaxonPositionerInterface
{
public function __construct (
private TaxonRepositoryInterface $taxonRepository,
) {
}

public function moveUp(TaxonInterface $taxon): void
{
$taxonAbove = $this->taxonRepository->findOneAbove($taxon);
jakubtobiasz marked this conversation as resolved.
Show resolved Hide resolved

if (null !== $taxonAbove) {
$this->swapTaxonPositions($taxon, $taxonAbove);
}
}

public function moveDown(TaxonInterface $taxon): void
{
$taxonBelow = $this->taxonRepository->findOneBelow($taxon);

if (null !== $taxonBelow) {
$this->swapTaxonPositions($taxon, $taxonBelow);
}
}

private function swapTaxonPositions(TaxonInterface $firstTaxon, TaxonInterface $secondTaxon): void
{
$firstTaxonPosition = $firstTaxon->getPosition();
$secondTaxonPosition = $secondTaxon->getPosition();

$firstTaxon->setPosition($secondTaxonPosition);
$secondTaxon->setPosition($firstTaxonPosition);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
<?php

/*
* This file is part of the Sylius package.
*
* (c) Sylius Sp. z o.o.
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

declare(strict_types=1);

namespace Sylius\Component\Taxonomy\Positioner;

use Sylius\Component\Taxonomy\Model\TaxonInterface;

interface TaxonPositionerInterface
{
public function moveUp(TaxonInterface $taxon): void;

public function moveDown(TaxonInterface $taxon): void;
}
Loading
Oops, something went wrong.