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/financial#6 Template contributions on the contact summary #20452

Merged
merged 5 commits into from
Jun 28, 2021

Conversation

jaapjansma
Copy link
Contributor

@jaapjansma jaapjansma commented May 31, 2021

Overview

This PR does the following

  • Exclude contributions with is_template = 1 from the Contributions tab on the contact summary
  • The count on the contribution tab only counts non template contributions.

This PR is part of the work done for https://lab.civicrm.org/dev/financial/-/issues/6

Before

All contributions, including the template contributions, where shown on the contributions tab on the contact summary.

After

The template contributions are not shown on the contribution tab.
Also the count on the tab for contributions does only count the non template contributions.

Comments

See https://lab.civicrm.org/dev/financial/-/issues/6
And https://lab.civicrm.org/dev/core/-/issues/2624

An alternative PR for creation of template contribution by the user is #20685

@civibot
Copy link

civibot bot commented May 31, 2021

(Standard links)

@eileenmcnaughton
Copy link
Contributor

I agree with the search changes - I'm on the fence about the UI change in core so I'll stick it in the dev-digest.

I wouldn't normally merge any change to search without a unit test

@mattwire
Copy link
Contributor

mattwire commented Jun 1, 2021

Hi @jaapjansma Can you split the visible changes (CRM/Contribute/Page/Tab.php) into it's own PR? I am happy to merge the rest now but I think that the new "View Template" button could confuse users if we don't a more "complete" implementation and some documentation. The next RC will be branched in the next few days and the non-visible changes could certainly go in there, but the visible changes could be better in the next master so there is more time to complete the UI changes.

@jaapjansma
Copy link
Contributor Author

jaapjansma commented Jun 4, 2021

@mattwire the thing I am working on right now (with funding) is to make this more complete. So I if you really insist I can move the view template contribution to a separate PR.
I am also happy to work on the documentation and it does not have to ben in the next RC, could be the one after or the one after that one.

@jaapjansma
Copy link
Contributor Author

@eileenmcnaughton

I'm on the fence about the UI change in core so I'll stick it in the dev-digest.

What do you mean with this? I read it as no body is able to do ui changes. I also read it in Eileen the benevolent dictator decides what gets in core and what not. I do believe you probably want to communicate something else.

@eileenmcnaughton
Copy link
Contributor

@jaapjansma it means I don't know if it is a good idea and that it should be put out for community discussion - via the dev digest

@jaapjansma
Copy link
Contributor Author

jaapjansma commented Jun 7, 2021

I believe that hiding stuff from the user could lead to potentially create a system with magic in it. (Because only experts know there is more data in the database). So I therefor I believe it is wise to make things visible in the user interface and I do no understand your hesitation.

On this specific PR (and the related ones): I have a client who wants to see the line items and want to be able to edit them. And they gave me funding to work on those PR's. On advice from @mattwire I have implemented the template contributions on the recurring contributions series so if you decide this could not be merged the point of my whole work has gone.
On the other hand I can imagine that editing the template contribution is not always wanted/desired (probably depending on the payment processor in use).

So if it helps I can put up another PR where the editing of template contribution is allowed based on a(new) permission and/or the edit ability from the payment processor.

…ry and add a button to view them to the recurring contributions tab
…ry and add a button to view them to the recurring contributions tab
@jaapjansma jaapjansma force-pushed the dev_financials_6_contactsummary branch from 439348f to 6e28570 Compare June 7, 2021 08:22
@seamuslee001
Copy link
Contributor

@jaapjansma

This is not about Eileen deciding anything, the point is that there may need to have more opinions from the broader community in regards to whether this is something we are looking to have exposed at a UI level and if we are exactly what do we want exposed and how that might be done.

The point about having the dev digest is to be able to circulate proposals to the broader community so that there can be more opinions sought.

I would also add that there is already an extension that has been written that deals with editing of line items,

@jaapjansma
Copy link
Contributor Author

jaapjansma commented Jun 7, 2021

@seamuslee001 I am not against a discussion whether this needed. However I understood it that this discussion already took place in Barcelona... See issue dev/financial#6, I have followed the principles set out from that discussion. So a sudden doubt doesn't make sense to me.

I would also add that there is already an extension that has been written that deals with editing of line items,

yes we can only use that extension if the line items are visible in the UI. This PR is adding a button to view the template contribution.

@eileenmcnaughton
Copy link
Contributor

I have pasted what I put into the dev-digest here https://lab.civicrm.org/dev/financial/-/issues/6#note_60264

@tttp
Copy link
Contributor

tttp commented Jun 11, 2021

I have been in the community for many (many) years, and I have to say I find the communication here not good enough. We strive being welcoming, inclusive and friendly, and I think we failed here.

I've known the participants in this discussion for years, we all know each other and talked face to face and drunk too many warm beers. I understand that some of the points are probably connected to other things that were discussed previously or offline.

I know as well than there are cultural differences and some cultures have very direct -borderline blunt- ways to communicate, and some like more convoluted -and possibly less clear- styles, and I'm pretty sure a lot of it are just stuff that got lost in translations

But all of that doesn't matter because online sadly, every piece of communication can -and will- be taken out of context, and I'm sure that none of us would want to join our community based on that exchange. I don't think that comments like these reflect what we strive to be.

nobody is able to do ui changes.... Eileen the benevolent dictator decides what gets in core
so if you decide this could not be merged the point of my whole work has gone

I know that jaap isn't sexist, and I know he's not trying to bully Eileen's or really thinking she's a dicator, but I doubt that a casual female reader considering contributing to our community will have that context, and I know that things online get taken out of context

One last thing: as a non native native english speaker, this sentence

I'm on the fence about the UI change in core so I'll stick it in the dev-digest
is hard to understand and might easily be mis-interpreted (in french "I'm on the fence" sounds like "I'm at the barricade prepared to fight you" and "stick it" tend to be followed by "up yours")

But I'll stop and try to put context without real knowledge of it, hopefully this will be sorted out and civicrm will end up better, but I'd like to finish with a "dear casual reader, we really try being an inclusive and welcoming community, don't think this exchange represent who we are and how we strive to exchange"

Love and sunny friday

Xavier

@totten
Copy link
Member

totten commented Jun 14, 2021

One last thing: as a non native native english speaker, this sentence

I'm on the fence about the UI change in core so I'll stick it in the dev-digest

is hard to understand and might easily be mis-interpreted (in french "I'm on the fence" sounds like "I'm at the barricade prepared to fight you" and "stick it" tend to be followed by "up yours")

Thank you @tttp for sharing that. As a native speaker, I never would have imagined that interpretation! 😳 The phrase "stick it" can convey two very different things. (By itself, yes, it can be offensive. But in the context of a full sentence with real subject matter, it's usually innocent - it just means "to put" in a casual/low-effort way. Ex: "Hey, I found a case of warm beer. Let's stick it in the fridge.")

@tttp
Copy link
Contributor

tttp commented Jun 14, 2021

I'm very sure that neither Eileen nor Jaap had any ill intent, just that communication got off track and that without context, the end result doesn't reflect how welcoming the community is ;)

and for any native English speaker, please keep the slang and colloquial, it's fun having to check things in the urban dictionary.

@jaapjansma
Copy link
Contributor Author

I don't think that comments like these reflect what we strive to be.

I want to apologize for my comment. My comment was written out of frustration. And my wording, and my tone of voice, does not fit on how we want to interact with each other in the community. It does reflect the warm and welcoming spirit of our community.
Thanks @tttp for pointing this out!

We strive being welcoming, inclusive and friendly, and I think we failed here.

I agree we failed here. In believe in many ways. And I also believe I failed here.

I want to use this comment to highlight where I think we are failing as a community. And I am saying we here, as I do think the community is an effort of as all. I am also worried that how we interact with each other is not always the way we wanted.
A good example is the RMS discussion on gitlab. A bad example and one where I am frustrated about is how we deal with PR's.

Since 2013 I have posted 57 PRs. 6 of those PR's have been closed without any reason. 9 of those PR's have status open. The oldest is open for 6 months and hold a promise from the core team they would come back on this PR. I am still waiting on this reaction.

I am open for feedback on my pr's and I am happy to close them when it is the wrong approach, wrong problem not a good solution etc... I am also open on feedback on my language, tone and behavior.

and for any native English speaker, please keep the slang and colloquial, it's fun having to check things in the urban dictionary.

Yes please use native sayings and be a little humble when miscommunications happen. (This is a note for myself).

…ry and add a button to view them to the recurring contributions tab
…ry and add a button to view them to the recurring contributions tab
…ry and add a button to view them to the recurring contributions tab
@mattwire mattwire merged commit 09042f5 into civicrm:master Jun 28, 2021
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