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 Languages adding/editing #11885

Merged
merged 42 commits into from Jan 20, 2019

Conversation

@sarjon
Copy link
Member

sarjon commented Dec 20, 2018

Questions Answers
Branch? develop
Description? It migrates "Improve > International > Languages" add/edit form functionality.
Type? refacto
Category? CO
BC breaks? no
Deprecations? no
Fixed ticket? #10580
How to test? Access new pages from admin-dev/index.php/improve/international/languages. Add/Edit pages should work the same as in legacy. NOTE: assets have to be rebuilt before testing.

This change is Reviewable

@sarjon

This comment has been minimized.

Copy link
Member Author

sarjon commented Dec 21, 2018

@matks from the AdminLanguagesController i see that there is some functionality to show missing files for language, however, i cannot get it to work, what do we do with it? 🤔

@matks matks added the migration label Jan 2, 2019

Show resolved Hide resolved classes/Language.php
@matks

This comment has been minimized.

Copy link
Contributor

matks commented Jan 7, 2019

@sarjon Looks like this file has been changed although it is not in the scope of this PR:
https://github.com/PrestaShop/PrestaShop/pull/11885/files#diff-aff0061d78ecbafd89e5bac2f052fcfc

mails/en/account.html → mails/en/nomoreaccount.html

@matks

This comment has been minimized.

Copy link
Contributor

matks commented Jan 7, 2019

@sarjon Nothing to change, but I've got a few questions. See above 😉

@sarjon

This comment has been minimized.

Copy link
Member Author

sarjon commented Jan 7, 2019

@sarjon Looks like this file has been changed although it is not in the scope of this PR:

good catch, ill revert it.

@matks

This comment has been minimized.

Copy link
Contributor

matks commented Jan 7, 2019

@matks from the AdminLanguagesController i see that there is some functionality to show missing files for language, however, i cannot get it to work, what do we do with it?

@sarjon I looked at AdminLanguagesController but did not see it. Can you show us ?

@sarjon

This comment has been minimized.

Copy link
Member Author

sarjon commented Jan 7, 2019

@matks this is the place, as far as i can remember, i couldnt get this if to pass and if i forced it, couldnt see any results.

https://github.com/PrestaShop/PrestaShop/blob/develop/controllers/admin/AdminLanguagesController.php#L269

@matks

This comment has been minimized.

Copy link
Contributor

matks commented Jan 8, 2019

@sarjon After digging in, I think this is a feature unavailable since 1.5.0 because the code executed involves https://github.com/PrestaShop/PrestaShop/blob/develop/classes/Language.php#L275. In this array of files, only 1 still remains on develop branch and it is deprecated since 1.5.0 (https://github.com/PrestaShop/PrestaShop/blob/develop/admin-dev/pdf.php#L33).

So ... you can forget about it 😄

@sarjon

This comment has been minimized.

Copy link
Member Author

sarjon commented Jan 8, 2019

So ... you can forget about it

thats what i thought. 😄

@sarjon

This comment has been minimized.

Copy link
Member Author

sarjon commented Jan 14, 2019

@matks rebased.

@marionf marionf self-assigned this Jan 14, 2019

@marionf

This comment has been minimized.

Copy link
Contributor

marionf commented Jan 14, 2019

Hello @sarjon

  1. The "no image" field shouldn't be required

capture d ecran_901

capture d ecran_900

  1. The error message isn't the same as before when I try to enter an ISO code already existing

capture d ecran_909

capture d ecran_908

  1. In RTL language, the expand/collapse don't work

capture d ecran_905

capture d ecran_904

  1. In the "all shop" drop down, I should only have the "all shop" context and not each shops and groups

capture d ecran_907

capture d ecran_906

  1. If there are many errors, only one is displayed

capture d ecran_911

capture d ecran_910

@sarjon

This comment has been minimized.

Copy link
Member Author

sarjon commented Jan 15, 2019

@marionf regarding "The "no image" field shouldn't be required" is it really okay to create Language without "No picture" image? I know it is possible, but i think it could be a bug, since there is validation in code that checks for both "Flag" and "No picture".

There's even an error message that says: "Flag and "No picture" image fields are required." However, this validation seems to be broken so im wondering whether its ok or not? 🤔

@marionf

This comment has been minimized.

Copy link
Contributor

marionf commented Jan 15, 2019

All right @sarjon it's ok to have the "no image" field required 👍

@matks matks dismissed their stale review via 8cf8b53 Jan 19, 2019

@matks matks force-pushed the sarjon:m/languages/add-edit branch from 4e3ca9a to 8cf8b53 Jan 19, 2019

@matks matks removed the waiting for QA label Jan 19, 2019

@matks

This comment has been minimized.

Copy link
Contributor

matks commented Jan 19, 2019

Rebased. I removed label "waiting for QA" after checking all previous QA feedbacks: they are all handled.

@matks

This comment has been minimized.

Copy link
Contributor

matks commented Jan 20, 2019

Thanks @sarjon

@matks matks merged commit 474c968 into PrestaShop:develop Jan 20, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

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

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