LTD: Add endpoint for licences list #2235

saruniitr · 2024-10-15T14:56:10Z

Aim

kevincarrogan · 2024-10-15T15:15:59Z

api/data_workspace/v2/serializers.py

+    name = serializers.CharField(source="*")
+
+
+class SIELLicenceSerializer(serializers.ModelSerializer):


Let’s just call this Licence for now.

kevincarrogan · 2024-10-15T15:16:17Z

api/data_workspace/v2/serializers.py

+from api.licences.models import Licence
+
+
+class LicenceStatusSerializer(serializers.Serializer):


Let’s not include status in this first prototype.

part of cherry-picked commit, will remove

kevincarrogan · 2024-10-15T15:16:56Z

api/data_workspace/v2/tests/bdd/scenarios/licence_statuses.feature

+    Then there are no other licence statuses
+
+
+Scenario: Issued licence is included in the extract


Can we have this in a separate BDD scenario file? I’d like it if each endpoint had its own scenario file.

kevincarrogan · 2024-10-15T15:17:33Z

api/data_workspace/v2/views.py

+    serializer_class = SIELLicenceSerializer
+
+    def get_queryset(self):
+        return Licence.objects.exclude(status=LicenceStatus.DRAFT)


It would be good to see a test for this somewhere. Possibly as a distinct scenario in the BDD file for the endpoint.

hnryjmes · 2024-10-16T14:45:54Z

api/data_workspace/v2/tests/bdd/conftest.py

+def unpage_data(client):
+    def _unpage_data(url):
+        unpaged_results = []
+        while True:


this works fine and looks great, although I am surprised there's not a way of doing this using the existing libraries we have

hnryjmes

everything looks good to me, happy to approve

this will be a long lived branch so does not make sense to approve now

kevincarrogan · 2024-10-21T09:08:42Z

api/data_workspace/v2/tests/bdd/scenarios/licences.feature

+Scenario: Check that draft licences are not included in the extract
+    Given a standard draft licence is created
+    Then the draft licence is not included in the extract
+
+Scenario: Issued licence is included in the extract
+    Given a standard licence is issued
+    Then the issued licence is included in the extract


I feel that it might be nice to have at test that shows that a licence transitioning from draft to issued makes a difference.

kevincarrogan · 2024-10-21T09:09:07Z

api/data_workspace/v2/views.py

+        # When an application with all goods as NLR is finalised then the current code
+        # creates a licence however the goods on this licence will be empty. This
+        # will skew licence data hence exclude them


A BDD test describing this NLR case would be nice to see.

kevincarrogan · 2024-10-21T09:09:53Z

api/data_workspace/v2/views.py

+                    LicenceStatus.DRAFT,
+                    LicenceStatus.CANCELLED,


Could we extract these statuses out into a named thing? For CaseStatus we have a range of named lists so I think it would be good to do the same here.

kevincarrogan · 2024-10-21T09:10:06Z

api/data_workspace/v2/views.py

+                case__status__status__in=[
+                    CaseStatusEnum.FINALISED,
+                    CaseStatusEnum.SUPERSEDED_BY_EXPORTER_EDIT,
+                ],


Could we come up with a named list for these statuses?

hnryjmes · 2024-10-23T08:24:50Z

api/data_workspace/v2/views.py

+        Case.objects.filter(
+            licences__generatedcasedocument__template_id=SIEL_TEMPLATE_ID,
+            licences__generatedcasedocument__visible_to_exporter=True,
+            licences__generatedcasedocument__safe=True,


is this just an extra precaution? the document is generated internally so I wouldn't think this would ever not be safe=True ?

safe=True indicates no virus found

kevincarrogan · 2024-10-28T13:29:53Z

api/data_workspace/v2/views.py

+    issued_qs = Case.objects.filter(
+        licences__generatedcasedocument__template_id=SIEL_TEMPLATE_ID,
+        licences__generatedcasedocument__visible_to_exporter=True,
+        licences__generatedcasedocument__safe=True,
+    )
+    refused_qs = Case.objects.filter(
+        id__in=GeneratedCaseDocument.objects.filter(
+            template_id=SIEL_REFUSAL_TEMPLATE_ID,
+            visible_to_exporter=True,
+            safe=True,
+        ).values_list("case", flat=True)
+    )
+
+    queryset = (issued_qs | refused_qs).distinct().order_by("-reference_code")


Isn't this just a very long-winded way to do the same thing as

Case.objects.filter( licences__generatedcasedocument__template_id__in=[SIEL_TEMPLATE_ID, SIEL_REFUSAL_TEMPLATE_ID], licences__generatedcasedocument__visible_to_exporter=True, licences__generatedcasedocument__safe=True, ).distinct()

Actually no because there won't be a licence for refused cases.
That was the initial change and as expected refusal cases were not filtered.

Are both generated documents attached to the case and we could just skip the licences__ bit entirely for both?

It would also be good to have some joined up thinking based on https://github.com/uktrade/lite-api/pull/2254/files#r1819089904 as it looks like we are diverging slightly with this particular heuristic and the future implementation.

kevincarrogan · 2024-10-28T13:33:30Z

api/data_workspace/v2/serializers.py

+        )
+
+    def get_decision(self, case):
+        if case.licences.count():


Can we name this in a way that clarifies what the case.licences.count() is doing and why it's important.

You mean just by adding a comment or annotate to a field and use that?

Whichever lets me know exactly why the case.licences.count matters.

kevincarrogan · 2024-10-28T13:33:58Z

api/data_workspace/v2/serializers.py

+            return LicenceDecisionType.ISSUED
+        else:
+            return LicenceDecisionType.REFUSED


This looks like it's mutually exclusive, but is it possible that something has been issued and then refused or vice versa?

I checked that, currently there are no such cases.
Because of amendments it is not possible going forward also but there is a possibility with Appeals.

- remove licence statuses endpoint for now - make tests work

Licence document generation date is taken for this purpose.

When all products on the application are NLR then the current code allowed creation of a licence due a bug. This is not fixed yet so exclude these cases as it can skew licence data.

In some case a licence is issued but subsequently application is marked as withdrawn. Licence also should've been cancelled in this case but we are not handling this atm so filter these cases.

As there can be multiple documents in these cases

Use Case as base object and use it to filter on the licence documents. This avoids filtering of various case statuses, NLR products etc

Use the refusal letter generated date as the decision date

saruniitr marked this pull request as ready for review October 15, 2024 14:56

saruniitr force-pushed the LTD-stats-init branch from 2b17507 to 50ee250 Compare October 15, 2024 15:07

kevincarrogan reviewed Oct 15, 2024

View reviewed changes

saruniitr force-pushed the LTD-stats-init branch 2 times, most recently from b92307d to ce7cbf9 Compare October 16, 2024 08:35

hnryjmes reviewed Oct 16, 2024

View reviewed changes

hnryjmes previously approved these changes Oct 16, 2024

View reviewed changes

kevincarrogan reviewed Oct 21, 2024

View reviewed changes

saruniitr force-pushed the LTD-stats-init branch from 91490ec to d49fc4a Compare October 22, 2024 16:42

hnryjmes reviewed Oct 23, 2024

View reviewed changes

kevincarrogan reviewed Oct 28, 2024

View reviewed changes

kevincarrogan and others added 17 commits October 31, 2024 11:23

Add DRF csv renderer

5d58f86

Add endpoint for licence statuses

684aac7

Add endpoint for retrieving SIEL licences list

6cb8532

Address review comments

f86d6f3

- remove licence statuses endpoint for now - make tests work

Update url name and fix fixture

d90ca37

Add licence issue date to licences list

545f1cd

Licence document generation date is taken for this purpose.

Update Licences list view to excludes licences with no goods

e73df0f

When all products on the application are NLR then the current code allowed creation of a licence due a bug. This is not fixed yet so exclude these cases as it can skew licence data.

Only consider licences for finalised Cases

95963ba

In some case a licence is issued but subsequently application is marked as withdrawn. Licence also should've been cancelled in this case but we are not handling this atm so filter these cases.

Ignore cancelled licences

0ae5925

As there can be multiple documents in these cases

Rename status to decision as that is what it is

2786805

Add more common fixtures

673b60c

Seed letter layouts and templates

7927d70

Update licence test to include steps for generating documents

4553cb0

Use Case as base object the get licences list

c0760ef

Use Case as base object and use it to filter on the licence documents. This avoids filtering of various case statuses, NLR products etc

Update licence list API view to be called licence decision

060f962

Allow pagination to be disabled for licence decisions

c10f0e9

Include refused cases in the extract

7b06322

Use the refusal letter generated date as the decision date

kevincarrogan added 7 commits October 31, 2024 11:25

Simplify query for getting cases with case documents

84c0b2f

Determine issued and refused in queryset

0600888

Condense down logic for finding decision made at

ce5f7ff

Temporarily skip hawk authentication

35f1450

Fix DW authentication

91b3d21

Add revoked as a licence decision

df81a44

Move into queryset

55c8493

kevincarrogan force-pushed the LTD-stats-init branch from c6737dd to 55c8493 Compare October 31, 2024 11:25

saruniitr marked this pull request as draft November 4, 2024 10:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

LTD: Add endpoint for licences list #2235

LTD: Add endpoint for licences list #2235

saruniitr commented Oct 15, 2024

kevincarrogan Oct 15, 2024

kevincarrogan Oct 15, 2024

saruniitr Oct 15, 2024

kevincarrogan Oct 15, 2024

kevincarrogan Oct 15, 2024

hnryjmes Oct 16, 2024

hnryjmes left a comment

kevincarrogan Oct 21, 2024

kevincarrogan Oct 21, 2024

kevincarrogan Oct 21, 2024

kevincarrogan Oct 21, 2024

hnryjmes Oct 23, 2024

saruniitr Oct 23, 2024

kevincarrogan Oct 28, 2024

saruniitr Oct 28, 2024

kevincarrogan Oct 28, 2024

kevincarrogan Oct 28, 2024

kevincarrogan Oct 28, 2024

saruniitr Oct 28, 2024

kevincarrogan Oct 28, 2024

kevincarrogan Oct 28, 2024

saruniitr Oct 28, 2024

		name = serializers.CharField(source="*")


		class SIELLicenceSerializer(serializers.ModelSerializer):

		from api.licences.models import Licence


		class LicenceStatusSerializer(serializers.Serializer):

		Then there are no other licence statuses


		Scenario: Issued licence is included in the extract

LTD: Add endpoint for licences list #2235

Are you sure you want to change the base?

LTD: Add endpoint for licences list #2235

Conversation

saruniitr commented Oct 15, 2024

Aim

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

hnryjmes 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

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