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

Migration of "International > Localization > Currencies" page listing #10774

Merged
merged 38 commits into from Jan 4, 2019

Conversation

Projects
None yet
9 participants
@tomas862
Copy link
Member

tomas862 commented Sep 29, 2018

Questions Answers
Branch? develop
Description? Migration of of "International > Localization > Currencies" page list only. Currently page can be reached via admin-dev/index.php/improve/international/currencies/
Type? new feature
Category? BO
BC breaks? no
Deprecations? no
Fixed ticket? #10189
How to test? Should work exactly as legacy Currency page list except that Add currency and edit will point to legacy form and when submitting legacy form you will be redirected to the old page.

This change is Reviewable

@tomas862 tomas862 changed the title [WIP ] Migration of "International > Localization > Currencies" page [WIP ] Migration of "International > Localization > Currencies" page listing Oct 4, 2018

@tomas862 tomas862 changed the title [WIP ] Migration of "International > Localization > Currencies" page listing Migration of "International > Localization > Currencies" page listing Oct 4, 2018

@sarjon
Copy link
Member

sarjon left a comment

few minor comments, but pretty good job 👍

private $defaultCurrencyId;
/**
* ToggleCurrencyStatusHandler constructor.

This comment has been minimized.

@sarjon

sarjon Oct 6, 2018

Member

im not sure this brings any value. 🤔

}
if ($command->getCurrencyId()->getValue() === $this->defaultCurrencyId) {
throw new CannotDeleteCurrencyException(

This comment has been minimized.

@sarjon

sarjon Oct 6, 2018

Member

how about DefaultCurrencyCannotBeDeletedException? @matks wdyt?

}
if ($entity->active && $command->getCurrencyId()->getValue() === $this->defaultCurrencyId) {
throw new CannotToggleCurrencyException(

This comment has been minimized.

@sarjon

sarjon Oct 6, 2018

Member

how about CannotDisableDefaultCurrencyException?

use PrestaShop\PrestaShop\Core\Domain\Currency\ValueObject\CurrencyId;
/**
* Class DeleteCurrencyCommand is responsible for providing data required for DeleteCurrencyCommandHandler.

This comment has been minimized.

@sarjon

sarjon Oct 6, 2018

Member

you should document what command actually does, it should be: Class DeleteCurrencyCommand deletes given currency. or something similar.

*/
private $contextShopIds;
public function __construct(

This comment has been minimized.

@sarjon

sarjon Oct 6, 2018

Member

docblock is missing.


_currency:
resource: "currency.yml"
prefix: /currency/

This comment has been minimized.

@sarjon

sarjon Oct 6, 2018

Member
- prefix: /currency/
+ prefix: /currencies/
@@ -0,0 +1,46 @@
admin_currency_index:

This comment has been minimized.

@sarjon

sarjon Oct 6, 2018

Member

update all routes to use currencies

methods: POST
defaults:
_controller: 'PrestaShopBundle:Admin\Improve\International\Currency:search'
_legacy_controller: AdminMeta

This comment has been minimized.

@sarjon

sarjon Oct 6, 2018

Member

AdminMeta 😕 ? check other routes as well.

currencyId: \d+

admin_currency_toggle_status:
path: /{currencyId}/toggle/status

This comment has been minimized.

@sarjon

sarjon Oct 6, 2018

Member
- path: /{currencyId}/toggle/status
+ path: /{currencyId}/toggle-status

admin_currency_delete:
path: /{currencyId}/delete
defaults:

This comment has been minimized.

@sarjon

sarjon Oct 6, 2018

Member

what methods does it accept? 🤔

@tomas862 tomas862 force-pushed the tomas862:migration/international/localization/currencies_page branch from 1bfc79e to 6fb3492 Oct 7, 2018

@tomas862

This comment has been minimized.

Copy link
Member Author

tomas862 commented Oct 7, 2018

Thanks @sarjon for the review!

Show resolved Hide resolved src/Core/Domain/Currency/Command/DeleteCurrencyCommand.php
])
->setAssociatedColumn('iso_code')
)
->add((new Filter('active', ChoiceType::class)) //todo: change to YesNoChoiceType::class when available

This comment has been minimized.

@matks

matks Oct 12, 2018

Contributor

Do you plan to add the YesNoChoiceType in another PR ?

This comment has been minimized.

@sarjon

sarjon Oct 12, 2018

Member

it already there, you can use it @tomas862

This comment has been minimized.

@tomas862

tomas862 Oct 13, 2018

Author Member

done 👍

}
/**
* Gets query builder with the common sql used for displaying webservice list and applying filter actions.

This comment has been minimized.

@matks

matks Oct 12, 2018

Contributor

I dont believe you 😄

This comment has been minimized.

@tomas862

tomas862 Oct 13, 2018

Author Member

changed to better class description 😄

$qb->setParameter('shops', $this->contextShopIds, Connection::PARAM_INT_ARRAY);
foreach ($filters as $filterName => $value) {
if (!in_array($filterName, $allowedFilters, true)) {

This comment has been minimized.

@matks

matks Oct 12, 2018

Contributor

👍

try {
$currencyId = new CurrencyId($currencyId);
$this->getCommandBus()->handle(new DeleteCurrencyCommand($currencyId));
} catch (CurrencyException $exception) {

This comment has been minimized.

@matks

matks Oct 12, 2018

Contributor

What happens if another Exception is raised, but not a CurrencyException ? Fatal error ? Then what happens in the browser ? If this causes a JS error that crashes the JS in browser, we need to handle that.

This comment has been minimized.

@tomas862

tomas862 Oct 13, 2018

Author Member

Currently , the CurrencyException handles also PrestaShopException which has a lot of scenarios covered when working with legacy ObjectModels inside the command. It also covers CurrencyNotFoundException, CannotDeleteCurrencyException, CannotDeleteDefaultCurrencyException etc...

I'm not sure I understand the problem, but do you suggest to handle exceptions like this:

        try {
            $currencyId = new CurrencyId($currencyId);
            $this->getCommandBus()->handle(new DeleteCurrencyCommand($currencyId));
        } catch (Exception $exception) {}

the main difference is that now I can handle every exception and display beautiful non informative error to regular user if an unexpected error occurred. But it might be harder for others to search and identify the problem 🤔

This comment has been minimized.

@matks

matks Oct 15, 2018

Contributor

We have had issues on some SF pages where all exceptions were not handled. The usecase was the following:

  • SF controller was being called by a javascript AJAX call
  • call was expected to return JSON
  • something went wrong, so an Exception was raised that was not catched in the controller
  • so a blank page was returned
  • but the Javascript expected a JSON return, so it crashed because it tried to parse the blank page which is "not valid JSON format" 🤣
  • so all the JS in the page would crash

So if this controller is being called by an AJAX call that expects JSON, we need to

  • either make sure the Javascript does not crash if the return is not JSON-compliant
  • or make sure it always return a valid JSON response (see below)

Example for the 2nd solution:

try {
            $currencyId = new CurrencyId($currencyId);
            $this->getCommandBus()->handle(new DeleteCurrencyCommand($currencyId));
} catch (Exception $exception) {
    return json_encode(['error' => true]);
}

This comment has been minimized.

@tomas862

tomas862 Oct 18, 2018

Author Member

In your case if that Ajax request is very depending on another it might be true to do that but let me define my scenario:

  • Its simple DELETE request and the CurrencyException covers a lot of cases since inside it handles the PrestaShopException etc...

  • If an unexpected Exception is being raised, then something terrible happened and the customer must know about it.

Lets follow a simple workflow:

Customer want to delete the currency but hes database just went down for some reason (Can't think of any other sample 😄 )

If I implement in a way you suggest me in that case he would see the error : "Unexpected error occurred."

on the current scenario the page would go down and if he turns on the debug mode he would see the clear error on he's screen that really says that something bad just happened and you need to fix that

If I see the error "Unexpected error occurred." I know I'll have a lot of business to do while analysing the code where it came from to print for my self the Exception message just to know that the database went down 😄

Let me know what you think about the problem I described 👍

This comment has been minimized.

@matks

matks Oct 18, 2018

Contributor

@tomas862 You got me convinced 👍

*
* @return string
*/
private function getErrorByExceptionType(CurrencyException $exception)

This comment has been minimized.

@matks

matks Oct 12, 2018

Contributor

I like this approach 👍

This comment has been minimized.

@tomas862

tomas862 Oct 13, 2018

Author Member

copied from @sarjon 😄

@tomas862 tomas862 force-pushed the tomas862:migration/international/localization/currencies_page branch from 6fb3492 to c3cf611 Oct 13, 2018

@matks

This comment has been minimized.

Copy link
Contributor

matks commented Oct 15, 2018

All right 😄 only #10774 (comment) left before approval !

@tomas862 tomas862 force-pushed the tomas862:migration/international/localization/currencies_page branch from 95b7aed to 1cae4ce Oct 18, 2018

@matks

This comment has been minimized.

Copy link
Contributor

matks commented Oct 18, 2018

@tomas862 Travis wants you to apply php-cs-fixer 😄

@tomas862

This comment has been minimized.

Copy link
Member Author

tomas862 commented Oct 18, 2018

done 👍

@marionf marionf self-assigned this Oct 19, 2018

@marionf

This comment has been minimized.

Copy link
Contributor

marionf commented Oct 19, 2018

@tomas862

The icon of the action drop-down should be like this

capture d ecran_473

Instead of this:

capture d ecran_471

The wheel should move like this:

capture d ecran_472

We should add also the arrows allowing to sort asc and desc next to the "Enabled" field

capture d ecran_475

We should do this for all migrated pages

@TristanLDD what do think of the link on the currency name, should we keep it ?

capture d ecran_478

@tomas862 tomas862 force-pushed the tomas862:migration/international/localization/currencies_page branch from 6e9cbcc to 73e5f44 Dec 20, 2018

@tomas862

This comment has been minimized.

Copy link
Member Author

tomas862 commented Dec 20, 2018

one part of the design change is included in the PR #11877

tomas862 added some commits Dec 20, 2018

@tomas862

This comment has been minimized.

Copy link
Member Author

tomas862 commented Dec 20, 2018

ready for review - to compile assets is a must in order to see changes 👍

@tomas862

This comment has been minimized.

Copy link
Member Author

tomas862 commented Dec 20, 2018

in this PR what I extra added is :
input_width
the max-width of this input is now 120px

in another PR #11877 the button float to right functionality is included (Affects all migrated lists)

@matks

matks approved these changes Jan 4, 2019

@matks

This comment has been minimized.

Copy link
Contributor

matks commented Jan 4, 2019

Thank you @tomas862

@matks matks merged commit 0149664 into PrestaShop:develop Jan 4, 2019

1 of 2 checks passed

Codacy/PR Quality Review Not up to standards. This pull request quality could be better.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@tomas862 tomas862 deleted the tomas862:migration/international/localization/currencies_page branch Jan 4, 2019

@PierreRambaud PierreRambaud added this to the 1.7.6.0 milestone Jan 7, 2019

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