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.

Sign up for GitHub

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

Jump to bottom

Fix "jumping" taxons while moving up or down #15272

Merged
merged 7 commits into from
Sep 14, 2023
Merged

Fix "jumping" taxons while moving up or down #15272

merged 7 commits into from
Sep 14, 2023

Conversation

jakubtobiasz
Copy link
Contributor

@jakubtobiasz jakubtobiasz commented Sep 5, 2023 •
edited
Loading

Q A
Branch? 1.12
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Related tickets fixes #9909, based on #11905
License MIT

Basing on #11905 I removed the "feature" part, to merge the "bug-fixing" part into 1.12. We'll consider later adding "Move top" and "Move end" features, but probably it'll be considered in terms of 2.0 with the new Admin Panel.

Thanks @igormukhingmailcom for your work! I'm happy we're finally going to merge your commits :).

Previously:
CleanShot 2023-09-05 at 15 37 49

Now:
CleanShot 2023-09-05 at 15 39 33

@jakubtobiasz jakubtobiasz added Admin AdminBundle related issues and PRs. Bug Confirmed bugs or bugfixes. labels Sep 5, 2023
@jakubtobiasz jakubtobiasz requested a review from a team as a code owner September 5, 2023 13:37
@github-actions
Copy link

github-actions bot commented Sep 5, 2023 •
edited
Loading

Bunnyshell Preview Environment deleted

Available commands:

  • /bns:deploy to redeploy the environment

@jakubtobiasz jakubtobiasz changed the base branch from 1.13 to 1.12 September 5, 2023 13:51
@jakubtobiasz jakubtobiasz changed the title 🚧 Fix "jumping" taxons while moving up or down Fix "jumping" taxons while moving up or down Sep 6, 2023
SzymonFilipek
SzymonFilipek previously approved these changes Sep 6, 2023
View reviewed changes
Copy link

@SzymonFilipek SzymonFilipek left a comment

Choose a reason for hiding this comment

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

Changes passed QA check

@igormukhingmailcom
Copy link
Contributor

Thanks for the fixes, @jakubtobiasz :)

@ehibes
Copy link
Contributor

ehibes commented Sep 7, 2023 •
edited
Loading

Hi guys,
Why not use nested set behavior for taxons position ? It’s already implemented on database... otherwise I don't see what use it would be.
#7585 this issue is about it too.

@jakubtobiasz
Copy link
Contributor Author

Hi @ehibes 👋🏼,
thanks for sharing your thoughts. This PR aims to fix the existing behavior, the issue you share might be considered in a longer term (e.g. Sylius 2.0 or 3.0 as it might require analysis + refactoring and changes in the behavior).

@jakubtobiasz
Copy link
Contributor Author

Thanks for leaving the review @igormukhingmailcom 😅

diimpp
diimpp reviewed Sep 12, 2023
View reviewed changes
src/Sylius/Bundle/TaxonomyBundle/Doctrine/ORM/TaxonRepository.php Outdated
->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.

diimpp
diimpp reviewed Sep 12, 2023
View reviewed changes
src/Sylius/Bundle/TaxonomyBundle/Doctrine/ORM/TaxonRepository.php Outdated
->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.

diimpp
diimpp requested changes Sep 12, 2023
View reviewed changes
Copy link
Member

@diimpp diimpp left a comment

Choose a reason for hiding this comment

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

New repository methods are unused anywhere in the codebase

@igormukhingmailcom
Copy link
Contributor

Is this works?

$taxonToBeMoved->setPosition($taxonToBeMoved->getPosition() - 1);

It just doing the same -1 as we had before (when things did not work), but on backend side rather than on front-end side - that is all the difference. Did someone tested this by hands?

@jakubtobiasz
Copy link
Contributor Author

@igormukhingmailcom
CleanShot 2023-09-13 at 14 41 10
There's an if statement preventing moving the taxon to -1 positions. Behat tests are passing that way, and we tested it yesterday.

@probot-autolabeler probot-autolabeler bot added the Maintenance CI configurations, READMEs, releases, etc. label Sep 13, 2023
GSadee
GSadee approved these changes Sep 14, 2023
View reviewed changes
features/taxonomy/managing_taxons/reordering_taxon.feature
When I want to see all taxons in store
And I move up "T-Shirts" taxon
Then I should see 4 taxons on the list
And the first taxon on the list should be "T-Shirts"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
And the first taxon on the list should be "T-Shirts"
And the first taxon on the list should still be "T-Shirts"

This would describe the behaviour even better, the same in the below scenario

@GSadee GSadee merged commit c324c2a into Sylius:1.12 Sep 14, 2023
@GSadee
Copy link
Member

GSadee commented Sep 14, 2023

Thank you, Jacob! 🥇

@jakubtobiasz jakubtobiasz mentioned this pull request Dec 7, 2023
Closed
2 tasks
@jakubtobiasz jakubtobiasz deleted the SYL-2657 branch December 7, 2023 15:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@igormukhingmailcom igormukhingmailcom igormukhingmailcom left review comments

@GSadee GSadee GSadee approved these changes

@diimpp diimpp diimpp approved these changes

@SzymonFilipek SzymonFilipek SzymonFilipek left review comments

Assignees
No one assigned
Labels
Admin AdminBundle related issues and PRs. Bug Confirmed bugs or bugfixes. Maintenance CI configurations, READMEs, releases, etc.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Updating taxon position not work properly
6 participants
@jakubtobiasz @igormukhingmailcom @ehibes @GSadee @diimpp @SzymonFilipek