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#3732) Show billing address on Edit contribution screen as well #24009

Merged
merged 1 commit into from
Sep 26, 2022

Conversation

yashodha
Copy link
Contributor

@yashodha yashodha commented Jul 18, 2022

Overview

We currently show billing address on View Contribution screen. We should show billing address on Edit Contribution screen as well for consistency.

Before

Billing address is shown only on View contribution screen
cf_pre

After

Billing address is shown on both View contribution and Edit contribution screens
cf_post

@civibot
Copy link

civibot bot commented Jul 18, 2022

(Standard links)

@civibot civibot bot added the master label Jul 18, 2022
$addressDetails = CRM_Core_BAO_Address::getValues($addressParams, FALSE, 'id');
$addressDetails = array_values($addressDetails);
$this->assign('billing_address', $addressDetails[0]['display']);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Overall this PR looks good but some notes:

  • We don't use CRM_Utils_Array::value() anymore if possible (use php 7 ??), but also this is inside an if where it already checks if the value exists, so it's not necessary.
  • You need to $this->assign('billing_address') outside the if so that it always has a value, since otherwise the tpl {if $billing_address} will give a warning. Maybe just before the if add $this->assign('billing_address', NULL); so it will be assigned something.

Copy link
Contributor

Choose a reason for hiding this comment

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

@yashodha this just needs a minor tweak ^^^^ and then would be mergeable

@yashodha
Copy link
Contributor Author

@demeritcowboy I have incorporated the feedback for the PR.

@demeritcowboy demeritcowboy merged commit 817311f into civicrm:master Sep 26, 2022
@yashodha
Copy link
Contributor Author

@demeritcowboy thanks!

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