Create mention notification for case worker when exporter responds #1805
Conversation
hnryjmes
changed the title
hnryjmes
changed the title
hnryjmes
force-pushed
the
LTD-4620_Create_mention_notification_for_case_worker
branch
from
848dcf9 to
f2f2b8e
Compare
hnryjmes
changed the title
hnryjmes
changed the title
hnryjmes
changed the title
hnryjmes
force-pushed
the
LTD-4620_Create_mention_notification_for_case_worker
branch
2 times, most recently
from
be0da09 to
3915ddf
Compare
hnryjmes
changed the title
hnryjmes
force-pushed
the
LTD-4620_Create_mention_notification_for_case_worker
branch
from
18b9e4d to
6edb861
Compare
|
The author of this PR, hnryjmes, is not an activated member of this organization on Codecov. |
api/cases/views/views.py
Outdated
| ecju_query.case.create_system_mention( | ||
| case_note_text=f"{exporter_user_full_name} has responded to a query.", | ||
| mention_user=ecju_query.raised_by_user, | ||
| ) |
There was a problem hiding this comment.
We really need not block this call till we create a mention so this can easily be deferred to a celery task.
It also indicates a clear separation between the two actions.
There was a problem hiding this comment.
I like this idea but probably need some guidance to get started with writing a celery task, do you have a good example of a task I can look at? or I will just look at all of them currently there.
There was a problem hiding this comment.
api/cases/views/views.py
Outdated
| exporter_user_full_name = getattr(get_user_by_pk(request.user.pk), "full_name", "Exporter user") | ||
|
|
There was a problem hiding this comment.
get_user_by_pk() can fail if the user doesn't exist but we are assigning a default value.
Should we also not fail if the user doesn't exist?
we already have is_govuser above so we can use that and query ExporterUser model directly instead of this or even request.user may already contain these details.
There was a problem hiding this comment.
I can look at using the ExporterUser model
There was a problem hiding this comment.
I updated it to use the ExporterUser model directly. I tried also with a try-except block initially, but this didn't seem to help much, so I removed it. if we need to test for this scenario we might need to mock request.user which seemed a bit strange hence not going down that path
api/cases/models.py
Outdated
| """ | ||
| Create a LITE system mention e.g. exporter responded to an ECJU query | ||
| """ | ||
| from api.audit_trail import service as audit_trail_service |
There was a problem hiding this comment.
assuming this is unavoidable?
There was a problem hiding this comment.
I actually just copied this pattern because it's already in the file. There are 4-5 other methods that do this in the file so assumed there was a reason we can't reuse this audit trail service object by putting it as a file-level import.
There was a problem hiding this comment.
May be there are circular dependencies which cannot be avoided unless we refactor quite a bit.
See moving this to the top causes any issues and if they are not easy to fix then it can be retained otherwise no reason to leave it here.
api/cases/views/views.py
Outdated
| @@ -649,7 +651,16 @@ def put(self, request, pk, ecju_pk): | |||
| target=serializer.instance.case, | |||
| payload={"ecju_response": data.get("response")}, | |||
| ) | |||
| return JsonResponse(data={"ecju_query": serializer.data}, status=status.HTTP_201_CREATED) | |||
|
|
|||
| ecju_query.case.create_system_mention( | |||
There was a problem hiding this comment.
this has been updated
| Create a LITE system mention e.g. exporter responded to an ECJU query | ||
| """ | ||
| # to avoid circular import ImportError these must be imported here | ||
| from api.cases.models import CaseNote, CaseNoteMentions |
There was a problem hiding this comment.
ahh I hate inline imports they can lead to all kind of problems.
how about moving to common or in it's own helper class ?
hnryjmes
force-pushed
the
LTD-4620_Create_mention_notification_for_case_worker
branch
from
5bcaa05 to
24393c2
Compare
delete-merged-branch
bot
deleted the
LTD-4620_Create_mention_notification_for_case_worker
branch
Aim
A query is raised by a specific case worker. When an exporter responds to the query, a system mention is created that notifies the case worker that the exporter has responded to their query.
LTD-4620