dev/core#1280 Fix ContributionPage soft_credit translation #16838
Conversation
|
(Standard links)
|
|
jenkins, test this please |
| $this->assertEquals($json['soft_credit']['en_US']['honor_block_title'], 'In Honor Title EN'); | ||
| $this->assertEquals($json['soft_credit']['en_US']['honor_block_text'], 'In Honor Text EN'); | ||
| $this->assertEquals($json['soft_credit']['fr_FR']['honor_block_title'], 'In Honor Title FR'); | ||
| $this->assertEquals($json['soft_credit']['fr_FR']['honor_block_text'], 'In Honor Text FR'); |
There was a problem hiding this comment.
I like the test - just these params are backwards. Expected is the first param. When it fails it's briefly confusing.
| $ufJoinDAO = new CRM_Core_DAO_UFJoin(); | ||
| $ufJoinDAO->module = $ufJoinParam['module']; | ||
| $ufJoinDAO->entity_id = $ufJoinParam['entity_id']; | ||
| $ufJoinDAO->find(TRUE); |
There was a problem hiding this comment.
Is it possible that soft-credit profiles could be used for something other than contribution pages? If so, you'd want to take entity_table into account. It used to before with the deleteAll. Otherwise this might match on multiple and might update the wrong one.
There was a problem hiding this comment.
I think the combo would be surprising, but I added an extra check, just in case.
However, I only did a quick code edit, need to re-test.
|
test this please |
|
@demeritcowboy what's your take on this - it looks like you reviewed it - is it pending a response from @mlutfy |
|
@mlutfy style issues... |
|
I don't remember running it just code review. Yes, one clerical, one functional. I must have run the test at least. |
|
Keeping a copy in case it disappears too quickly:
|
mlutfy
force-pushed
the
core1280
branch
3 times, most recently
from
1ca3fd8 to
0c0aed6
Compare
|
Thanks for the review @demeritcowboy ! I updated my PR with the recommandations:
I just need to do one last test to make sure that adding the extra check didn't break anything. |
|
I did a bit more testing and it seems to be working as expected.
|
|
@mlutfy please self-merge once @demeritcowboy gives this a thumbs up |
|
@mlutfy I think you've found an awesomely sneaky way of getting tests to pass. Just name the file slightly differently than the actual test class. The test framework won't even run it! Easy pass. (FYI @totten @seamuslee001 ) The test runs fine if you run it manually. So either the file or the classname needs to be renamed, but otherwise all good. |
|
Yikes, good catch 😬 Note to self: in the full test log, ctrl-F for ContributionPageTranslationTest |
|
Looks good. So I think this is now "merge-on-pass". |
|
Thanks again, @demeritcowboy, very much appreciated! |
|
Tests have not completed yet, but seeing this now: |
|
Cool. And actually if you have a minute #17806 just needs an r-run and that's the last piece before the outputhandler stuff is ready. |
Overview
https://lab.civicrm.org/dev/core/issues/1280
To reproduce:
Now switch languages, edit text and save.
Result: data disappears or in wrong language.