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

Revert "[Behat] Remove js requirement for bulk delete scenarios" #11708

Merged
merged 2 commits into from Aug 15, 2020
Merged

Revert "[Behat] Remove js requirement for bulk delete scenarios" #11708

merged 2 commits into from Aug 15, 2020

Conversation

GalloisLuca
Copy link
Contributor

This reverts commit 4013e51.

Q A
Branch? 1.7
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Related tickets
License MIT

Related to this issue #11324
Custom bulk action can't work cause of the id #bulk-delete wich has been had in the reverted commit.

@GalloisLuca GalloisLuca requested a review from a team as a code owner August 2, 2020 20:32
@lchrusciel
Copy link
Member

Hey Luca!

thank a lot for this revert. I would like to merge it and publish it with the nearest release. However, the build is red and it seems to not be a random one. What I think you could do, is to rebase your PR with the newest master and check if the build is still red. I assume it may still be red due to missing part of your revert 4013e51#diff-a3ee22317b316e28312b526441fee27fR140-R142.

Nonetheless, let me welcome you to our community. Even if you are reverting part of work :)

@GalloisLuca
Copy link
Contributor Author

@lchrusciel Hey i updated the PR on the new master and add the missing modification (don't know why it didn't there before) but it still red ... i think you made some test on this specific case, so if we wants the custom bulkAction works again, we maybe have to dev something for it ? What do you think about that ?

@lchrusciel
Copy link
Member

Ok, I know what is wrong with your PR. You should add two @javascript tags to features/addressing/managing_zones/deleting_multiple_zones.feature and features/product/managing_product_association_types/deleting_multiple_product_association_types.feature. I hope that will make build green again.

@GalloisLuca
Copy link
Contributor Author

@lchrusciel The build is now green, you were right ! 😄

@probot-autolabeler probot-autolabeler bot added Admin AdminBundle related issues and PRs. Documentation Documentation related issues and PRs - requests, fixes, proposals. Shop ShopBundle related issues and PRs. labels Aug 15, 2020
@lchrusciel lchrusciel changed the base branch from master to 1.7 August 15, 2020 11:57
@lchrusciel
Copy link
Member

Great! I had to mess a little bit with your commits, as it should be opened from 1.7 branch(so you will not have to wait for 1.8 to take benefit from this fix ;)). If you would like to add something on a top of it, please remember to fetch data from the remote first.

@lchrusciel lchrusciel closed this Aug 15, 2020
@lchrusciel lchrusciel reopened this Aug 15, 2020
@lchrusciel lchrusciel merged commit 0d4ef66 into Sylius:1.7 Aug 15, 2020
@lchrusciel
Copy link
Member

Thank you, Luca! 🥇

@GalloisLuca
Copy link
Contributor Author

@lchrusciel You are welcome ! Even if i made a revert ! Sorry about how i updated my fork repo, that's my first "contribution" overall, i didn't know how to update the fork properly 😕 . Anyway, thanks for the merge and your time !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Admin AdminBundle related issues and PRs. Documentation Documentation related issues and PRs - requests, fixes, proposals. Shop ShopBundle related issues and PRs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants