LTD: Imrove validation during submission #2337

saruniitr · 2024-12-09T13:04:16Z

Change description

We validate several things when a draft application is submitted. The current code assumes only one end-user which is not going to be the case for SIEL Temporary applications.

Before changing the code the idea is to simplify the current validation code. This is also not updated with the new fields that we added to the model. This PR doesn't add any new changes except that it simplifies validation.

Once this is merged then further changes will be done to allow for multiple end-users.

… defined

Goods location is currently set in StandardApplicationFactory so update expected test data accordingly

hnryjmes · 2024-12-09T17:01:04Z

api/core/model_mixins.py

@@ -42,3 +42,20 @@ class Trackable:

    def get_history(self, field):
        raise NotImplementedError()
+
+
+class BaseApplicationValidator:


I assume this is general enough that it could be used for other licence types than just SIELs (e.g. OIELs)?

Current code sorting the results based on first key in the results but for one of the test this is same for all rows so randomly it sorts by different order and causing the test to fail. Update the test to sort by all keys so we have a consistent order.

hnryjmes · 2024-12-10T10:54:42Z

api/applications/validators.py

+from api.parties.models import PartyDocument
+
+
+def siel_locations_validator(application):


there are a couple of differences in the checks between this function and the original _validate_siel_locations, is that because they are not needed and so they can be removed?

yes, there was a check related to GoodsTypeCategory.CRYPTOGRAPHIC which is for OIELs and we don't support yet. When we support it will be in a different method as this is only for SIELs

hnryjmes · 2024-12-10T11:07:11Z

api/applications/models.py

@@ -268,12 +268,17 @@ def set_appealed(self, appeal, exporter_user):
        self.set_sub_status(CaseSubStatusIdEnum.UNDER_APPEAL__APPEAL_RECEIVED)
        self.add_to_queue(Queue.objects.get(id=QueuesEnum.LU_APPEALS))

+    def validate(self):
+        raise NotImplementedError("Validator to validate application attributes is not implemented")


hnryjmes

I'm wondering if we can get this branch deployed to an env and have someone from UCD click around and do a few submissions as a regression test. Just because there's a lot changing here

saruniitr · 2024-12-11T12:12:18Z

I'm wondering if we can get this branch deployed to an env and have someone from UCD click around and do a few submissions as a regression test. Just because there's a lot changing here

@hnryjmes Verified these changes on devdata and compared with uat at the same time. In both cases the errors messages shown for the items in application task list is same. As the sections are filled then the errors are no longer shown.

hnryjmes · 2024-12-11T13:15:11Z

api/applications/validators.py

+    if not application.end_user:
+        return "To submit the application, add an end user"


it seems like this should be at the top, before the check for export type being temporary?

hnryjmes · 2024-12-11T13:24:30Z

api/applications/validators.py

+def siel_ultimate_end_users_validator(application):
+    """If ultimate end users are required and they added any documents check if they are all valid"""
+    error = None
+    if not application.end_user:
+        return "To submit the application, add an end user"
+
+    ultimate_end_user_required = application.goods.filter(
+        Q(is_good_incorporated=True) | Q(is_onward_incorporated=True)
+    ).exists()
+
+    if ultimate_end_user_required and application.ultimate_end_users.count() == 0:
+        error = "To submit the application, add an ultimate end-user"
+
+    return error


should there be a check for ultimate end user documents here?

It is optional,
https://github.com/uktrade/lite-api/blob/dev/api/applications/creators.py#L154

hnryjmes

have checked all the validators and they look right to me by eye. if we can get product review from UCD on this later that would give us extra confidence but it looks to me like everything has been copied over correctly.

currycoder · 2024-12-12T10:30:35Z

api/applications/models.py

    def create_amendment(self, user):
        raise NotImplementedError()


 # Licence     Applications
 class StandardApplication(BaseApplication, Clonable):
+    from api.applications.validators import StandardApplicationValidator


Does this import need to be here?

actually added this to avoid circular dependencies but they are not there atm, I will move this outside of this.

currycoder · 2024-12-12T10:51:53Z

api/applications/validators.py

+        and application.goods_recipients not in recipient_choices
+        and application.is_shipped_waybill_or_lading is None
+    ):
+        return "To submit the application, add a product location"


It would be good to discuss; should the API validation errors be returned as end-user readable strings or as lite-frontend consumable error keys?

class StandardApplicationValidationErrors: MISSING_PRODUCT_LOCATION = "MISSING_PRODUCT_LOCATION" ... return StandardApplicationValidationErrors.MISSING_PRODUCT_LOCATION

I feel a bit like this is a similar muddying of the API that we currently have for audit trail text formatting; the API does some text formatting rather than just responding with pure data. That means when changing the format of audit trail messages, we always have to do work on the API rather than just adjusting the frontend.

Similarly for this, the API is getting unnecessarily tied to frontend structures/wording. Instead, the frontend could translate the error key in to the user readable string. Or have the freedom to do bullet points instead e.g.

To submit the application: - Add a product location <a href="/place_to_fix_it">Here</a> - Add an end user

Definitely worth considering, initially I only want to get parity with the current functionality. Once this is merged the add further improvements.

saruniitr added 17 commits March 25, 2024 12:08

Add basic validator to validate an application for submission

6bee13f

Update validator to validate end-user details

c3714dd

Add locations validator for SIEL applications

c7b6edb

Add validator to check if security approvals are completed or not

b7d915f

Merge branch 'dev' into LTD-improve-validation-during-submission

52c7fbd

Add goods and other validators for SIEL applications

4bcafc1

Refactor StandardApplication validators functions

6609026

Fix linter error and raise not implemented error if no validators are…

1539921

… defined

Merge branch 'dev' into LTD-improve-validation-during-submission

889222b

Delete unused validation helper functions

a229adb

Update test data

e7f3dc6

Goods location is currently set in StandardApplicationFactory so update expected test data accordingly

Merge branch 'dev' into LTD-improve-validation-during-submission

2417bf7

Add/update tests to fix coverage issues

e71affa

Merge branch 'dev' into LTD-improve-validation-during-submission

7191da4

Merge branch 'dev' into LTD-improve-validation-during-submission

acc05f5

Merge branch 'dev' into LTD-improve-validation-during-submission

ed49b60

Move the import into the class to avoid circular imports

b768b39

saruniitr marked this pull request as draft December 9, 2024 13:04

hnryjmes reviewed Dec 9, 2024

View reviewed changes

Correct an error message

cee13c6

saruniitr marked this pull request as ready for review December 9, 2024 17:33

Fix sorting issue

83f21be

Current code sorting the results based on first key in the results but for one of the test this is same for all rows so randomly it sorts by different order and causing the test to fail. Update the test to sort by all keys so we have a consistent order.

hnryjmes reviewed Dec 10, 2024

View reviewed changes

hnryjmes reviewed Dec 11, 2024

View reviewed changes

hnryjmes approved these changes Dec 11, 2024

View reviewed changes

currycoder reviewed Dec 12, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

LTD: Imrove validation during submission #2337

LTD: Imrove validation during submission #2337

saruniitr commented Dec 9, 2024 •

edited

Loading

hnryjmes Dec 9, 2024

saruniitr Dec 9, 2024

hnryjmes Dec 10, 2024

saruniitr Dec 10, 2024 •

edited

Loading

hnryjmes Dec 10, 2024

hnryjmes left a comment

saruniitr commented Dec 11, 2024

hnryjmes Dec 11, 2024

hnryjmes Dec 11, 2024

saruniitr Dec 11, 2024

hnryjmes left a comment

currycoder Dec 12, 2024

saruniitr Dec 12, 2024

currycoder Dec 12, 2024

saruniitr Dec 12, 2024

		from api.parties.models import PartyDocument


		def siel_locations_validator(application):

		if not application.end_user:
		return "To submit the application, add an end user"

LTD: Imrove validation during submission #2337

Are you sure you want to change the base?

LTD: Imrove validation during submission #2337

Conversation

saruniitr commented Dec 9, 2024 • edited Loading

Change description

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saruniitr Dec 10, 2024 • edited Loading

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

saruniitr commented Dec 11, 2024

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

saruniitr commented Dec 9, 2024 •

edited

Loading

saruniitr Dec 10, 2024 •

edited

Loading