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

Update bulk action combinations number after delete or add combination #7938

Merged
merged 2 commits into from Sep 25, 2017

Conversation

Projects
None yet
9 participants
@fatmaBouchekoua
Contributor

fatmaBouchekoua commented May 30, 2017

Questions Answers
Branch? 1.7.2.x
Description? If you create some combination then the text of the bulk actions block won't be updated
Type? bug fix
Category? BO
BC breaks? no
Deprecations? no
Fixed ticket? http://forge.prestashop.com/browse/BOOM-1868
How to test? Add or delete combinations, the bulk action total combination number will be updated.
@vincentbz

This comment has been minimized.

Show comment
Hide comment
@vincentbz

vincentbz Jun 6, 2017

Contributor

Tested and ok, thanks !

Contributor

vincentbz commented Jun 6, 2017

Tested and ok, thanks !

@prestonBot prestonBot added the QA ✔️ label Jun 6, 2017

@vincentbz vincentbz added this to the 1.7.2.0 milestone Jun 6, 2017

@maximebiloe maximebiloe modified the milestones: 1.7.2.0, 1.7.2.1 Jun 26, 2017

@fatmaBouchekoua fatmaBouchekoua added the Bug label Jun 30, 2017

@maximebiloe maximebiloe changed the base branch from develop to 1.7.2.x Jul 21, 2017

@maximebiloe maximebiloe modified the milestones: 1.7.2.2, 1.7.2.1 Jul 26, 2017

Show outdated Hide outdated ...Bundle/Resources/views/Admin/Product/Include/form_combinations.html.twig
Show outdated Hide outdated js/tools.js
Show outdated Hide outdated js/tools.js
Show outdated Hide outdated js/tools.js

@eternoendless eternoendless modified the milestones: 1.7.2.3, 1.7.2.2 Aug 25, 2017

Show outdated Hide outdated js/tools.js
Show outdated Hide outdated js/tools.js
Show outdated Hide outdated js/tools.js
* @param {number} sign
* @param {number} number
*/
var refreshTotalCombinations = function (sign, number) {

This comment has been minimized.

@eternoendless

eternoendless Sep 20, 2017

Member

After further analysis, this method should be owned by admin-dev/themes/new-theme/js/product-page/product-bulk-combinations.js, but if we moved it there then it wouldn't be accessible from here.

Ideally, an event should be emitted when adding/removing combinations, and bulkCombinations would listen to it in order to update the number of combinations whenever a combination is added/removed.

However, the current state of things doesn't allow it without refactoring, so unfortunately this will have to do.

@eternoendless

eternoendless Sep 20, 2017

Member

After further analysis, this method should be owned by admin-dev/themes/new-theme/js/product-page/product-bulk-combinations.js, but if we moved it there then it wouldn't be accessible from here.

Ideally, an event should be emitted when adding/removing combinations, and bulkCombinations would listen to it in order to update the number of combinations whenever a combination is added/removed.

However, the current state of things doesn't allow it without refactoring, so unfortunately this will have to do.

@Quetzacoalt91

This comment has been minimized.

Show comment
Hide comment
@Quetzacoalt91

Quetzacoalt91 Sep 21, 2017

Member

@fatmaBouchekoua, can you please rebase your PR?

Member

Quetzacoalt91 commented Sep 21, 2017

@fatmaBouchekoua, can you please rebase your PR?

@codacy-bot

This comment has been minimized.

Show comment
Hide comment
@codacy-bot

codacy-bot Sep 22, 2017

Codacy Here is an overview of what got changed by this pull request:

Issues
======
- Added 2
           

See the complete overview on Codacy

codacy-bot commented Sep 22, 2017

Codacy Here is an overview of what got changed by this pull request:

Issues
======
- Added 2
           

See the complete overview on Codacy

@fatmaBouchekoua

This comment has been minimized.

Show comment
Hide comment
@fatmaBouchekoua

fatmaBouchekoua Sep 22, 2017

Contributor

Hello @Quetzacoalt91 ,
Done!

Contributor

fatmaBouchekoua commented Sep 22, 2017

Hello @Quetzacoalt91 ,
Done!

@Quetzacoalt91 Quetzacoalt91 merged commit d40f049 into PrestaShop:1.7.2.x Sep 25, 2017

1 of 2 checks passed

codacy/pr Not so good... This pull request quality could be better.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Quetzacoalt91

This comment has been minimized.

Show comment
Hide comment
@Quetzacoalt91
Member

Quetzacoalt91 commented Sep 25, 2017

Thank you @fatmaBouchekoua

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