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

feature: New endpoint to return total counters for subject (M2-7889) #1669

Merged
merged 24 commits into from
Dec 9, 2024

Conversation

rcmerlo
Copy link
Contributor

@rcmerlo rcmerlo commented Nov 29, 2024

  • Tests for the changes have been added
  • Related documentation has been added / updated
  • For new features, QA automation engineers have been tagged
  • OSS packages added to MindLogger open source credit page

📝 Description

🔗 Jira Ticket M2-7889

New endpoint /activities/applet/{applet_id}/subject/{subject_id}/metadata to return activities metadata for subjects as target and respondent, including a list of activities with submissions counts and subjects counts

{
  "result" : {
    "subjectId" : "729f0e37-9818-4862-92d2-b228fc903827",
    "respondentActivitiesCount" : 4,
    "targetActivitiesCount" : 4,
    "activitiesOrFlows" : [ {
      "activityOrFlowId" : "68df4c64-323e-4961-a562-dd6478f5df59",
      "respondentsCount" : 0,
      "respondentSubmissionsCount" : 0,
      "subjectsCount" : 0,
      "subjectSubmissionsCount" : 0
    }, {
      "activityOrFlowId" : "b97de63c-0923-4011-99e6-c045a6b15a09",
      "respondentsCount" : 0,
      "respondentSubmissionsCount" : 0,
      "subjectsCount" : 0,
      "subjectSubmissionsCount" : 0
    }, {
      "activityOrFlowId" : "4379ad52-7dd5-40a8-ba80-5c82e7ab7333",
      "respondentsCount" : 0,
      "respondentSubmissionsCount" : 0,
      "subjectsCount" : 0,
      "subjectSubmissionsCount" : 0
    }, {
      "activityOrFlowId" : "aa6ca02c-4987-4b13-ac10-307306d99e2f",
      "respondentsCount" : 0,
      "respondentSubmissionsCount" : 0,
      "subjectsCount" : 0,
      "subjectSubmissionsCount" : 0
    }, {
      "activityOrFlowId" : "6eda8154-2e6f-48fa-a426-ae4032c8b607",
      "respondentsCount" : 0,
      "respondentSubmissionsCount" : 0,
      "subjectsCount" : 0,
      "subjectSubmissionsCount" : 0
    }, {
      "activityOrFlowId" : "366a9e07-7a48-4ddc-9022-fd96b88d66ea",
      "respondentsCount" : 0,
      "respondentSubmissionsCount" : 0,
      "subjectsCount" : 0,
      "subjectSubmissionsCount" : 0
    } ]
  }
}

@rcmerlo
Copy link
Contributor Author

rcmerlo commented Nov 29, 2024

@Phillipe-Bojorquez New endpoint

Copy link

github-actions bot commented Nov 29, 2024

➡️ Preview environment failed to be destroyed

Copy link

❌ E2E tests failed

@rcmerlo rcmerlo force-pushed the M2-7889-R2-BE-PDP-Totals-Endpoint branch from 78b07be to f083e6e Compare November 29, 2024 15:13
@rcmerlo rcmerlo requested a review from mbanting December 2, 2024 17:05
@rcmerlo rcmerlo marked this pull request as ready for review December 2, 2024 17:12
Copy link
Contributor

@farmerpaul farmerpaul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a few initial comments, but I have more to add, just getting these out 🙂

src/apps/activities/router.py Outdated Show resolved Hide resolved
src/apps/activities/api/activities.py Outdated Show resolved Hide resolved
src/apps/activities/api/activities.py Outdated Show resolved Hide resolved
Copy link
Contributor

@mbanting mbanting left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Preliminary testing

@rcmerlo @farmerpaul can we align on the property semantics, to make sure this endpoint is being developed, tested and integrated with the same understanding? Do the following Definitions align with your understanding?

Dec 3: Definitions and test results updated after aligning with @farmerpaul's comment below.

Definitions

  • respondentActivitiesCount / respondentActivitiesCountExisting: Total number of Activities/Flows that have ever been completed by participant or have been assigned to this participant as the respondent.
  • targetActivitiesCount/ targetActivitiesCountExisting: Total number of Activities/Flows that have ever been completed about participant or have been assigned to this participant as the target.
  • respondentsCount: Total number of respondents that have completed the activity/flow about the participant or are assigned the activity/flow as a respondent about the participant
  • subjectSubmissionsCount: Total number of submissions completed about the participant for this activity/flow
  • subjectsCount: Total number of subjects the participant has completed the activity/flow about or the participant is assigned to as a respondent for the activity/flow
  • respondentSubmissionsCount: Total number of submissions the participant has completed for this activity/flow

Tests

Dec 3
  • 1 flow (not auto assigned)
  • 3 activities (2 auto assigned, 1 manually assigned)
Property Expected Actual Result
respondentActivitiesCount 2 2
targetActivitiesCount 2 2
  • completed the first auto-assigned activity
Property Expected Actual Result
activityOrFlowId 3f6c92ef 3f6c92ef
respondentsCount 1 1
respondentSubmissionsCount 1 1
subjectsCount 1 1
subjectSubmissionsCount 1 1
  • completed the first auto-assigned activity again
Property Expected Actual Result
activityOrFlowId 3f6c92ef 3f6c92ef
respondentsCount 1 1
respondentSubmissionsCount 2 1
subjectsCount 1 1
subjectSubmissionsCount 2 1
  • added limited account subject
  • Performed Take Now on first auto-assigned activity for limited account
Property Expected Actual Result
activityOrFlowId 3f6c92ef 3f6c92ef
respondentsCount 1 1
respondentSubmissionsCount 3 2 ✅ (mismatch due to previous test)
subjectsCount 2 2
subjectSubmissionsCount 2 1 ✅ (mismatch due to previous test
  • Performed Take Now on manually assigned (currently unassigned) activity for limited account
Property Expected Actual Result
respondentActivitiesCount 3 2
targetActivitiesCount 2 3
activityOrFlowId 96adf4c0 96adf4c0
respondentsCount 0 0
respondentSubmissionsCount 1 1
subjectsCount 1 1
subjectSubmissionsCount 0 0
Dec 5
  • 1 flow (not auto assigned)
  • 3 activities (2 auto assigned, 1 manually assigned)
Property Expected Actual Result
respondentActivitiesCountExisting 2 2
targetActivitiesCountExisting 2 2
  • completed the first auto-assigned activity
Property Expected Actual Result
activityOrFlowId c979189136f3 c979189136f3
respondentsCount 1 1
respondentSubmissionsCount 1 1
subjectsCount 1 1
subjectSubmissionsCount 1 1
  • completed the first auto-assigned activity again
Property Expected Actual Result
activityOrFlowId c979189136f3 c979189136f3
respondentsCount 1 1
respondentSubmissionsCount 2 2
subjectsCount 1 1
subjectSubmissionsCount 2 2
  • added limited account subject
  • Performed Take Now on first auto-assigned activity for limited account
Property Expected Actual Result
activityOrFlowId c979189136f3 c979189136f3
respondentsCount 1 1
respondentSubmissionsCount 3 3
subjectsCount 2 2
subjectSubmissionsCount 2 2
  • Performed Take Now on manually assigned (currently unassigned) activity for limited account
Property Expected Actual Result
respondentActivitiesCountExisting 3 3
targetActivitiesCountExisting 2 2
activityOrFlowId 15e2486d5d3e 15e2486d5d3e
respondentsCount 0 0
respondentSubmissionsCount 1 1
subjectsCount 1 1
subjectSubmissionsCount 0 0
  • Assigned manual flow to participant for limited account
Property Expected Actual Result
respondentActivitiesCountExisting 4 4
targetActivitiesCountExisting 2 2
  • Completed flow for limited account
Property Expected Actual Result
activityOrFlowId f03f9d63708f f03f9d63708f
respondentsCount 0 0
respondentSubmissionsCount 2 (TBC) 2
subjectsCount 1 1
subjectSubmissionsCount 0 0

@farmerpaul
Copy link
Contributor

Hi Marty, thanks for doing all that testing and sharing your results in such detail. I've been seeing similar results locally. I think two of the definitions you listed above I would have interpreted differently:

respondentSubmissionsCount: Total number of submissions completed about the participant for this activity/flow

I think in the code, it's the submissions completed by the participant. "respondent" here is suggesting that the passed subject is the respondent.

subjectSubmissionsCount: Total number of submissions the participant has completed for this activity/flow

And conversely, what's being calculated here is submissions completed about the participant, "subject" here suggesting that the passed subject is the target subject.

@mbanting
Copy link
Contributor

mbanting commented Dec 3, 2024

respondentSubmissionsCount: Total number of submissions completed about the participant for this activity/flow

I think in the code, it's the submissions completed by the participant. "respondent" here is suggesting that the passed subject is the respondent.

@farmerpaul thanks, glad we can get on the same page. I wavered back and forth between guessing what each meant. I ultimately thought respondentSubmissionsCount was about the participant as the target because respondentsCount is. Conversely the same thinking with subjectSubmissionsCount.

I'll flip the definitions and test results in my comment above to align with how you and @rcmerlo defined these.

@farmerpaul farmerpaul self-assigned this Dec 3, 2024
@farmerpaul farmerpaul self-requested a review December 3, 2024 17:27
Copy link
Contributor

@farmerpaul farmerpaul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @rcmerlo, I've been working on some of the remaining discrepancies in the results returned by this endpoint, and I've come up with 3 fixes. (These fixes are needed in addition to what you're currently working on with respect to activities/flows that only have assignments, and no submissions.)

You should just be able to plug these fixes into your current PR. Let me know if you have any questions!

src/apps/answers/crud/answers.py Outdated Show resolved Hide resolved
src/apps/answers/crud/answers.py Show resolved Hide resolved
src/apps/answers/service.py Show resolved Hide resolved
@farmerpaul farmerpaul self-requested a review December 3, 2024 20:58
@rcmerlo
Copy link
Contributor Author

rcmerlo commented Dec 3, 2024

@mbanting @farmerpaul I did a big refactoring in code to get correct data, I'm still working on automated tests, but please check again

@rcmerlo rcmerlo requested a review from mbanting December 4, 2024 13:50
@farmerpaul farmerpaul self-requested a review December 4, 2024 15:45
@rcmerlo rcmerlo force-pushed the M2-7889-R2-BE-PDP-Totals-Endpoint branch from d022b47 to 7c7a88c Compare December 4, 2024 17:39
Copy link
Contributor

@farmerpaul farmerpaul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discovered a few more issues around deleted subjects, see comments below.

Also, there's something else that seems to be a problem since your last change; all the submissions counts are now coming back as either 0 or 1 for me. I haven't dug down as to what's going on there…

src/apps/answers/service.py Outdated Show resolved Hide resolved
src/apps/answers/service.py Outdated Show resolved Hide resolved
src/apps/activity_assignments/service.py Show resolved Hide resolved
@rcmerlo rcmerlo requested a review from farmerpaul December 4, 2024 19:50
Copy link
Contributor

@farmerpaul farmerpaul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My last 2 comments for now! Let me know if you have any questions…

src/apps/answers/crud/answers.py Show resolved Hide resolved
src/apps/answers/service.py Outdated Show resolved Hide resolved
farmerpaul
farmerpaul previously approved these changes Dec 5, 2024
Copy link
Contributor

@farmerpaul farmerpaul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking good to me know, @rcmerlo! Thanks for making all those changes, I know it's been a slog. And some of them had nothing to do with your code. Really appreciate it!

@mbanting can you re-run your test cases now to confirm things are working as expected?

@farmerpaul farmerpaul dismissed their stale review December 5, 2024 14:43

I just discovered there's an issue with the total activites count on the By Participant tab for limited accounts – investigating!

@farmerpaul
Copy link
Contributor

farmerpaul commented Dec 6, 2024

I noticed that when completing an activity flow, respondentSubmissionsCount isn't incremented by 1, instead it is incremented by the number of activities in the flow. That seems to make sense given answers are submitted by activity, but just wanted to make sure that was expected?

Good question, @mbanting. I'd actually assumed each flow submission should count as "1" in each of the submissions counts, but now I'm not so sure. If you go to the Applet Overview tab, then it appears that submissions are only listed on the activity level, not the activity flow level. In fact, there's not even any mention of submitted activity flows on the Applet Overview tab, which doesn't really make a whole lot of sense to me. Shouldn't it be recorded somewhere that the flow was also completed? Hmm…

I'm checking again now, and it appears that the endpoint used to list the expanded view of activity flows on the By Participant tab treats each flow submission as "1 submission" – that is, the submissions column of the expanded view for activity flows is not adding up to the total count of submission for the activity flow as a whole (coming from the metadata endpoint). These numbers need to align, as to have those counts not add up is very confusing, and QA would definitely have a problem with it!

To align with the Applet Overview tab, let's count submissions on the activity level only. If we're changing how submissions are counted, that should be taken up with product (which I think we should do – I'll take that on), but for now let's just make sure all the counts are in agreement.

@rcmerlo I'm going to find the offending part of the SQL that's causing the counts to not add up correctly in the expanded view of activity flows. Stay tuned…

After chatting with @rcmerlo more, I felt it made more sense to count unique submit_ids for each activity flow – so, treating the expanded view showing correct submission counts, and updating the code for the metadata endpoint to align with those counts. Will push up that change. However, I've also reached out to product to confirm.

@farmerpaul farmerpaul force-pushed the M2-7889-R2-BE-PDP-Totals-Endpoint branch from 2008eac to e9e5282 Compare December 6, 2024 15:49
@farmerpaul farmerpaul force-pushed the M2-7889-R2-BE-PDP-Totals-Endpoint branch from e9e5282 to 05008b5 Compare December 6, 2024 15:57
For all the PDP queries, a "submission" for a flow should only be
**completed** ones. A flow that has only been partially completed (say,
only the 1st activity of 5 activities have been submitted) should not
count towards having any submissions, and thus should NOT be:

- listed on the PDP pages for the associated subjects
- included in the tallying of activities/flows for that tab
- included in the submissions count on the flow level within each tab
- included in that flow's expanded view's submissions counts
@farmerpaul
Copy link
Contributor

farmerpaul commented Dec 6, 2024

After discussions with product, it was agreed that a "submission" for a flow = a completed assessment of that flow, where all activities contained in that assessment were submitted. This corresponds to records in the answers table whose is_flow_completed column is set to TRUE (if it's an activity flow).

With my latest commit, all these endpoints should now be taking this into consideration in their querying against the answers table:

  • /activities/applet/{applet_id}/target/{subject_id}
  • /activities/applet/{applet_id}/respondent/{subject_id}
  • /subjects/respondent/{respondent_subject_id}/activity-or-flow/{activity_or_flow_id}
  • /activities/applet/{applet_id}/subject/{subject_id}/metadata

So for example, if you create a new activity flow with 2 activities, and it's unassigned (not auto-assigned nor manually assigned), and you do Take Now but only complete the 1st activity, it should NOT be listed on the PDP for the associated subject. Nor should any other such partial activity flow "submissions" count towards the totals shown on the PDP.

I've confirmed locally but feel free to do further testing. We should probably add specific unit tests for this as well (@rcmerlo feel free to take this on when you get back 😅🙏🏻).

@rcmerlo rcmerlo merged commit f6990e6 into develop Dec 9, 2024
3 of 5 checks passed
@rcmerlo rcmerlo deleted the M2-7889-R2-BE-PDP-Totals-Endpoint branch December 10, 2024 00:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants