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

dev/core#1613/Updated Misleading labels #16651

Merged
merged 1 commit into from
Mar 11, 2020
Merged

Conversation

kartik1000
Copy link
Contributor

Signed-off-by: Kartik Kathuria kathuriakartik0@gmail.com

Overview

Update the misleading labels on public Contribution and Registration forms, as mentioned in dev/core#1613 . Changed 'confirm payment' to 'Review your contribution' on contribution form and 'continue' on registration page to 'Review your registration' .

@civibot
Copy link

civibot bot commented Feb 28, 2020

(Standard links)

@civibot civibot bot added the master label Feb 28, 2020
@@ -479,7 +479,7 @@ public function buildQuickForm() {
if (!($allAreBillingModeProcessors && !$this->_values['is_pay_later'])) {
$submitButton = [
'type' => 'upload',
'name' => CRM_Utils_Array::value('is_confirm_enabled', $this->_values) ? ts('Confirm Contribution') : ts('Contribute'),
'name' => CRM_Utils_Array::value('is_confirm_enabled', $this->_values) ? ts('Review your contribution') : ts('Contribute'),
Copy link
Contributor

Choose a reason for hiding this comment

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

@kartik1000 can you fix the indent issue here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, sure. 5 mins.

@kartik1000
Copy link
Contributor Author

@seamuslee001, I have fixed it.

@yashodha
Copy link
Contributor

yashodha commented Mar 2, 2020

test this please

@colemanw
Copy link
Member

colemanw commented Mar 2, 2020

@civibot test this please

@kartik1000
Copy link
Contributor Author

@colemanw are there any issues?

@yashodha
Copy link
Contributor

yashodha commented Mar 4, 2020

@kartik1000 test failure looks unrelated. In general we always try to make sure the tests pass before we merge the PR

@yashodha
Copy link
Contributor

yashodha commented Mar 4, 2020

test this please

@kartik1000
Copy link
Contributor Author

Ok @yashodha.

@kartik1000
Copy link
Contributor Author

@yashodha @colemanw What can we do to make sure the tests pass. Probably go back to the merge from where it's causing the error?

@demeritcowboy
Copy link
Contributor

@kartik1000 The test that is failing (if you click the details link on the right-hand side where it says the checks have failed) is a common false-positive (see https://lab.civicrm.org/dev/core/-/wikis/Testy-tests), so you don't need to do anything about that. But there is a second problem where it says "This branch has conflicts". You need to rebase your branch - see https://docs.civicrm.org/dev/en/latest/tools/git/#rebasing and the section below it.

@kartik1000
Copy link
Contributor Author

Ok @demeritcowboy, I will rebase. So do you feel that if we might run the tests again, these tests might pass then?

@demeritcowboy
Copy link
Contributor

Right. When you push your rebased branch jenkins here will automatically restart the tests.

Signed-off-by: Kartik Kathuria <kathuriakartik0@gmail.com>
@kartik1000
Copy link
Contributor Author

@demeritcowboy, Consequently you were right, all tests have passed!

@demeritcowboy
Copy link
Contributor

demeritcowboy commented Mar 11, 2020

  • General standards
    • [r-explain] PASS
    • [r-user] PASS
      • In the lab ticket there's a lot of agreement about this change.
      • Just for the record: Since the new string isn't already translated elsewhere the button will now start appearing in english on non-english sites until it gets retranslated, but also as noted in the ticket using Word Replacement should work well for this new string. Ditto it will break existing use of Word Replacements, so people will need to update theirs.
    • [r-doc] PASS
    • [r-run] PASS
      • Did a very light run on the test site here since this is just a text change. Works.
  • Developer standards
    • [r-tech] PASS
    • [r-code] PASS
    • [r-maint] N/A
    • [r-test] PASS

@mattwire mattwire merged commit 0c6209e into civicrm:master Mar 11, 2020
@mattwire
Copy link
Contributor

Thanks @kartik1000 merging based on @demeritcowboy review

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

Successfully merging this pull request may close these issues.

6 participants