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

CRM-21756: Freeze total amount field on Edit Contribution if related to Membership or Event registration #11780

Merged
merged 1 commit into from
Mar 11, 2018

Conversation

monishdeb
Copy link
Member

@monishdeb monishdeb commented Mar 7, 2018

Overview

Currently, when we change the total amount of membership payment, it doesn't update the corresponding line-item data. Instead, we can recreate the membership payment or we either use Lineitem editor.

Before

Total amount field is editable and on change leads to outdated line-item entry.

After

Total amount field is frozen and there is help text beside this field that provides instruction to perform the same task either by recreating the membership or using Lineitem Editor.
screen shot 2018-03-07 at 7 39 48 pm


@monishdeb
Copy link
Member Author

monishdeb commented Mar 7, 2018

@eileenmcnaughton will you help me to change the help text more appropriate?

@kcristiano
Copy link
Member

(CiviCRM Review Template WORD-1.1)

(r-explain) Pass
(r-test) Pass
(r-code) Pass
(r-doc) Pass
(r-maint) Pass
(r-run) Pass - I was able to replicate the problem without the patch.   With the patch you can no longer edit teh total amount
(r-user) Pass
(r-tech) Pass

The message

You are not allowed to change the total amount as it will lead to incorrect line item entries. You can either delete or recreate or install Line Item Editor.

My only issue is that I cannot install the Line Item editor from the CiviCRM Extension Page If we are promoting the extension, I think it should be available for users to install from inside CiviCRM.

As far as my attempt on the language:

You cannot change the total amount as it will lead to incorrect records in the database. If you need to edit the Total Amount you can either delete and recreate the transaction or install Line Item Editor.

@eileenmcnaughton
Copy link
Contributor

test this please

@eileenmcnaughton
Copy link
Contributor

I tested this & looked at the code as @kcristiano had & am happy to merge it. I agree with Kevin's points ie. 1) his wording is slightly better - that will be easy to merge as a follow up & 2) @monishdeb @JoeMurray can I get you to commit to submitting lineitemedit for review. I am happy to submit a review as I've already been looking at it & the code looks solid to me & it seems a clear improvement installing it

@tannerm
Copy link

tannerm commented Sep 13, 2018

This commit caused an issue with a client site where they are no longer able to update the Financial Type of contributions associated with Membership or Participant. Previously there was a conditional isset($this->_values['tax_amount']) that failed thus allowing them to edit the Financial Type since the tax_amount was set to NULL.

Can you explain a little more why the tax_amount conditional was removed? Thanks!

@eileenmcnaughton
Copy link
Contributor

I think because it was not actually accurately updating all the related financial entities - have you tried the line item editor extension?

@tannerm
Copy link

tannerm commented Sep 13, 2018

Oh! I have not. I'll look into that... thanks for the quick reply!

@tannerm
Copy link

tannerm commented Oct 2, 2018

@eileenmcnaughton finally getting back around to this and that extension worked perfectly. Thanks!

@eileenmcnaughton
Copy link
Contributor

@tannerm great (fyi @JoeMurray @monishdeb )

@JoeMurray
Copy link
Contributor

@Edzelopez could you ensure that the Line Item Edit extension is published and up to date and available for auto-download? Thanks.

@mfb
Copy link
Contributor

mfb commented Feb 8, 2019

We need to be able to edit the contribution financial type, but it looks like if we use the Line Item Edit extension, that lets us modify the line item, but the financial type of its parent contribution is not updated.

@eileenmcnaughton
Copy link
Contributor

@monishdeb do you think you can update line item editor to permit editing financial type on the contribution at the same time ?

@JoeMurray
Copy link
Contributor

We could. Please create an issue on its repo and assign to @Monish.

@JoeMurray
Copy link
Contributor

@mfb would it be okay if just let people choose whatever they want for contrib FT? It's irrelevant to accounting entries, though many orgs just use reports where contrib FT has historically been key.

@eileenmcnaughton
Copy link
Contributor

thanks @JoeMurray

@mfb
Copy link
Contributor

mfb commented Feb 8, 2019

Yes, reports is why we need to be able to set it. I would say civi admins should be able to set it to whatever they want - either Civi core should let them do this, or the extension could do it. My 2 cents is that Civi core could make this editable, but I leave this up to folks who have spent more time looking at the related code :)

@mfb
Copy link
Contributor

mfb commented Feb 8, 2019

Created an issue on the extension: JMAConsulting/biz.jmaconsulting.lineitemedit#32

@JoeMurray
Copy link
Contributor

Thanks @mfb

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