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.

Sign up for GitHub

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

Jump to bottom

Fix adding and deleting language entries from tables #33894

Merged
merged 1 commit into from
Sep 14, 2023
Merged

Fix adding and deleting language entries from tables #33894

merged 1 commit into from
Sep 14, 2023

Conversation

Hlavtox
Copy link
Contributor

@Hlavtox Hlavtox commented Sep 10, 2023 •
edited
Loading

Questions Answers
Branch? 8.1.x
Description? See below
Type? bug fix
Category? CO
BC breaks? no
Deprecations? no
How to test? See below.
Fixed issue or discussion? Fixes #27506, Fixes #33893
Related PRs
Sponsor company

Description

  • This PR fixes unreliable logic when adding and deleting lang entries for objects.
  • It fixes the need of using magically created primary keys when adding new entries and some weird select. Now it just selects data in default language and shop, cross joins ps_lang table and inserts ignore it back. It doesn't care about any columns and their order.
  • It fixes a delete logic when there is a custom table that doesn't contain id_lang, it would throw an error. I added a check for this.
  • It also fixes the same issue when adding a language. Before copying the data, we check if there is id_lang column.

How to test - 1

  • Have 1 language.
  • Have native GDPR module installed and see if there are some entries in ps_psgdpr_consent_lang.
  • Add a new language and see that they are created for the new language you added.

How to test - 2

  • Create a table ps_blabla_lang in phpmyadmin and put only test column there.
  • Try to delete a language - will work fine.
  • Try adding a language - will work fine.

Auto test

https://github.com/Hlavtox/ga.tests.ui.pr/actions/runs/6156328994

@Hlavtox Hlavtox requested a review from a team as a code owner September 10, 2023 12:49
@prestonBot
Copy link
Collaborator

Hi, thanks for this contribution!

I found some issues with the Pull Request description:

  • Your pull request does not seem to fix any issue, consider creating one (see note below) and linking it by writing Fixes #1234.

Would you mind having a look at it? This will help us understand how interesting your contribution is, thank you very much!

About linked issues

Please consider opening an issue before submitting a Pull Request:

  • If it's a bug fix, it helps maintainers verify that the bug is effectively due to a defect in the code, and that it hasn't been fixed already.
  • It can help trigger a discussion about the best implementation path before a single line of code is written.
  • It may lead the Core Product team to mark that issue as a priority, further attracting the maintainers' attention.

(Note: this is an automated message, but answering it will reach a real human)

@prestonBot prestonBot added 8.1.x Branch Bug fix Type: Bug fix labels Sep 10, 2023
@Hlavtox Hlavtox mentioned this pull request Sep 10, 2023
Closed
M0rgan01
M0rgan01 approved these changes Sep 13, 2023
View reviewed changes
Copy link
Contributor

@M0rgan01 M0rgan01 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the comments, which helps to understand the code

@M0rgan01 M0rgan01 added the Waiting for QA Status: action required, waiting for test feedback label Sep 13, 2023
aniszr
aniszr approved these changes Sep 14, 2023
View reviewed changes
Copy link

@aniszr aniszr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @Hlavtox

I tested your PR and all improvements you'v mentioned in description are verified :

  • 1st Improvement
    Afer adding a new language ps_psgdpr_consent_lang table is well updated by new entries from he added language

See attached screenshots:

image

  • 2nd Improvement

Deleting and adding language works fine event with new table on database ps_blabla_lang for example.

image

image

image

Auto Test : https://github.com/Hlavtox/ga.tests.ui.pr/actions/runs/6156328994 ✔️
QA ✔️

@aniszr aniszr self-assigned this Sep 14, 2023
@aniszr aniszr added QA ✔️ Status: check done, code approved and removed Waiting for QA Status: action required, waiting for test feedback labels Sep 14, 2023
@prestonBot
Copy link
Collaborator

QA approved, well done! Message to the maintainers: do not forget to milestone it before the merge.

@Hlavtox Hlavtox merged commit 26102cf into PrestaShop:8.1.x Sep 14, 2023
@Hlavtox Hlavtox added this to the 8.1.2 milestone Sep 14, 2023
@Hlavtox
Copy link
Contributor Author

Hlavtox commented Sep 14, 2023

Thanks @aniszr! :-)

@Hlavtox Hlavtox deleted the fix-lang-filling branch September 14, 2023 15:45
@Hlavtox Hlavtox mentioned this pull request Feb 28, 2024
Closed
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aniszr aniszr aniszr approved these changes

@M0rgan01 M0rgan01 M0rgan01 approved these changes

@boherm boherm boherm approved these changes

Assignees

@aniszr aniszr

Labels
8.1.x Branch Bug fix Type: Bug fix QA ✔️ Status: check done, code approved
Projects
PR Dashboard
Archived in project
Milestone
8.1.2
Development

Successfully merging this pull request may close these issues.
5 participants
@Hlavtox @prestonBot @boherm @M0rgan01 @aniszr