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 Customer Service view #14288

Merged

Conversation

@sarjon
Copy link
Contributor

sarjon commented Jun 19, 2019

Questions Answers
Branch? develop
Description? Migrates "View" action of Customer Service
Type? refacto
Category? BO
BC breaks? no
Deprecations? no
Fixed ticket? #10553
How to test? ⚠️ Build assets before testing ⚠️ Create at least 1 customer service and access new page with url admin-dev/index.php/sell/customer-service/customer-threads/{ID}/view where {ID} is Customer Service ID.

This change is Reviewable

@sarjon sarjon requested a review from PrestaShop/prestashop-core-developers as a code owner Jun 19, 2019
@matks matks added the migration label Jun 24, 2019
@sarjon sarjon changed the title [WIP] Migrate Customer Service view Migrate Customer Service view Jun 27, 2019
@sarjon

This comment has been minimized.

Copy link
Contributor Author

sarjon commented Jun 27, 2019

@matks 1 todo is still left, but this PR is ready for review.

@matks matks mentioned this pull request Jul 4, 2019
1 of 30 tasks complete
@sarjon sarjon force-pushed the sarjon:m/customer-service/commands branch from 376f14c to 046e5f7 Jul 15, 2019
@sarjon

This comment has been minimized.

Copy link
Contributor Author

sarjon commented Jul 23, 2019

@matks this is weird. In test environment context language were set to PrestaShopBundle\Install\Language which breaks API. It is suppose to be legacy Language. 😕

Check 56dbe72 wdyt? Before this fix, all of my tests where failing. I could keep this fix or instead of injecting Language into PrestaShop\PrestaShop\Adapter\Twig\LocaleExtension I could inject only a property of it.

@sarjon

This comment has been minimized.

Copy link
Contributor Author

sarjon commented Jul 23, 2019

One test is still failing because of wrong language object in context. I'll take safe fix here and inject property instead of object into my service.

@sarjon sarjon force-pushed the sarjon:m/customer-service/commands branch from 5bccbda to a2a6a9c Jul 25, 2019
@sarjon

This comment has been minimized.

Copy link
Contributor Author

sarjon commented Jul 25, 2019

@matks addressed comments.

@matks

This comment has been minimized.

Copy link
Contributor

matks commented Aug 20, 2019

@sarjon I checked, everything is for me however there are

@sarjon

This comment has been minimized.

Copy link
Contributor Author

sarjon commented Aug 21, 2019

Damn, missed those, will check it out. :)

@sarjon

This comment has been minimized.

Copy link
Contributor Author

sarjon commented Aug 21, 2019

@matks comments addressed.

@sarjon

This comment has been minimized.

Copy link
Contributor Author

sarjon commented Aug 21, 2019

@matks can you rerun travis...?

@matks

This comment has been minimized.

Copy link
Contributor

matks commented Aug 21, 2019

@matks can you rerun travis...?

I'm going to kill it rather #15161

@matks

This comment has been minimized.

Copy link
Contributor

matks commented Aug 26, 2019

@LouiseBonnard Code is OK for me, Wording awaits your validation @LouiseBonnard 😉

Your changes have been done in 3bacb94

@jolelievre jolelievre force-pushed the sarjon:m/customer-service/commands branch from 0ab4c3f to dd02a41 Nov 14, 2019
@jolelievre

This comment has been minimized.

Copy link
Contributor

jolelievre commented Nov 14, 2019

Hi @Robin-Fischer-PS

So I fixed these bugs:

  • download attachment link
  • the \n when forwarding a message
  • the useless button Reply to the next unanswered message in this thread was removed

However about your last issue, the product associated is always the same as in the first message, this is inherent to message threads which can only be associated to one product It already workde this way in legacy so it's not a regression
This could be improved however it would require to change the behaviour in the front office as well in the Customer order page

@PierreRambaud PierreRambaud added this to the 1.7.7.0 milestone Nov 14, 2019
@Robin-Fischer-PS

This comment has been minimized.

Copy link

Robin-Fischer-PS commented Nov 15, 2019

Hi @jolelievre ,

Weird, I still have two of these bugs :

  • download attachment link
  • the \n when forwarding a message

See screenshot :
PR14288 round 2 Migrated

Curiously, now the "\n" bug also appears in Legacy :
PR14288 round 2 New Legacy

On an 1.7.7 without this PR, we have this display :
PR14288 round 2 Old Legacy

Thanks ;)

@Robin-Fischer-PS

This comment has been minimized.

Copy link

Robin-Fischer-PS commented Nov 15, 2019

Thanks @jolelievre ! It works fine now :)

pr14288 proof

@matthieu-rolland matthieu-rolland merged commit 412e013 into PrestaShop:develop Nov 15, 2019
2 checks passed
2 checks passed
PrettyCI Code formatting
Details
Travis CI - Pull Request Build Passed
Details
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.