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 Payment block of Order view page #15778

Merged
merged 13 commits into from Oct 14, 2019

Conversation

@sarjon
Copy link
Contributor

sarjon commented Oct 1, 2019

Questions Answers
Branch? develop
Description? Payment block of Order view page should be fully functional.
Type? refacto
Category? BO
BC breaks? no
Deprecations? no
Fixed ticket? Fixes #15827
How to test? ⚠️ Build assets before testing ⚠️ Access new page under admin-dev/index.php/sell/orders/orders/{orderId}/view. "Payment" block (https://prnt.sc/pdf5kl) should be fully functional as it was in legacy.

This change is Reviewable

@sarjon sarjon requested a review from PrestaShop/prestashop-core-developers as a code owner Oct 1, 2019
@LouiseBonnard

This comment has been minimized.

Copy link
Contributor

LouiseBonnard commented Oct 2, 2019

Hello there! Change of wordings are not applied here, do I need to comment where it needs to be modified?

@sarjon

This comment has been minimized.

Copy link
Contributor Author

sarjon commented Oct 2, 2019

Sure @LouiseBonnard, but make sure it's not duplicate of #15761 Maybe I could rebase PR first. :)

@sarjon sarjon force-pushed the sarjon:m/orders/view-payment branch from cec2ce6 to d46fe89 Oct 3, 2019
@sarjon

This comment has been minimized.

Copy link
Contributor Author

sarjon commented Oct 3, 2019

@LouiseBonnard feel free check wording now. :)

@matks review comments addressed.

@LouiseBonnard

This comment has been minimized.

Copy link
Contributor

LouiseBonnard commented Oct 3, 2019

Hi, thanks @sarjon, two comments raised yesterday with @MatShir :

  • please turn 'Card Brand' into 'Card type' at line 81 cf. here

  • also, are the card details necessarily displayed? I think about the check payment, for instance

@sarjon

This comment has been minimized.

Copy link
Contributor Author

sarjon commented Oct 3, 2019

also, are the card details necessarily displayed? I think about the check payment, for instance

What do you mean? User has to click "Detail" button to see payment details.

@LouiseBonnard

This comment has been minimized.

Copy link
Contributor

LouiseBonnard commented Oct 3, 2019

@sarjon, I mean the payment is not always made by card, it can be made by bank wire or with cash at delivery. Yet, the details here only concern card payments with the number, the name, the type of card, etc. So to comply with all payment methods, we should:

  • adapt the wording to cover every payment method
  • or do not display this details line for non-card payments
@sarahdib sarahdib self-assigned this Oct 3, 2019
@sarjon

This comment has been minimized.

Copy link
Contributor Author

sarjon commented Oct 4, 2019

@LouiseBonnard payment details are displayed for every payment regardless of payment method (cash, bank wire, paypal or etc.). See https://prnt.sc/peomqz

@sarjon sarjon added WIP and removed waiting for QA labels Oct 4, 2019
@sarjon

This comment has been minimized.

Copy link
Contributor Author

sarjon commented Oct 4, 2019

@sarahdib I've found one missing feature for this block, so I'm removing "waiting for QA" for now. :)

@sarjon sarjon force-pushed the sarjon:m/orders/view-payment branch from d46fe89 to c4e0ba6 Oct 4, 2019
@sarjon

This comment has been minimized.

Copy link
Contributor Author

sarjon commented Oct 4, 2019

@matks could you review c4e0ba6 commit?

@LouiseBonnard

This comment has been minimized.

Copy link
Contributor

LouiseBonnard commented Oct 4, 2019

@sarjon, thank you, in this case, I'd prefer not to display the details if the payment is not made by card.

cc @MatShir, @TristanLDD

@sarjon

This comment has been minimized.

Copy link
Contributor Author

sarjon commented Oct 4, 2019

@sarjon, thank you, in this case, I'd prefer not to display the details if the payment is not made by card.

I'm not sure if we can know that, unless we can check every field if it has value, but it might have only one value of all available.

@sarjon sarjon added waiting for QA and removed WIP labels Oct 4, 2019
@TristanLDD

This comment has been minimized.

Copy link

TristanLDD commented Oct 4, 2019

I agree with Louise, it would be better not to display the details button when details informations are missing. @sarjon @MatShir Could you discuss it? If we can't integrate this in the 1.7.7 we should keep it it mind for further improvement.

@sarjon sarjon force-pushed the sarjon:m/orders/view-payment branch from 0258295 to 34e1f0c Oct 10, 2019
@matks

This comment has been minimized.

Copy link
Contributor

matks commented Oct 10, 2019

@sarjon, thank you, in this case, I'd prefer not to display the details if the payment is not made by card.

cc @MatShir, @TristanLDD

Issue created here #15893 for this topic

@sarahdib

This comment has been minimized.

Copy link

sarahdib commented Oct 11, 2019

Hello @sarjon

There is a problem with the dropdown in the bill column. If there is no bill the dropdown shouldn't appear
Capture d’écran 2019-10-11 à 09 15 30

In the date field it should be a calendar not a dropdown and arrow
Capture d’écran 2019-10-11 à 09 18 42

@sarjon

This comment has been minimized.

Copy link
Contributor Author

sarjon commented Oct 11, 2019

@sarahdib done! :)

@matks
matks approved these changes Oct 11, 2019
@sarahdib sarahdib added QA ✔️ and removed waiting for QA labels Oct 14, 2019
@matks

This comment has been minimized.

Copy link
Contributor

matks commented Oct 14, 2019

Thank you @sarjon

@matks matks added this to the 1.7.7.0 milestone Oct 14, 2019
@matks matks merged commit 1663429 into PrestaShop:develop Oct 14, 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/view-payment branch Oct 14, 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.