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

Invoice enhancements #16569

Closed
wants to merge 0 commits into from
Closed

Conversation

magnolia61
Copy link
Contributor

@magnolia61 magnolia61 commented Feb 17, 2020

Overview

The Contribution Invoice workflow template needed some quick fixes:

  • style wise clean up
  • some serious layout fixes
  • make CiviCRM logo comply with Display "empowered by CiviCRM" setting

Before

Bit of a messy Invoice Layout
Screenshot from 2020-02-17 22-41-59

After

Clean professional layout
WITHOUT CIVICRM LOGO
Screenshot from 2020-02-17 22-43-14
WITH CIVICRM LOGO
Screenshot from 2020-02-17 22-52-02

Technical Details

Just the template file for now, upgrade files will follow

Comments

Ideally the Contribution Invoice would use a configurable logo.
I have been thinking to use the Profile picture from the system user, but I will open a gitlab to discuss the best option for this.

@civibot
Copy link

civibot bot commented Feb 17, 2020

(Standard links)

@civibot civibot bot added the master label Feb 17, 2020
@magnolia61 magnolia61 force-pushed the invoice_enhancements branch 10 times, most recently from a8d8697 to 69d0973 Compare February 17, 2020 21:36
@magnolia61 magnolia61 force-pushed the invoice_enhancements branch 2 times, most recently from 1463751 to ef97170 Compare February 17, 2020 22:18
@yashodha
Copy link
Contributor

@magnolia61 This needs to be handled for upgrade as well right?

@magnolia61
Copy link
Contributor Author

magnolia61 commented Feb 18, 2020

@magnolia61 This needs to be handled for upgrade as well right?

yep, i will add the upgrade files later today, but I will also create a gitlab issue for this one, as I hope some people will test the pdf creation of the invoice and comment on it. Are you able to test this invoice template?

@sunilpawar
Copy link
Contributor

@magnolia61 i did similar changes for formatting the invoice template

Key improvement was :
1 .fixing empty line if address part is missing for contact, adjust space, comma between the 2 word if variable value is missing.
2. add domain_postal_code_suffix
3. Add country Abbreviation for billing contact
4. Correcting Billing Name When Payment made using on behalf of org contact.
5. Option to Expose Payment Method (Payment Instrument) for received payments.
6. Email Invoice with CC and Subject field Option.
7. Change Header 'Invoice' / 'Receipt' on Payment Status.

I would like to add these changes.. may be after this PR get merged.

@magnolia61
Copy link
Contributor Author

@magnolia61 i did similar changes for formatting the invoice template

Key improvement was :
1 .fixing empty line if address part is missing for contact, adjust space, comma between the 2 word if variable value is missing.
2. add domain_postal_code_suffix
3. Add country Abbreviation for billing contact
4. Correcting Billing Name When Payment made using on behalf of org contact.
5. Option to Expose Payment Method (Payment Instrument) for received payments.
6. Email Invoice with CC and Subject field Option.
7. Change Header 'Invoice' / 'Receipt' on Payment Status.

I would like to add these changes.. may be after this PR get merged.

Nice Sunil, Great plan I think. I am finalizing my first layout cleanup part. After that I want to take a look more at the logic of the invoice.

I tried to add the country for billing contact but {$country} did not work for me. What did you use?

@sunilpawar
Copy link
Contributor

@magnolia61
can you try this in 'CRM/Contribute/Form/Task/Invoice.php' file?

      $countryAbbreviation = '';
      if (!empty($billingAddress[$contribution->contact_id]['country_id'])) {
        $countryAbbreviation = CRM_Core_PseudoConstant::country($billingAddress[$contribution->contact_id]['country_id']);
      }

then assign it to template
'countryAbbreviation' => $countryAbbreviation,

@mattwire
Copy link
Contributor

I think this PR fixes the country token? #15964

@magnolia61 @sunilpawar If that's the case then I think we should get that merged and then build on that for invoice improvements.

@magnolia61
Copy link
Contributor Author

I think this PR fixes the country token? #15964

@magnolia61 @sunilpawar If that's the case then I think we should get that merged and then build on that for invoice improvements.

Hi Matt, I can confirm that patch enables {$country} as a token in the template.

@magnolia61
Copy link
Contributor Author

@magnolia61 i did similar changes for formatting the invoice template

Key improvement was :
1 .fixing empty line if address part is missing for contact, adjust space, comma between the 2 word if variable value is missing.
2. add domain_postal_code_suffix
3. Add country Abbreviation for billing contact
4. Correcting Billing Name When Payment made using on behalf of org contact.
5. Option to Expose Payment Method (Payment Instrument) for received payments.
6. Email Invoice with CC and Subject field Option.
7. Change Header 'Invoice' / 'Receipt' on Payment Status.

I would like to add these changes.. may be after this PR get merged.

Hello Sunil, I opened up https://lab.civicrm.org/dev/core/issues/1617
Could you try my current PR and comment on it, and possibly also comment in the gitlab issue. I would like to get some people check the layout on their system to see of it all working properly.

@mlutfy
Copy link
Member

mlutfy commented Feb 29, 2020

Logo nitpick: I think it was meant as an example of what an invoice could look like, but otherwise it gives the impression that the invoice is from an organization called CiviCRM, which is misleading.

It might be best to remove the logo completely? Otherwise we are just encouraging people to disable the display of the CiviCRM logo on their frontend forms.

edit: to clarify, overall this is a really nice improvement. We/Symbiotic use invoicing a lot, it's a really nice & undervalued feature of CiviCRM. We always replaced the default template, but it's a nice feature and we should provide a sane default invoice that everyone can use.

@eileenmcnaughton
Copy link
Contributor

@magnolia61 can you rebase - the upgrade script is hitting conflicts.

Regarding the logo - good points by @mlutfy but I think it's probably better not to remove the logo in this PR as I this PR doesn't add it & it feels like scope creep - also as a follow up it might attract discussion which might be appropriate in it's own issue

@magnolia61 magnolia61 force-pushed the invoice_enhancements branch from ff2751e to 72afd77 Compare March 1, 2020 20:21
@magnolia61 magnolia61 force-pushed the invoice_enhancements branch from 67694ba to 6df1469 Compare March 1, 2020 23:35
@eileenmcnaughton
Copy link
Contributor

@magnolia61 how about removing the upgrade from this PR & putting it up separately once this is merged?

@magnolia61 magnolia61 force-pushed the invoice_enhancements branch 5 times, most recently from 7c99cc2 to 4937a52 Compare March 2, 2020 09:16
@magnolia61
Copy link
Contributor Author

I removed the upgrader and test.
But the test still says:

error: patch failed: CRM/Upgrade/Incremental/MessageTemplates.php:199                    
error: CRM/Upgrade/Incremental/MessageTemplates.php: patch does not apply                

Did the test not properly re-run?

@magnolia61
Copy link
Contributor Author

jenkins, test this please

@mlutfy
Copy link
Member

mlutfy commented Mar 2, 2020

I feel like there is a weird github caching issue:

$ git apply /tmp/16569.patch
error: patch failed: CRM/Upgrade/Incremental/MessageTemplates.php:199
error: CRM/Upgrade/Incremental/MessageTemplates.php: patch does not apply

yet when we view the changes, the MessageTEmplates.php changes have been removed, but not when we view the patch directly: https://github.com/civicrm/civicrm-core/pull/16569.patch

@demeritcowboy
Copy link
Contributor

I think it's the way git apply works when there's multiple commits in the patch across time and other changes in between. If you squash the commits it will probably fix it.

@eileenmcnaughton
Copy link
Contributor

@magnolia61 try squashing to one commit

@magnolia61 magnolia61 force-pushed the invoice_enhancements branch 4 times, most recently from 777f7ea to 572f54f Compare March 3, 2020 00:14
@magnolia61 magnolia61 closed this Mar 3, 2020
@magnolia61 magnolia61 force-pushed the invoice_enhancements branch from 2bd7057 to 8c20952 Compare March 3, 2020 00:21
@magnolia61 magnolia61 reopened this Mar 3, 2020
@magnolia61 magnolia61 force-pushed the invoice_enhancements branch 4 times, most recently from 268963c to ea7a4b9 Compare March 3, 2020 00:57
@magnolia61
Copy link
Contributor Author

:-) maybe better to start a new pr to bypass the mess?

@demeritcowboy
Copy link
Contributor

Git is fun yeah? What's kept me from taking a hammer to the nearest thing I can find is usually:

git pull --rebase upstream master
git reset --soft upstream/master

But if you have actual file conflicts during the first command then I find it's easier to make a new branch based on a fresh master and copy/paste your changes over.

If you go the hammer route, aim for something that isn't yours in a way that makes it look like you were aiming for something else.

@magnolia61 magnolia61 force-pushed the invoice_enhancements branch from b3e770b to a2cb3ec Compare March 3, 2020 08:21
@magnolia61 magnolia61 closed this Mar 3, 2020
@magnolia61 magnolia61 force-pushed the invoice_enhancements branch from c2e3c26 to 2bef0ef Compare March 3, 2020 08:24
@magnolia61
Copy link
Contributor Author

I gave up on this one. Next attempt at #16680 :-)

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.

7 participants