fix unit plural in contribution page #21944

masetto · 2021-10-30T17:20:57Z

This is the proposal to solve the problem of translation of the plural of frequency units in contribution page.
See this issue.

If the solution is good, it should also be applied in the pledge page.

civibot · 2021-10-30T17:21:00Z

(Standard links)

If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

demeritcowboy · 2021-10-31T12:59:25Z

It looks like all the .po files were autoupdated and now a lot of things have become untranslated, e.g. https://lab.civicrm.org/dev/translation/-/commit/5590912b2eb023c84975d86c51da68922c1c300b#21b169ad3734b06f683b854acd7a6621196eed52_30615_31048

masetto · 2021-10-31T18:29:18Z

@demeritcowboy sorry, I don't understand your comment and why checks are not passed.

demeritcowboy · 2021-10-31T18:31:33Z

The month names used to be in report.po, but after the latest transifex update they are now in common-base.po and are no longer translated. fr_FR and es_MX need the month names to be retranslated in transifex.

masetto · 2021-10-31T18:49:10Z

But what does that have to do with my pull request?

demeritcowboy · 2021-10-31T18:52:04Z

It just explains the test fails. It's not caused by your PR. All the PRs will fail until those month names are updated in fr_FR and es_MX. I posted here since it's where I saw it first but you're right it's a more general test fail.

mlutfy · 2021-11-05T18:56:27Z

jenkins, test this please

demeritcowboy · 2021-11-05T23:29:23Z

jenkins retest this please

strange error

FATAL: command execution failed
java.io.EOFException
	at java.io.ObjectInputStream$PeekInputStream.readFully(ObjectInputStream.java:2681)
	at java.io.ObjectInputStream$BlockDataInputStream.readShort(ObjectInputStream.java:3156)
	at java.io.ObjectInputStream.readStreamHeader(ObjectInputStream.java:862)
	at java.io.ObjectInputStream.<init>(ObjectInputStream.java:358)
	at hudson.remoting.ObjectInputStreamEx.<init>(ObjectInputStreamEx.java:49)
	at hudson.remoting.Command.readFrom(Command.java:142)
	at hudson.remoting.Command.readFrom(Command.java:128)
	at hudson.remoting.AbstractSynchronousByteArrayCommandTransport.read(AbstractSynchronousByteArrayCommandTransport.java:35)
	at hudson.remoting.SynchronousCommandTransport$ReaderThread.run(SynchronousCommandTransport.java:63)
Caused: java.io.IOException: Unexpected termination of the channel
	at hudson.remoting.SynchronousCommandTransport$ReaderThread.run(SynchronousCommandTransport.java:77)

braders · 2021-11-10T19:05:16Z

CRM/Contribute/BAO/Contribution.php

+        return ts('year(s)');
+
+      default:
+        throw new CRM_Core_Exception('Unknown unit: ' . $unit);


It's worth noting that it's currently possible to edit the "Recurring Frequency Units" options from the "Administer" -> "Option Groups" area of the CiviCRM UI.

I can't think of any good reasons for adding or removing frequency units in this way, but there might be - if so it might be better to fallback to the exiting method of appending '(s)' if an unrecognised string is found, rather than throwing the exception.

Alternatively, maybe the "Recurring Frequency Units" option_group should be set to is_locked so that new options cannot be added. (and appreciate that's a seperate task, but maybe something to consider in tandem).

@braders see comments of this issue: I would have happily done so: return ts($unit . '(s)'); but we cannot put a variable in translation: https://docs.civicrm.org/dev/en/latest/translation/#best-practices.

Furthermore, the intervention is consistent with this. See also this comment.

jaapjansma · 2021-11-17T07:14:38Z

test this please

jaapjansma · 2021-11-17T08:59:09Z

@masetto we (@kainuk, @BettyDolfing and I) are doing reviews of PR's. We tried to test this PR, with CiviCRM in Dutch, but we failed to do so. On the contribution page we get errors and the dropdown with frequencies is still in English. It might be that something unrelated on the test site is broken but it might also be that your commits breaks something else. We are not sure about that.

Can you have another look at it? And maybe come up with instructions on how to test this? So we can reproduce the problem on dmaster.demo.civicrm.org and see whether your fix works.

Here some screenshot:

masetto · 2021-11-17T10:39:59Z

@jaapjansma I am sorry that you have had these problems, but they do not seem to be due to my changes.
To test them I add plurals of units in Words Replacement:

Them my contribution form appears like this:

multiple units:
single unit:

demeritcowboy · 2021-11-21T00:20:20Z

General standards
- [r-explain] PASS
- [r-user] PASS
- [r-doc] PASS
- [r-run] Issue
  - In the case where there's only one frequency unit it crashes when you submit the confirmation page. See r-code.
  - Otherwise it seems to work just these strings (day(s), etc) aren't in transifex yet so as noted earlier they don't translate unless you do word replacements.
Developer standards
- [r-tech] PASS
  - As noted in the lab tickets redesigning the form might be better long-term, but this seems like an improvement.
  - There was some concern also in the tickets about the awkwardness of the (s), and even in english it feels funny, but again it still seems like an improvement pending a bigger fix. (This is probably why the strings don't already exist in transifex.)
- [r-code] Issue
  - The hidden field frequency_unitCRM_Contribute_BAO_Contribution seems odd and unused. In the case where there's only one frequency unit available, you get a crash when submitting the confirmation form. Even if you put the field name back to frequency_unit, it then still crashes but with InvalidArgumentException: "recurFrequencyUnit must be day|week|month|year
  - The CRM_Core_OptionGroup::values() calls (especially where it calls it with the label column requesting value), and the $frUnits and $frequencyUnits and $unitVals manipulations are all a bit confusing. But some of it is pre-existing, and it seems to work out. But there might be some opportunity for cleanup here.
- [r-maint] ?
  - It would be nice to have but I don't know how you'd write a test for this.
- [r-test] PASS

Also should squash the commits.

demeritcowboy · 2021-11-21T12:17:12Z

I need to update my review. The frequency_unitCRM_Contribute_BAO_Contribution field is problematic. In testing I had completed the form and submitted a couple variations but I guess not that one. I've updated above.

masetto · 2021-11-21T17:12:41Z

I am really sorry about the typo.:thumbsdown:

demeritcowboy · 2021-11-21T17:21:51Z

No worries but note it still crashes. I think because the hidden field has the translated value.

masetto · 2021-11-21T17:45:33Z

In my tests it does not crash, I can't replicate it.
$form->_values['recur_frequency_unit']` shouldn't have always option group value (always month, year, week or day)?

demeritcowboy · 2021-11-21T17:48:43Z

When there's only one unit option, e.g. if only months is checked on the config page.

masetto · 2021-11-21T21:22:05Z

Yes, I know, I'm testing like this, only one unit option. And these are my recur_frequency_units:

This is recurring contribution configuration of my contribution test page:

demeritcowboy · 2021-11-21T21:59:09Z

On the test site here: http://core-21944-2pmad.test-1.civicrm.org:8001/civicrm/contribute/transact?reset=1&id=4

This is the config for that page:

Then fill it out and submit:

Then click the button to complete the contribution:

Then it gives the error:

InvalidArgumentException: "recurFrequencyUnit must be day|week|month|year"

0 /home/jenkins/bknix-dfl/build/core-21944-2pmad/web/sites/all/modules/civicrm/Civi/Payment/PropertyBag.php(232): Civi\Payment\PropertyBag->setRecurFrequencyUnit("mese", "default")
1 /home/jenkins/bknix-dfl/build/core-21944-2pmad/web/sites/all/modules/civicrm/Civi/Payment/PropertyBag.php(352): Civi\Payment\PropertyBag->offsetSet("frequency_unit", "mese")
2 /home/jenkins/bknix-dfl/build/core-21944-2pmad/web/sites/all/modules/civicrm/Civi/Payment/PropertyBag.php(120): Civi\Payment\PropertyBag->mergeLegacyInputParams((Array:72))
3 /home/jenkins/bknix-dfl/build/core-21944-2pmad/web/sites/all/modules/civicrm/CRM/Core/Payment/Dummy.php(70): Civi\Payment\PropertyBag::cast((Array:72))
4 /home/jenkins/bknix-dfl/build/core-21944-2pmad/web/sites/all/modules/civicrm/CRM/Contribute/Form/Contribution/Confirm.php(2770): CRM_Core_Payment_Dummy->doPayment((Array:72))
5 /home/jenkins/bknix-dfl/build/core-21944-2pmad/web/sites/all/modules/civicrm/CRM/Contribute/Form/Contribution/Confirm.php(2397): CRM_Contribute_Form_Contribution_Confirm->processConfirm((Array:72), "204", "1", 0, "1")
6 /home/jenkins/bknix-dfl/build/core-21944-2pmad/web/sites/all/modules/civicrm/CRM/Contribute/Form/Contribution/Confirm.php(856):

I haven't tested but I think you need to put back line 786 because right now it's the translated value in the hidden field.

masetto · 2021-11-22T08:10:12Z

Right! I push a fix.

demeritcowboy · 2021-11-22T19:30:56Z

Thanks @masetto looks good. Can you just squash commits and then this is ready to go.

style fixes for compiling check fix a typo fix `recur_frequency_unit` hidden field value when there is only one unit option

BettyDolfing · 2021-11-30T08:00:41Z

test this please

civibot bot added the master label Oct 30, 2021

braders reviewed Nov 10, 2021

View reviewed changes

fix unit plural in contribution page

46c7f9c

style fixes for compiling check fix a typo fix `recur_frequency_unit` hidden field value when there is only one unit option

masetto force-pushed the unit_plural branch from 21478e5 to 46c7f9c Compare November 23, 2021 07:38

demeritcowboy merged commit 6e35ea4 into civicrm:master Nov 23, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix unit plural in contribution page #21944

fix unit plural in contribution page #21944

masetto commented Oct 30, 2021

civibot bot commented Oct 30, 2021

demeritcowboy commented Oct 31, 2021

masetto commented Oct 31, 2021

demeritcowboy commented Oct 31, 2021

masetto commented Oct 31, 2021

demeritcowboy commented Oct 31, 2021

mlutfy commented Nov 5, 2021

demeritcowboy commented Nov 5, 2021

braders Nov 10, 2021

masetto Nov 10, 2021

jaapjansma commented Nov 17, 2021

jaapjansma commented Nov 17, 2021

masetto commented Nov 17, 2021 •

edited

Loading

demeritcowboy commented Nov 21, 2021 •

edited

Loading

demeritcowboy commented Nov 21, 2021

masetto commented Nov 21, 2021

demeritcowboy commented Nov 21, 2021

masetto commented Nov 21, 2021

demeritcowboy commented Nov 21, 2021

masetto commented Nov 21, 2021 •

edited

Loading

demeritcowboy commented Nov 21, 2021

masetto commented Nov 22, 2021

demeritcowboy commented Nov 22, 2021

BettyDolfing commented Nov 30, 2021

fix unit plural in contribution page #21944

fix unit plural in contribution page #21944

Conversation

masetto commented Oct 30, 2021

civibot bot commented Oct 30, 2021

demeritcowboy commented Oct 31, 2021

masetto commented Oct 31, 2021

demeritcowboy commented Oct 31, 2021

masetto commented Oct 31, 2021

demeritcowboy commented Oct 31, 2021

mlutfy commented Nov 5, 2021

demeritcowboy commented Nov 5, 2021

braders Nov 10, 2021

Choose a reason for hiding this comment

masetto Nov 10, 2021

Choose a reason for hiding this comment

jaapjansma commented Nov 17, 2021

jaapjansma commented Nov 17, 2021

masetto commented Nov 17, 2021 • edited Loading

demeritcowboy commented Nov 21, 2021 • edited Loading

demeritcowboy commented Nov 21, 2021

masetto commented Nov 21, 2021

demeritcowboy commented Nov 21, 2021

masetto commented Nov 21, 2021

demeritcowboy commented Nov 21, 2021

masetto commented Nov 21, 2021 • edited Loading

demeritcowboy commented Nov 21, 2021

masetto commented Nov 22, 2021

demeritcowboy commented Nov 22, 2021

BettyDolfing commented Nov 30, 2021

masetto commented Nov 17, 2021 •

edited

Loading

demeritcowboy commented Nov 21, 2021 •

edited

Loading

masetto commented Nov 21, 2021 •

edited

Loading