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

Migrate Documents block of Order View page #16046

Merged
merged 13 commits into from Nov 7, 2019

Conversation

@sarjon
Copy link
Contributor

sarjon commented Oct 21, 2019

Questions Answers
Branch? develop
Description?
Type? improvement
Category? BO
BC breaks? no
Deprecations? no
Fixed ticket? #15823
How to test? ⚠️ Rebuild assets ⚠️ Documents block should behave the same as it was in legacy.

This change is Reviewable

@sarjon sarjon requested a review from PrestaShop/prestashop-core-developers as a code owner Oct 21, 2019
@matks matks added this to the 1.7.7.0 milestone Oct 21, 2019
@sarjon sarjon force-pushed the sarjon:m/orders/documents branch from 64fafa1 to e254f13 Oct 25, 2019
@sarjon

This comment has been minimized.

Copy link
Contributor Author

sarjon commented Oct 25, 2019

Rebased.

@matks

This comment has been minimized.

Copy link
Contributor

matks commented Oct 29, 2019

1 conflict fixed, be careful when you git pull

@Robin-Fischer-PS

This comment has been minimized.

Copy link

Robin-Fischer-PS commented Oct 29, 2019

Hi @sarjon ! Thanks for the PR 😄

I've noticed two bugs :

  • When there is no document on an order, there should be a "generate invoice" button in document block.
    Here in legacy :
    PR16046 generate invoice legacy

  • When there is no payment associated with an invoice, there should be a "Enter payment" button.
    Here in legacy :
    PR16046 enter payment legacy

@sarjon

This comment has been minimized.

Copy link
Contributor Author

sarjon commented Oct 30, 2019

When there is no payment associated with an invoice, there should be a "Enter payment" button.
Here in legacy :

Regarding this one, do we really need it? In legacy, Payments panel were way below Documents panel, but in new page (thus new layout), Payments panel is just below Documents, so does it make sense to have this button and scroll user a few pixels below?

@matks wdyt? Maybe we should ask Product team as well? 🤔

@matks matks dismissed their stale review via e1105e3 Nov 6, 2019
@matks matks force-pushed the sarjon:m/orders/documents branch from 302fbcc to e1105e3 Nov 6, 2019
@MatShir

This comment has been minimized.

Copy link

MatShir commented Nov 6, 2019

When there is no payment associated with an invoice, there should be a "Enter payment" button.
Here in legacy :

Regarding this one, do we really need it? In legacy, Payments panel were way below Documents panel, but in new page (thus new layout), Payments panel is just below Documents, so does it make sense to have this button and scroll user a few pixels below?

@matks wdyt? Maybe we should ask Product team as well?

Seen with Tristan, we should keep the same behaviours which are to scroll down and add the invoice amount in the amount input of the payment line with the right invoice.

@matks matks removed the waiting for PM label Nov 6, 2019
@matks

This comment has been minimized.

Copy link
Contributor

matks commented Nov 6, 2019

@Robin-Fischer-PS @MatShir The 2 items reported by Robin have been fixed. Dont forget to rebuild the assets. Please check his PR again 😊

@Robin-Fischer-PS

This comment has been minimized.

Copy link

Robin-Fischer-PS commented Nov 7, 2019

Hi @matks ! It's QA ✔️ !

@matks matks dismissed stale reviews from PierreRambaud and themself via 7680db5 Nov 7, 2019
@matks
matks approved these changes Nov 7, 2019
@matks

This comment has been minimized.

Copy link
Contributor

matks commented Nov 7, 2019

Thank you @sarjon

@matks matks merged commit 549eae1 into PrestaShop:develop Nov 7, 2019
2 checks passed
2 checks passed
PrettyCI Code formatting
Details
Travis CI - Pull Request Build Passed
Details
@sarjon sarjon deleted the sarjon:m/orders/documents branch Nov 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.