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 Advanced Parameters -> Import (Step 2) #10120

Merged
merged 91 commits into from Oct 15, 2018

Conversation

@rokaszygmantas
Contributor

rokaszygmantas commented Aug 24, 2018

Questions Answers
Branch? develop
Description? Migration of Import second step (accessed when you click next step in the Import page)
Type? new feature
Category? BO
BC breaks? no
Deprecations? no
Fixed ticket? #10187
How to test? Compare to the legacy Import controller, should work exactly the same.

Additional things to test:

  1. Available fields block in import step 1 (https://prnt.sc/krnni9). The available fields implementation was reworked with this PR so it affected step 1 as well. The fields should refresh when changing the import entity dropdown, also the help boxes should work.
  2. Validation of selected entity fields:
    • When required fields are not selected it should not allow to import (https://prnt.sc/krnpd0). Note: not all entities have required fields, they are marked by asterisks in the dropdowns.
    • When two same fields are selected it should not allow to import (https://prnt.sc/krnokz).
  3. Different buttons are appearing in the import modal window, depending on the import state:
    • Abort button, appearing when import is running. (https://prnt.sc/l0ag46)
    • Ignore warnings button, appearing when there are warnings in the import. Clicking it would continue the import. Information or error messages are not making this button appear, only warning messages do and they are quite rare in import. (https://prnt.sc/l0agli)
    • Close button, simply closes the modal. It should appear whenever the import is not running - for example, when validation errors occurred or when the import finished. (https://prnt.sc/l0ajui)
      Notes:
  • Some translation strings are not being translated, because they didn't have any translation domain in the legacy code and now they were assigned a correct translation domain. These strings are:
    Match your data
    Load a data matching configuration
    Save your data matching configuration
    Rows to skip
    Indicate how many of the first rows of your file should be skipped when importing the data. For instance set it to 1 if the first row of your file contains headers.
    Load
    Please match each column of your source file to one of the destination columns.
    All other texts should be translatable.
  • This PR affects only the data configuration form and import progress modal window, which appears after clicking "Import" button. The import logic is not migrated.

This change is Reviewable

@rokaszygmantas

This comment has been minimized.

Contributor

rokaszygmantas commented Aug 24, 2018

@mickaelandrieu please don't waste your time reviewing it yet, because it's very WIP and a lot might still change :)

{{ form_start(dataConfigurationForm) }}
<div class="card">
<h3 class="card-header">
<i class="material-icons">list</i> {{ ' Match your data'|trans }}

This comment has been minimized.

@LouiseBonnard

LouiseBonnard Aug 24, 2018

Contributor

Watch the space at the beginning of the string. ;-)

@rokaszygmantas

This comment has been minimized.

Contributor

rokaszygmantas commented Sep 7, 2018

@mickaelandrieu the gates are almost open. 😄
the only thing that's not migrated in the import step 2 is this modal: https://prnt.sc/krnli2
it shows the import progress and errors/notices that occurred during import. I can't split it to a separate PR even though I'd like to, because it makes the import work via ajax, so it doesn't timeout on big imports. Now the import works, but it's importing on page load which can timeout on bigger import.

$fields = [
new EntityField('id', $this->trans('ID', 'Admin.Global')),
new EntityField('alias', $this->trans('Alias', 'Admin.Shopparameters.Feature'), '', true),
new EntityField('active', $this->trans('Active (0/1)')),

This comment has been minimized.

@LouiseBonnard

LouiseBonnard Sep 7, 2018

Contributor

No domains or default domains indicated here?

This comment has been minimized.

@rokaszygmantas

rokaszygmantas Sep 7, 2018

Contributor

Default domain for these messages is 'Admin.Advparameters.Feature', it's defined in trans() method in the same class. It's a custom private method, so I've set default domain there. :)

This comment has been minimized.

@LouiseBonnard

LouiseBonnard Sep 10, 2018

Contributor

Okay, thanks @rokaszygmantas. It is defined at line 91, right? I thought it was supposed to be set before the wording concerned, that's why I'm a bit confused here.

This comment has been minimized.

@rokaszygmantas

rokaszygmantas Sep 10, 2018

Contributor

Yes exactly, it's at line 91. Btw, I've just checked these texts in a different language and they are being translated properly :)

This comment has been minimized.

@matks

matks Oct 3, 2018

Contributor

@eternoendless is this behavior expected for TranslationTools 😮 ?

$session->getFlashBag()->add(
'success',
$this->trans(
'Your file has been successfully imported into your shop. Don\'t forget to re - build the products\' search index.',

This comment has been minimized.

@LouiseBonnard

LouiseBonnard Sep 7, 2018

Contributor

Shouldn't it be re-build instead?

This comment has been minimized.

@rokaszygmantas

rokaszygmantas Sep 7, 2018

Contributor

Fixed. Thanks @LouiseBonnard !

new EntityField('supplier', $this->trans('Supplier', 'Admin.Global')),
new EntityField('company', $this->trans('Company', 'Admin.Global')),
new EntityField('lastname', $this->trans('Last name', 'Admin.Global'), '', true),
new EntityField('firstname', $this->trans('First name ', 'Admin.Global'), '', true),

This comment has been minimized.

@LouiseBonnard

LouiseBonnard Sep 10, 2018

Contributor

Watch the blank at the end of the string. ;-)

This comment has been minimized.

@rokaszygmantas

rokaszygmantas Sep 10, 2018

Contributor

It's the same in legacy code: https://github.com/PrestaShop/PrestaShop/blob/develop/controllers/admin/AdminImportController.php#L415 :)
If i'll change it - the translations won't be working for this string.

This comment has been minimized.

@LouiseBonnard

LouiseBonnard Sep 10, 2018

Contributor

Alright, just leave it this way then, thank you so much!

@mickaelandrieu

This comment has been minimized.

Contributor

mickaelandrieu commented Sep 13, 2018

Hello @rokaszygmantas, does it need extra work from you or are you waiting for a review?

@rokaszygmantas

This comment has been minimized.

Contributor

rokaszygmantas commented Sep 13, 2018

Hi @mickaelandrieu, it still needs some work (need to migrate import progress modal) - I plan to finish it during this week.

@mickaelandrieu

This comment has been minimized.

Contributor

mickaelandrieu commented Sep 13, 2018

@rokaszygmantas great 👍 ping me for a review when you consider you work done :)

@matks

This comment has been minimized.

Contributor

matks commented Sep 18, 2018

Should fix #10187

@rokaszygmantas

This comment has been minimized.

Contributor

rokaszygmantas commented Oct 11, 2018

@matks I've done some small fixes, they start with commit 5684ae2. There are quite a few new commits, but the changes are very small so should be easy to review. Also rebased it again.

There are a few unanswered questions left, but I've left my input on all of them, so just waiting for some feedback there. :)

@matks

This comment has been minimized.

Contributor

matks commented Oct 12, 2018

@rokaszygmantas All right ! I reviewed your latest commits, nothing to say 👍
Answered some of your answers, and the other ones I did not answer because the answer was right.

I think we can merge this and start the 2nd PR ?

@rokaszygmantas

This comment has been minimized.

Contributor

rokaszygmantas commented Oct 12, 2018

@matks yeah let's just start fresh with a new PR for improvements, cause this is becoming messy :) thanks for reviews!

@matks

matks approved these changes Oct 15, 2018

@matks matks merged commit 855193f into PrestaShop:develop Oct 15, 2018

1 of 2 checks passed

Codacy/PR Quality Review Codacy was unable to analyse your pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@matks

This comment has been minimized.

Contributor

matks commented Oct 15, 2018

@rokaszygmantas done ;)

@rokaszygmantas rokaszygmantas deleted the rokaszygmantas:migration/import/step-2 branch Oct 26, 2018

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