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.

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

Fix adding languages in multilingual #17228

Merged
merged 3 commits into from
May 25, 2020

Conversation

mattwire
Copy link
Contributor

@mattwire mattwire commented May 4, 2020

Overview

Adding or removing a language when multilingual is enabled fails with a DB error. This is because it deletes the "currenciesEnabled" optiongroup and values and then tries to recreate them - but in multilingual this fails because label is a required field in optionvalue for all languages but each language query will try and run a query that adds/updates an entry for it's own language (leaving a required field not set for the other language labels).

Before

Issues adding/removing languages in multilingual.

After

  • Languages can be added/removed.
  • Updating optionGroup/optionValues now tries to update instead of blindly deleting first.
  • currenciesEnabled is only updated AFTER enabling/disabling multilingual - it doesn't make sense to try to do this before.

Technical Details

We use the DAO find() method to find existing optiongroup/values and update them.

Comments

@mlutfy @aydun @MikeyMJCO Testing welcome :-)

@civibot civibot bot added the master label May 4, 2020
@civibot
Copy link

civibot bot commented May 4, 2020

(Standard links)

@jaapjansma
Copy link
Contributor

test this please

@jaapjansma
Copy link
Contributor

  • General standards
    • Explain (r-explain)
      • PASS : The goal/problem/solution have been adequately explained in the PR.
    • User impact (r-user)
      • PASS: The change would be intuitive or unnoticeable for a majority of users who work with this feature.
    • Documentation (r-doc)
      • PASS: The changes do not require documentation.
    • Run it (r-run)
      • PASS: We have run it in both dmaster and in the test environment of this pr.
  • Developer standards
    • Technical impact (r-tech)
      • PASS: The change preserves compatibility with existing callers/code/downstream.
    • Code quality (r-code)
      • PASS: The functionality, purpose, and style of the code seems clear+sensible.
    • Maintainability (r-maint)
      • PASS: The change is trivial enough that it does not require tests.
      • COMMENT: I dont think this needs a test but a test might be helpful to detect if this breaks again in the future. So a test would have been a surplus but it is not blocking our advice of merging this PR.
    • Test results (r-test)
      • PASS: The test results are all-clear.

@eileenmcnaughton or @colemanw could one of you merge this PR?

@mlutfy mlutfy merged commit 18d2227 into civicrm:master May 25, 2020
@mlutfy
Copy link
Member

mlutfy commented May 25, 2020

Thanks @mattwire and @jaapjansma for reviewing.

I'm a bit surprised that the test coverage did not catch this, since we do have tests for multilingual?

@eileenmcnaughton
Copy link
Contributor

note this caused a regression which is partially fixed in #20627

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

Successfully merging this pull request may close these issues.

4 participants