fix: Adds activity_id filter to answers dates query to fix problem with dataviz calendar (M2-8215) #1680

ramirlm · 2024-12-10T00:14:06Z

📝 Description

This change introduces a new (optional) field to the answers dates endpoint, which includes an additional filter to the query and removes incorrectly retrieved answers.

🪤 Peer Testing

Access Dataviz for Limited users, and search for activities with answers from limited users on different dates.
Only the answers for the user within that activity should be seen in the correct days.

✏️ Notes

As an optional field, this change shall not cause any incompatibility issues.

…ema to fix dataviz calendar issue

github-actions · 2024-12-10T00:18:54Z

➡️ Preview environment created: Click Me!

github-actions · 2024-12-10T00:18:56Z

❌ E2E tests failed

src/apps/answers/crud/answers.py

farmerpaul · 2024-12-10T15:58:31Z

src/apps/answers/filters.py

@@ -34,6 +34,7 @@ class AppletSubmissionsFilter(BaseQueryParams):
 class AppletSubmitDateFilter(BaseQueryParams):
    respondent_id: uuid.UUID | None
    target_subject_id: uuid.UUID | None
+    activity_id: str | None


I'm just checking in with @ChaconC about task definition. I think this endpoint should also accept an optional activity_flow_id. Stay tuned!

Or maybe use a field named activity_or_flow_id and in query use same code we are using in other endpoints

case( ( AnswerSchema.flow_history_id.isnot(None), AnswerSchema.id_from_history_id(AnswerSchema.flow_history_id) == str(activity_or_flow_id), ), else_=AnswerSchema.id_from_history_id(AnswerSchema.activity_history_id) == str(activity_or_flow_id), )

src/apps/answers/crud/answers.py

rcmerlo

You should add automated tests

farmerpaul · 2024-12-11T14:38:35Z

src/apps/answers/filters.py

@@ -34,6 +34,7 @@ class AppletSubmissionsFilter(BaseQueryParams):
 class AppletSubmitDateFilter(BaseQueryParams):
    respondent_id: uuid.UUID | None
    target_subject_id: uuid.UUID | None
+    activity_or_flow_id: str | None


Shouldn't this remain uuid.UUID to maintain validation rules? I don't see anywhere else in the codebase where we type activity_or_flow_id as str.

Were you running into a typing issue that prompted you to switch to str?

pleas see comment bellow

farmerpaul · 2024-12-11T14:44:43Z

src/apps/answers/crud/answers.py

+                    AnswerSchema.id_from_history_id(AnswerSchema.flow_history_id) == str(filters.activity_or_flow_id),  # noqa: E501
+                ),
+                else_=AnswerSchema.id_from_history_id(AnswerSchema.activity_history_id)  # noqa: E501
+                == str(filters.activity_or_flow_id),


I'm not clear why we need to convert the UUID to string. The id_from_history_id functions return a UUID | none type, so it should be fine to do quality comparison against another UUID | none type, no? Maybe I'm missing something…

Suggested change

AnswerSchema.id_from_history_id(AnswerSchema.flow_history_id) == str(filters.activity_or_flow_id), # noqa: E501

),

else_=AnswerSchema.id_from_history_id(AnswerSchema.activity_history_id) # noqa: E501

== str(filters.activity_or_flow_id),

AnswerSchema.id_from_history_id(AnswerSchema.flow_history_id) == filters.activity_or_flow_id,

),

else_=AnswerSchema.id_from_history_id(AnswerSchema.activity_history_id) == filters.activity_or_flow_id,

@farmerpaul If you see this field in the database, the uuid is saved with a version in the end, causing an error parsing as uuid, that's why I had to use string instead

yes, but filters.activity_or_flow_id should be UUID, because we should validate it by pydantic, we can not accept a free format string, so original code should work, calling str(filters.activity_or_flow_id) will have same behavior

please, do not use # noqa: E501 you can break the line, use make cqf to automatically fix this, ignoring a lint or mypy error should be always the last solution

causing an error parsing as uuid, that's why I had to use string instead

@ramirlm I am not seeing any errors related to the types in my VS Code when using UUID. I'm also not seeing any errors that suggest that # noqa: E501 is needed anywhere. With the code I suggested above, everything works fine, no issues. The reason make cqf isn't making any formatting changes is because there's no issue with the formatting, at least on my end.

I'm curious where you're seeing errors @ramirlm – is it in VS Code? Perhaps there's a configuration issue? 🤔

@farmerpaul this is the error

Oh, you're talking about a runtime error. I was only talking about pylance/linting errors till now, but I've reproduced your runtime error.

The error you showed is actually the result of the str() coercion above, which is what I was trying to point out is not correct. The str() is coercing None (when the parameter is omitted) to a zero-length string '', which is not a well-formed UUID string.

When I updated the code to what I suggested above – without the str() coercion:

( AnswerSchema.flow_history_id.isnot(None), AnswerSchema.id_from_history_id(AnswerSchema.flow_history_id) == filters.activity_or_flow_id, ), else_=AnswerSchema.id_from_history_id(AnswerSchema.activity_history_id) == filters.activity_or_flow_id,

the error went away.

Are you seeing something different?

ok, now I understood the problem here,

without str() in filters.activity_or_flow_id, we have a comparison between str resulted from AnswerSchema.id_from_history_id() and UUID, this will not work, we should keep str()

We should include this where clause only when filters.activity_or_flow_id is not None, it make more sense to me, because it is a optional filter

@rcmerlo I don't quite follow your first bullet. According to my VS Code, AnswerSchema.id_from_history_id returns a UUID or None, not a string:

So I don't see the reason for keeping str().

However, I agree we should omit this clause altogether if activity_or_flow_id is None.

Ok, you are right, I was confused because of hybrid_method decorator from SQLAlchemy used in this method

rcmerlo · 2024-12-12T12:57:00Z

src/apps/answers/filters.py

@@ -34,6 +34,7 @@ class AppletSubmissionsFilter(BaseQueryParams):
 class AppletSubmitDateFilter(BaseQueryParams):
    respondent_id: uuid.UUID | None
    target_subject_id: uuid.UUID | None
+    activity_or_flow_id: str | None


pleas see comment bellow

rcmerlo · 2024-12-12T12:58:25Z

src/apps/answers/tests/test_answers_arbitrary.py

@@ -295,6 +297,7 @@ async def test_list_submit_dates(
                respondentId=tom.id,
                fromDate=datetime.date.today() - datetime.timedelta(days=10),
                toDate=datetime.date.today() + datetime.timedelta(days=10),
+                activityOrFlowId=applet.activities[0].id,
            ),


all tests are using only activites, please add one with a activity flow

rcmerlo · 2024-12-12T13:01:11Z

src/apps/answers/crud/answers.py

+                    AnswerSchema.id_from_history_id(AnswerSchema.flow_history_id) == str(filters.activity_or_flow_id),  # noqa: E501
+                ),
+                else_=AnswerSchema.id_from_history_id(AnswerSchema.activity_history_id)  # noqa: E501
+                == str(filters.activity_or_flow_id),


please, do not use # noqa: E501 you can break the line, use make cqf to automatically fix this, ignoring a lint or mypy error should be always the last solution

ramirlm · 2024-12-12T13:05:12Z

@rcmerlo make cfq did not fix this issue, that's why I added the noqa command. Any suggestions? Thanks!

ramirlm · 2024-12-16T12:41:54Z

It will be necessary to change the reviews and flows endpoints as well. This is back to draft
CC @ChaconC @rcmerlo

…bug on summary and answers page

…nswers

filter: add activity_id filter to answers query and update filter sch…

3b7a2cb

…ema to fix dataviz calendar issue

rcmerlo requested changes Dec 10, 2024

View reviewed changes

src/apps/answers/crud/answers.py Show resolved Hide resolved

fix: change schema query to make more appropriate

400a0c6

ramirlm changed the title ~~fix: Adds activity_id filter to answers dates query to fix problem with dataviz calendar (M2-8215)~~ fix: Adds activity_id filter to answers dates query to fix problem with dataviz calendar (M2-8216) Dec 10, 2024

ramirlm changed the title ~~fix: Adds activity_id filter to answers dates query to fix problem with dataviz calendar (M2-8216)~~ fix: Adds activity_id filter to answers dates query to fix problem with dataviz calendar (M2-8215) Dec 10, 2024

ramirlm mentioned this pull request Dec 10, 2024

fix: fixes datepicker issue within dataviz (M2-7348) ChildMindInstitute/mindlogger-admin#1987

Draft

farmerpaul self-requested a review December 10, 2024 15:56

farmerpaul reviewed Dec 10, 2024

View reviewed changes

fix: update activity_id filter to support activity or flowid

41debc8

rcmerlo approved these changes Dec 10, 2024

View reviewed changes

src/apps/answers/crud/answers.py Show resolved Hide resolved

rcmerlo requested changes Dec 10, 2024

View reviewed changes

Ramir Mesquita added 4 commits December 10, 2024 20:38

fix: improving logic for querying flux or activity id

cd6af87

fix: ignoring ruff error 501 in lines

6e3483b

fix: adds ruff ignore rules for long lines

588d2a7

fix: changing activity_or_flow_id to str to fix the query

d1ca4d3

farmerpaul reviewed Dec 11, 2024

View reviewed changes

Ramir Mesquita added 3 commits December 12, 2024 08:00

fix: adding missing param to the test

e57a58f

fix: adding missing answer or flow id arg to test

8a205f9

fix: adding missing answer or flow id arg to test to arbitrary test

7bda73f

ramirlm requested review from rcmerlo and farmerpaul December 12, 2024 12:00

rcmerlo reviewed Dec 12, 2024

View reviewed changes

fix: adds more answer tests to cover flow id on activityOrFlowId field

4a605a3

ramirlm marked this pull request as draft December 16, 2024 12:41

Ramir Mesquita added 2 commits December 26, 2024 10:35

fix: enhance query logic to filter answers by flow history id to fix …

c9e5a35

…bug on summary and answers page

fix: ensure flow_history_id is checked before filtering versions in a…

58a3baa

…nswers

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Adds activity_id filter to answers dates query to fix problem with dataviz calendar (M2-8215) #1680

fix: Adds activity_id filter to answers dates query to fix problem with dataviz calendar (M2-8215) #1680

ramirlm commented Dec 10, 2024

github-actions bot commented Dec 10, 2024

github-actions bot commented Dec 10, 2024

farmerpaul Dec 10, 2024

rcmerlo Dec 10, 2024

rcmerlo left a comment

farmerpaul Dec 11, 2024

rcmerlo Dec 12, 2024

farmerpaul Dec 11, 2024

ramirlm Dec 12, 2024

rcmerlo Dec 12, 2024

rcmerlo Dec 12, 2024

farmerpaul Dec 12, 2024

ramirlm Dec 16, 2024

farmerpaul Dec 16, 2024 •

edited

Loading

rcmerlo Dec 16, 2024

farmerpaul Dec 16, 2024

rcmerlo Dec 16, 2024

rcmerlo Dec 12, 2024

rcmerlo Dec 12, 2024

rcmerlo Dec 12, 2024

ramirlm commented Dec 12, 2024

ramirlm commented Dec 16, 2024

fix: Adds activity_id filter to answers dates query to fix problem with dataviz calendar (M2-8215) #1680

Are you sure you want to change the base?

fix: Adds activity_id filter to answers dates query to fix problem with dataviz calendar (M2-8215) #1680

Conversation

ramirlm commented Dec 10, 2024

📝 Description

🪤 Peer Testing

✏️ Notes

github-actions bot commented Dec 10, 2024

github-actions bot commented Dec 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rcmerlo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

farmerpaul Dec 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ramirlm commented Dec 12, 2024

ramirlm commented Dec 16, 2024

farmerpaul Dec 16, 2024 •

edited

Loading