eileenmcnaughton / nz.co.fuzion.extendedreport

Extended Reports for CiviCRM
GNU Affero General Public License v3.0
20 stars 57 forks source link

Merged / Deleted Contacts Appear in Membership Pivot Report #295

Closed amyers735 closed 1 year ago

amyers735 commented 5 years ago

I use the Membership Pivot Report to report totals of various membership statues against a Custom Field on my contact. I noticed the totals were larger in the Pivot Report than what I made them using my previous method, and it appears the cause is because I have a number of merged contacts which were being counted.

I believe a fix may be to add Contact "is_deleted" as a filter option, and am about to submit a pull request that does this. Although I'm not fully sure whether this will have any other consequences.

stale[bot] commented 4 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

francescbassas commented 4 years ago

We just ran into this bug. @am2605 Do you have any PR that can be applied?

amyers735 commented 4 years ago

The PR I submitted never got merged because it caused some tests to fail. I didn’t understand why so I wasn’t able to progress it any further - hopefully one of the project leads might be able to work it out

francescbassas commented 4 years ago

@am2605 https://github.com/eileenmcnaughton/nz.co.fuzion.extendedreport/pull/296 works for you?

amyers735 commented 4 years ago

@am2605 #296 works for you?

I haven’t really looked I’m sorry, so I can’t confirm

stale[bot] commented 3 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

eileenmcnaughton commented 3 years ago

I just merged a fix on this one

francescbassas commented 3 years ago

It seems that https://github.com/eileenmcnaughton/nz.co.fuzion.extendedreport/pull/296 not solves this issue. I just reproduced with the 5.10 extension's version. @eileenmcnaughton can we reopen it?

eileenmcnaughton commented 3 years ago

@francescbassas a lot of this stuff can done using search kit now

francescbassas commented 3 years ago

@eileenmcnaughton a membership report crossing membership type and membership statuses?

In any case I could propose and test something based on https://github.com/eileenmcnaughton/nz.co.fuzion.extendedreport/pull/296.

stale[bot] commented 2 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

eileenmcnaughton commented 2 years ago

@francescbassas did you want to put up a patch for this still?

francescbassas commented 2 years ago

@eileenmcnaughton I have noticed that the problem is no longer valid. Since #296 you can set value for the filter "is deleted".

However, I think it would be of great help and would resemble the behavior of the core reports, if by default all reports had the values of "is deleted" and "is deceased" set to false.

I checked locally to set this default filters here and worked. I have doubts that it is the right place. Especially with regard to the "is deceased" filter.

eileenmcnaughton commented 2 years ago

@francescbassas I guess it should be set in getContactColumns - the gotcha is that if the filter is set it means the contact table will be joined in (usually a good thing but I think there are some edge cases - eg. when it is joined in via another table & has the then effect of inner joining in that table)

francescbassas commented 2 years ago

@eileenmcnaughton but I don't see any reason why someone by default would want include to search deceased or deleted contacts. And if it is being done I am sure that in most cases it must be due to ignorance and causing unwanted results (as was my case).

eileenmcnaughton commented 2 years ago

@francescbassas I think it's safer passed into the getColumns function - otherwise you risk say it adding the soft credit contact with those filters - and then excluding rows with no soft credti contact

francescbassas commented 2 years ago

Thanks @eileenmcnaughton. I really hadn't thought about all the casuistry. Seeing it like this, I would only think of leaving the "is deleted" filter to "no" by default. Set "is deceased" filter to "no" could backfire in many cases. For example in the case of a donation summary.

In any case, I think this ticket can be closed. And if you consider it appropriate, another could be opened to finish maturing setting "is deleted" filter to no by default.

eileenmcnaughton commented 2 years ago

@francescbassas just noting that I don't think there is much that extended reports does that can't be done by search kit now

francescbassas commented 2 years ago

@eileenmcnaughton pivot tables are out of scope of Search Kit (at least for now) https://chat.civicrm.org/civicrm/pl/eg1mjf33abdefd955jzcmxhrdc

eileenmcnaughton commented 2 years ago

Oh yeah I guess that's true

stale[bot] commented 2 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

eileenmcnaughton commented 2 years ago

well since this discussion Pivot tables have been added to search kit - targetting 5.51

stale[bot] commented 1 year ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

francescbassas commented 5 months ago

See https://chat.civicrm.org/civicrm/pl/ta6or5o63383uqhzgs5a4dsxka