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

Harmonize migrated controllers #11030

Merged
merged 8 commits into from Oct 17, 2018

Conversation

Projects
None yet
4 participants
@sarjon
Member

sarjon commented Oct 16, 2018

Questions Answers
Branch? 1.7.5.x
Description? Updates controllers that were migrated for 1.7.5 version with new naming conventions.
Type? bug fix
Category? CO
BC breaks? no
Deprecations? no
Fixed ticket? n/a
How to test? Controllers listed below should work as they did before, no functionality as changed.

Updated controllers:

  • EmailController (E-mail page)
  • BackupController (Backups page)
  • MetaController (Traffic & SEO page)
  • LocalizationController (Localization page)
  • GeolocationController (Geolocation page)
  • LogsController (Logs page)

This change is Reviewable

@sarjon

This comment has been minimized.

Member

sarjon commented Oct 16, 2018

@matks here's PR which is kinda related to #11021

sarjon added some commits Oct 16, 2018

@matks matks added the migration label Oct 17, 2018

@matks

matks approved these changes Oct 17, 2018

Perfect 😃 you even added some improvements that make the code simpler

@matks

This comment has been minimized.

Contributor

matks commented Oct 17, 2018

@eternoendless @PierreRambaud I'd like to merge this 1.7.5.x. This is not a bugfix however if we do not merge this for 1.7.5.x then this will require a lot of deprecations / aliases / workarounds to avoid introducing BC breaks.

@marionf

This comment has been minimized.

Contributor

marionf commented Oct 17, 2018

@sarjon

When I try to delete in bulk on the emails page, I have this exception:

capture d ecran_446

I have configured Geolocation to see catalog only from Afghanistan and can see the shop from France, and also in private navigation

capture d ecran_448

capture d ecran_449

With same settings in 1.74.3 it's working:

capture d ecran_450

@sarjon

This comment has been minimized.

Member

sarjon commented Oct 17, 2018

hi @marionf

  1. regarding exception, it is totally not related to this PR
  2. Geolocation rules should work if you try to access actual shop (on remote server). In addition, behavior has changed a little between 1.7.4 and 1.7.5 due to this PR #9441, thats why you can access shop from your localhost.
@marionf

This comment has been minimized.

Contributor

marionf commented Oct 17, 2018

You are right @sarjon, I have these issues also on 1.7.5.x branch
I will create a new issue

@sarjon

This comment has been minimized.

Member

sarjon commented Oct 17, 2018

I will create a new issue

@marionf i dont think that Geolocation has an issue, it just ignores Geolocation rules for localhost, it should work well on remote server. :)

@matks

This comment has been minimized.

Contributor

matks commented Oct 17, 2018

@PierreRambaud approved so let's merge !

@matks matks added this to the 1.7.5.0 milestone Oct 17, 2018

@matks matks merged commit a68462a into PrestaShop:1.7.5.x Oct 17, 2018

2 checks passed

Codacy/PR Quality Review Up to standards. A positive pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment