OpenTechStrategies / lisc-ttm

LISC TTM code. See https://ttm.lisc-chicago.org/.
GNU Affero General Public License v3.0
1 stars 4 forks source link

Enlace: Survey completion calc incorrect #65

Closed MegFord closed 9 years ago

MegFord commented 10 years ago

Reported to Chapin Hall Helpdesk on April 9, 2014. On a program's page, the surveys completion calculation on the left side of the page appears to be adding too many in the denominator (see [production data redacted]). This bug was originally reported in Issue #58

kevinrak9 commented 10 years ago

Hello,

I can't speak to the code that would reproduce the error, but the attached screenshot enlace geek squad testing program page screenshot is an example from our live site. (Note: This is our test program page that does not contain sensitive information). As you can see, we have 4 participants registered, but the database thinks there should be 6 program quality surveys. Hope that helps, and let me know if you need any more information.

Also, I would say this is a low priority from our perspective. It would help keep things cleaner to have them calculate correctly, but it's not seriously getting in the way at the moment.

Thanks, Kevin

MegFord commented 10 years ago

Thanks! I'll take another look at this. The screenshot should be enough for me to duplicate the issue, I'll comment here if I need more info.

kfogel commented 10 years ago

Thanks for the screenshot, Kevin. Understood about prioritization; this is also partly about familiarizing Meg with the code -- she and I selected issues based on that as well as on absolute priority. Tracing down where that "0/6" comes from (when it should be "0/4" because 4 participants) is a good chance to learn what's happening on the back end.

By the way, Meg, Kevin's screenshot above also happens to show the kind of Program page that is being referred to in https://github.com/OpenTechStrategies/lisc-ttm/issues/64#issuecomment-55629106 -- the upper left sub-rectangle that has the little Edit button at its bottom is the section being referred to in that issue. (Mentioning it in case it helps to have a visual guide.)

MegFord commented 10 years ago

This bug is a little odd and I'm not sure which query actually needs to be fixed. The query (lines 458-62) which is getting the participants shown on the right hand side, under participants, returns fewer participants : Query 1 SELECT * FROM Participants_Programs INNER JOIN Participants ON Participants.Participant_ID=Participants_Programs.Participant_ID INNER JOIN Session_Names ON Participants_Programs.Program_ID=Session_ID WHERE Session_Names.Program_ID='" . $program->program_id . "' ORDER BY Session_ID, Participants.Last_Name";

than the query on line 336 which returns participants based on session and program: Query 2 $get_people = "SELECT COUNT(*) FROM Participants_Programs WHERE Program_Id=$sesh[0]";

However, here is the confusing part: When I run this db query Query 3 SELECT * FROM participants_programs; I see that the second query is returning the correct number of participants for the program but the first query is not getting all of them for display.

Can anyone verify this, or tell me what I am missing here?

MegFord commented 10 years ago

I think the inconsistency is actually caused by the Participant_ID field being set to zero. That is why the first query doesn't get the participant but the second does. So the question I have is, since it appears that this is caused by inconsistent data, what is the best way to fix it?

kfogel commented 10 years ago

Hmmm. Good questions. My head is too deep in issue #68 right now for me to come out and look at this code with the attention it needs, so I'd like to defer it until @cecilia-donnelly is back tomorrow morning. Is that okay by you, Meg?

cecilia-donnelly commented 10 years ago

Hi Meg! Option 1: I think if the first query is being broken by inconsistent data, then it might work to substitute "LEFT JOIN Participants" where we currently have "INNER JOIN Participants." After all, if the participant has an id of zero then s/he wouldn't be present in the Participants table, so an inner join would remove that row. Right? I'm guessing that would cause the first query to also return 6 in the example above.

Option 2: Mind you, do we want to count participants with ids of zero? They don't really exist. Another way to ensure consistency would be to replace Query 2 with "SELECT COUNT( * ) FROM Participants_Programs WHERE Program_ID=$sesh[0] AND Participant_ID != '0'" or IS NOT NULL or something like that - or even "SELECT COUNT( * ) FROM Participants_Programs INNER JOIN Participants ON Participants_Programs.Participant_ID=Participants.Participant_ID WHERE Program_ID=$sesh[0]" for greatest consistency.

I prefer the second option in some ways, since "participants" with IDs of zero shouldn't be counted. However, the first one presents everything we have, and then users can decide to throw out the non-participants. @kevinrak9? Thoughts?

Hope this helps!

kevinrak9 commented 10 years ago

I can't really speak to which code would be better, but I would imagine we would not want to count participants whose ID is zero.

Where I am getting confused is how/why there are participants whose ID is zero. When I run a search for all participants on the production site (https://ttm.lisc-chicago.org/enlace/participants/participants.php, leaving all fields blank, clicking on search), the database does not return any participants with an ID of zero. What I am wondering, then, is where did the participants with IDs of zero come from, where are they stored, and do they have names and other information attached to them? Sorry for my confusion; I just want to make sure we're not losing information.

cecilia-donnelly commented 10 years ago

That makes total sense. No, participants with an ID of zero would not have names and other information attached to them. I have a few ideas about where they might have come from.

  1. If, while testing, a person was linked to the Geek Squad Testing program and then that person was deleted, their ID in the Participants_Programs table could be converted to zero (unlikely, because we delete information related to people when they are deleted).
  2. If, while testing, we used an incorrect query that did not save the person's ID, the table would have filled it in as zero automatically (this seems less likely with the production data, but not impossible).
  3. Most likely: When users are adding participants to a program, they might accidentally click save without choosing a participant, in which case the "participant" would be saved as an id of zero. That's actually a bug, since we shouldn't allow the save to happen without a participant chosen.

Does that help?

kevinrak9 commented 10 years ago

That does help, thanks. I imagine it would be No. 3 because we have so many different users, and I know some of them have not gotten the hang of adding participants on the first try.

MegFord commented 10 years ago

That sounds fine Cecilia, I fixed the issue using the second query you mentioned. I wasn't sure what an ID of zero meant exactly, thanks!

I pushed the fix to a new branch.

cecilia-donnelly commented 9 years ago

Merged this branch into master on 12/5. I'll pull to prod mirror, alert @kevinrak9, and we can close this!

cecilia-donnelly commented 9 years ago

Pushed to production and user @kevinrak9 confirmed that all is well.