OpenTechStrategies / streetcrm

StreetCRM is a free software contact management application
Other
5 stars 4 forks source link

Add simple reporting features #336

Closed pjsier closed 6 years ago

pjsier commented 7 years ago

Closes #332. Adds a count of "Unique Participants" to event results (changes previous "Unique" header to just "Participants"), adds a count of attendances per action to the CSV export, and allows for filtering on exact number of events attended in the filtered time period.

My main question is if the attendance count filter should be "attended at least this many" actions, or whether we should some way of either searching for an exact count or a range

cecilia-donnelly commented 7 years ago

Thanks, @pjsier.

We still need the first item from #332: "Participant results should include total # of actions that each participant in the result set attended"

As far as your question:

My main question is if the attendance count filter should be "attended at least this many" actions, or whether we should some way of either searching for an exact count or a range

I like the idea of "at least this many" but probably they'll want to do an exact count in some cases. We can do "x or more" and see what feedback we get. If the export has the exact count then they'd also be able to sort there.

pjsier commented 7 years ago

@cecilia-donnelly ah I had added the functionality but not the export columns, thanks for catching that. And that works for me, so I changed the event count filtering to greater than or equal to

cecilia-donnelly commented 7 years ago

@pjsier The "count attendances" is there now, but it was empty. I had searched for participants that had attended at least one action, so presumably they should've all had a value in that column.

pjsier commented 7 years ago

Sorry about missing that, I tried to do more thorough testing this time around. That bug was because leadership level wasn't added at all if it wasn't found, so the attendance count displayed in that column. There were also some issues with generating attendance counts on participants, and displaying search parameters in the export file that are fixed here. Here's what I tested against, so let me know if I'm missing any edge cases:

Export

Participant search

Event search

Also, there are two things I wanted to flag.

If you see any obvious tests cases that I'm missing here let me know and I'll check those before any other review!

cecilia-donnelly commented 6 years ago

@pjsier this is a great list! I wouldn't have thought of the case where a participant is removed/re-added to an event. Thanks very much for writing these up.

If you filter with 0 attendances or more and a start or end date on the participant search, it won't pull all participants, only those with at least one attendance in that range (because the event filter is applied first). I don't see why people would check for 0 attendances in a time period, but wanted to flag it in case you thought a workaround was necessary or if we should set the input field to have a minimum of 1.

I like the idea of having a minimum of 1. That gives them something obvious to ask about in case they do want to search for zero attendances in a time period, instead of mysteriously getting incorrect results. Can you make that change?

Events without dates will be excluded from any search that specifies a date filter, which also seems expected but I wanted to check to be sure.

Yup, makes perfect sense. Thanks for flagging it!

pjsier commented 6 years ago

@cecilia-donnelly great! Just updated the minimum value in the attendance field and it's setting the min attribute on the input. I'm guessing the built in browser functionality for validating that is enough, and we won't be adding any polyfills for browsers that don't support that

cecilia-donnelly commented 6 years ago

This looks good to me! Thanks for making that change, @pjsier. I changed the column headers in https://github.com/OpenTechStrategies/streetcrm/commit/7453aea10be18fff465d0e1372870c366b5754d9. Would you cherry-pick that commit into your branch? I found having the same "Count attendances" title for both people and actions was a little confusing.

Then I think we're good to merge!

pjsier commented 6 years ago

Makes sense, just merged that in, thanks!