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

[NFC] {Test} Minor cleanup #21116

Merged
merged 1 commit into from
Aug 13, 2021
Merged

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented Aug 13, 2021

Overview

[NFC] {Test} Minor cleanup

Before

    'invoice_number' => CRM_Utils_Array::value('invoice_prefix', Civi::settings()->get('contribution_invoice_settings')) . "" . $contributionID,

After

      'invoice_number' => Civi::settings()->get('invoice_prefix') . $contributionID,

@civibot
Copy link

civibot bot commented Aug 13, 2021

(Standard links)

@civibot civibot bot added the master label Aug 13, 2021
@@ -1288,7 +1282,7 @@ public function testCreateContributionPendingOnline() {
$this->assertEquals($contribution['values'][$contribution['id']]['trxn_id'], 12345);
$this->assertEquals($contribution['values'][$contribution['id']]['invoice_id'], 67890);
$this->assertEquals($contribution['values'][$contribution['id']]['source'], 'SSF');
$this->assertEquals($contribution['values'][$contribution['id']]['contribution_status_id'], 2);
$this->assertEquals(2, $contribution['values'][$contribution['id']]['contribution_status_id']);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK - I realise all the lines above need the same change - I decided after the first one not to get carried away

@@ -316,7 +310,7 @@ public function testGetContributionReturnFunctionality(): void {
// update contribution with invoice number
$params = array_merge($params, [
'id' => $contributionID,
'invoice_number' => CRM_Utils_Array::value('invoice_prefix', Civi::settings()->get('contribution_invoice_settings')) . "" . $contributionID,
'invoice_number' => Civi::settings()->get('invoice_prefix') . $contributionID,
Copy link
Contributor

Choose a reason for hiding this comment

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

I can see the original was copy/paste from elsewhere, hence the extra . "". In other languages I've seen sometimes you need to do this type of thing to force string, since maybe a prefix like 0001 got output as just 1, although that doesn't seem to be true in php. But maybe something along those lines, I just can't think what.

But specifically for this test, isn't the prefix always null and known to be such? Not a blocker just was probably also part of copy/paste in the original.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@demeritcowboy yeah I guess if I'd thought it through further I would have removed it

@demeritcowboy demeritcowboy merged commit 483eccb into civicrm:master Aug 13, 2021
@eileenmcnaughton eileenmcnaughton deleted the nfc branch August 13, 2021 20:13
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.

2 participants