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

Voucher issue fix #8213

Merged
merged 1 commit into from Sep 4, 2017

Conversation

Projects
None yet
5 participants
@Prestaworks
Contributor

Prestaworks commented Aug 2, 2017

Questions Answers
Branch? 1.7.2.x
Description? Voucher fix, when voucher value is calculated on order creation, shipping cost is not added if free shipping was used on the voucher and it has remaining amount that is carried over to new voucher. Also, the free shipping settings was always reset.
Type? bug fix
Category? CO
BC breaks? no
Deprecations? no
Fixed ticket? http://forge.prestashop.com/browse/BOOM-3553
How to test? Create a cart rule with, 1000 euro discount and free shipping and partial use allowed.
make an order for less than 1000 euro.

Important guidelines

@vincentbz vincentbz added this to the 1.7.2.2 milestone Aug 7, 2017

@LittleBigDev

Hello @Prestaworks, Thanks for this PR.
I see some little changes that could improve your solution. See below !

Show outdated Hide outdated classes/PaymentModule.php
Show outdated Hide outdated classes/PaymentModule.php
Show outdated Hide outdated classes/PaymentModule.php
@Prestaworks

This comment has been minimized.

Show comment
Hide comment
@Prestaworks

Prestaworks Aug 23, 2017

Contributor

Did the changes get updated now?

Contributor

Prestaworks commented Aug 23, 2017

Did the changes get updated now?

@LittleBigDev

Nice. Code approved.
But I suggest you squash your 4 commits so the history is clean and ready to be merged. We will lose reviews and comments history but whatever; I'll approve again :).

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

@Prestaworks

This comment has been minimized.

Show comment
Hide comment
@Prestaworks

Prestaworks Aug 28, 2017

Contributor

I seem to have some issues with Squashing it :( it just complains about merge issue.

Contributor

Prestaworks commented Aug 28, 2017

I seem to have some issues with Squashing it :( it just complains about merge issue.

@LittleBigDev

This comment has been minimized.

Show comment
Hide comment
@LittleBigDev

LittleBigDev Aug 28, 2017

Contributor

Then you can use a soft git reset to revert your local commits before your changes. Then new commit, then force push.

git reset HEAD~4
git add path/to/file
git commit -m 'Your message'
git push -f
Contributor

LittleBigDev commented Aug 28, 2017

Then you can use a soft git reset to revert your local commits before your changes. Then new commit, then force push.

git reset HEAD~4
git add path/to/file
git commit -m 'Your message'
git push -f
@Prestaworks

This comment has been minimized.

Show comment
Hide comment
@Prestaworks

Prestaworks Aug 28, 2017

Contributor

It dosn't work, it just tries to add like a bunch of css files and complains about the brance.. Maybe it's easier to just close this and start over with a new fork.

Contributor

Prestaworks commented Aug 28, 2017

It dosn't work, it just tries to add like a bunch of css files and complains about the brance.. Maybe it's easier to just close this and start over with a new fork.

@LittleBigDev

This comment has been minimized.

Show comment
Hide comment
@LittleBigDev

LittleBigDev Aug 28, 2017

Contributor

Maybe you should have done git reset HEAD~3. My bad.
If it's easier for you, you can create a new PR and link it here.

Contributor

LittleBigDev commented Aug 28, 2017

Maybe you should have done git reset HEAD~3. My bad.
If it's easier for you, you can create a new PR and link it here.

@Prestaworks

This comment has been minimized.

Show comment
Hide comment
@Prestaworks

Prestaworks Aug 30, 2017

Contributor

Changing the 4 to 3 seems to have solved it

Contributor

Prestaworks commented Aug 30, 2017

Changing the 4 to 3 seems to have solved it

@LittleBigDev

Great !

@marionf marionf added QA ✔️ and removed waiting for QA labels Aug 30, 2017

@eternoendless

This comment has been minimized.

Show comment
Hide comment
@eternoendless
Member

eternoendless commented Sep 4, 2017

Thank you @Prestaworks

@eternoendless eternoendless merged commit 43913da into PrestaShop:1.7.2.x Sep 4, 2017

2 checks passed

codacy/pr Good work! A positive pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@eternoendless eternoendless changed the title from CO: voucher issue fix to Voucher issue fix Sep 28, 2017

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