feat: validators for cdd,ead, and error pipeline #177

johanseto · 2024-06-15T01:13:35Z

Description

feat: validators pipeline step for cdd and ead
feat: pipelines for validation error handling trigger.

Testing instructions

Tested in stage.

After

Use a user with missing data.
for eg lastname without data.
Send a CDD request
Check in logs validator errors
Check in audit model info related validation error.

easteregg: Check sentry and find the error.

Additional information

Jira story
https://edunext.atlassian.net/browse/FUTUREX-775

Checklist for Merge

Tested in a remote environment
Updated documentation
Rebased master/main
Squashed commits

eox_nelp/pearson_vue/validators.py

andrey-canon · 2024-06-18T00:15:55Z

eox_nelp/pearson_vue/validators.py

+
+
+@audit_method(action="PearsonVue Error validating data")
+def audit_validation_error(*args, **kwargs):


I'd like to suggest something different here, because this just audit errors, I mean a success is not an option here, however you can decorate the method validate_cdd_request and validate_ead_request and audit both cases and remove the extra logs, I'm thinking in something like validation_error_pipeline that would be a method that executes multiple task on the error case, at this moment you just need to raise the exception but probably we will need to send an email to inform the user that some fields has to be updated or something similar

eox_nelp/pearson_vue/tests/test_validators.py

andrey-canon · 2024-06-18T00:18:32Z

eox_nelp/pearson_vue/validators.py

+    try:
+        EadRequest(**ead_request)
+    except ValidationError as validation_exception:
+        logger.info("Validation error for ead_request: %s \n %s", ead_request, validation_exception)


if we raise the exception do we need really the log ?

johanseto · 2024-06-19T20:42:51Z

@andrey-canon now there is a new task for error handling.

In this case error_validation_task only has one function in the pipe: audit_error_validation This creates one record in eox-audit and no more. (thrown once)

eox_nelp/pearson_vue/tasks.py

andrey-canon

I left some code suggestions that works together, basically the suggestion is to raise a custom exception instead of sending the launch_validation_error_pipeline key and then handle that based on the exception we can achieve something similar to that with current implementation and dictionary key, however look this example:

def build_ead_request(
    profile_metadata,
    exam_metadata,
    transaction_type="Add",
    **kwargs
):  # pylint: disable=unused-argument
    """Build the ead_request dict.

    Args:
        profile_metadata (dict): Basic user data.
        exam_metadata (dict): Exam information.
        transaction_type (str): The type of transaction for the authorization (default is "Add").
        **kwargs: A dictionary containing the following key-value pairs:

    Returns:
        dict: dict with ead_request dict
    """
    try:
        ead_request = {
            "@clientAuthorizationID": exam_metadata["client_authorization_id"],
            "@clientID": getattr(settings, "PEARSON_RTI_WSDL_CLIENT_ID"),
            "@authorizationTransactionType": transaction_type,
            "clientCandidateID": f'NELC{profile_metadata["anonymous_user_id"]}',
            "examAuthorizationCount": exam_metadata["exam_authorization_count"],
            "examSeriesCode": exam_metadata["exam_series_code"],
            "eligibilityApptDateFirst": exam_metadata["eligibility_appt_date_first"],
            "eligibilityApptDateLast": exam_metadata["eligibility_appt_date_last"],
            "lastUpdate": timezone.now().strftime("%Y/%m/%d %H:%M:%S GMT"),
        }
    except KeyError:
        raise PearsonKeyError()

    # Validates 
    ead_request = CddRequest(**ead_request)  # lets image that this raises PearsonValidationError and returns the data that we required

    return {
        "ead_request": ead_request
    }

So in the example we have a pipeline that raises two different exceptions we could return a dictionary that indicates the error but we should also catch the validation error, finally if we apply that in the error handler pipeline we could have some pipes that make something based on the exception_type

andrey-canon · 2024-06-20T16:19:12Z

eox_nelp/pearson_vue/rti_backend.py

@@ -54,6 +60,17 @@ def run_pipeline(self):
                self.backend_data["pipeline_index"] = len(pipeline) - 1
                break

+            if result.get("launch_validation_error_pipeline"):


def run_pipeline(self): """ Executes the RTI pipeline by iterating through the pipeline functions. """ pipeline = self.get_pipeline() pipeline_index = self.backend_data.get("pipeline_index", 0) for idx, func in enumerate(pipeline[pipeline_index:]): self.backend_data["pipeline_index"] = pipeline_index + idx try: result = func(**self.backend_data) or {} except PearsonBaseError as exc : # just handle Pearson exceptions result["safely_pipeline_termination"] = True tasks = importlib.import_module("eox_nelp.pearson_vue.tasks") tasks.error_validation_task.delay(exception_type=exc.exception_type, ...) self.backend_data.update(result) if result.get("safely_pipeline_termination"): self.backend_data["pipeline_index"] = len(pipeline) - 1 break

I think you can remove this block

andrey-canon · 2024-06-20T16:24:06Z

eox_nelp/pearson_vue/pipeline.py

+    except ValidationError as validation_exception:
+        return {
+            "launch_validation_error_pipeline": True,
+            "validation_exception": validation_exception.json()
+        }
+
+    return None


Suggested change

except ValidationError as validation_exception:

return {

"launch_validation_error_pipeline": True,

"validation_exception": validation_exception.json()

}

return None

except ValidationError as validation_exception:

raise PearsonValidationError()

andrey-canon · 2024-06-20T16:28:16Z

eox_nelp/pearson_vue/pipeline.py

+    try:
+        raise_audit_validation_exception(*args, **kwargs)
+    except ValueError:
+        pass
+    logger.error("Validation Error args:%s-kwargs:%s", args, kwargs)


Suggested change

try:

raise_audit_validation_exception(*args, **kwargs)

except ValueError:

pass

logger.error("Validation Error args:%s-kwargs:%s", args, kwargs)

if exception_type == PearsonValidationError.exception_type:

try:

raise_audit_validation_exception(*args, **kwargs)

except ValueError:

logger.error("Validation Error args:%s-kwargs:%s", args, kwargs)

andrey-canon · 2024-06-20T16:37:29Z

eox_nelp/pearson_vue/rti_backend.py

+    validate_cdd_request,
+    validate_ead_request,


you are validating after sending the request

These are the imports I think so

hahah you are right

andrey-canon · 2024-06-20T16:43:49Z

eox_nelp/pearson_vue/tasks.py

+
+
+@shared_task(bind=True)
+def error_validation_task(self, pipeline_index=0, **kwargs):


Suggested change

def error_validation_task(self, pipeline_index=0, **kwargs):

def handle_error_task(self, pipeline_index=0, **kwargs):

andrey-canon · 2024-06-20T16:45:05Z

eox_nelp/pearson_vue/rti_backend.py

            check_service_availability,
            import_candidate_demographics,
        ]
+
+
+class ErrorValidationDataImport(RealTimeImport):


Suggested change

class ErrorValidationDataImport(RealTimeImport):

class ErrorRealTimeImportHandler(RealTimeImport):

johanseto · 2024-06-21T14:01:57Z

@andrey-canon Take a look to the new refactor proposals.
Here is how now, audit saves 2 different errors:

eox_nelp/pearson_vue/exceptions.py

andrey-canon · 2024-06-21T17:25:22Z

eox_nelp/pearson_vue/rti_backend.py

+            try:
+                result = func(**self.backend_data) or {}
+            except PearsonBaseError as pearson_error:
+                self.backend_data["pipeline_index"] = len(pipeline) - 1


I left a suggestion to avoid this line

andrey-canon · 2024-06-21T17:34:35Z

eox_nelp/pearson_vue/rti_backend.py

+                # clean kwargs to dont finish next pipeline launch.
+                executed__pipeline_kwargs = remove_keys_from_dict(self.backend_data, ["pipeline_index"])
+                executed__pipeline_kwargs["failed_step_pipeline"] = func.__name__
+                tasks = importlib.import_module("eox_nelp.pearson_vue.tasks")
+                tasks.rti_error_handler_task.delay(**executed__pipeline_kwargs, **pearson_error.__dict__)
+                break


Suggested change

# clean kwargs to dont finish next pipeline launch.

executed__pipeline_kwargs = remove_keys_from_dict(self.backend_data, ["pipeline_index"])

executed__pipeline_kwargs["failed_step_pipeline"] = func.__name__

tasks = importlib.import_module("eox_nelp.pearson_vue.tasks")

tasks.rti_error_handler_task.delay(**executed__pipeline_kwargs, **pearson_error.__dict__)

break

tasks = importlib.import_module("eox_nelp.pearson_vue.tasks")

tasks.rti_error_handler_task.delay(

failed_step_pipeline=func.__name__,

exception_data=pearson_error.__dict__,

)

The suggestion is to avoid to pass arguments that we don't need, the current implementation sends everything but we don't know the content of that and that content is variable, that depends on which pipe failed,so this suggestion is to pass explicit arguments then we can implement pipe based on arguments that we know

I like to have the same kwarg that the original pipeline had. Eg to send an email you would need the profile_metadata to know the user...
Then an error pipeline like social core way, you could use it like kwargs.get("profile_metadata")...

The only problem, is for example that the audit_pipe error saves everything but that I could change it.

andrey-canon · 2024-06-21T17:34:56Z

eox_nelp/pearson_vue/rti_backend.py

@@ -54,6 +60,17 @@ def run_pipeline(self):
                self.backend_data["pipeline_index"] = len(pipeline) - 1
                break

+            if result.get("launch_validation_error_pipeline"):


I think you can remove this block

andrey-canon · 2024-06-21T18:06:21Z

eox_nelp/pearson_vue/pipeline.py


    return {
        "ead_request": ead_request
    }
+
+
+def audit_pearson_error(*args, **kwargs):


I have conflict with this pipe, I think it's too general and this shouldn't raise a ValueError I think this should raise the same exception that started the rti_error_handler_task you are also passing the whole kwargs and args that could have sensitive data

andrey-canon · 2024-06-21T18:06:27Z

eox_nelp/pearson_vue/pipeline.py


    return {
        "ead_request": ead_request
    }
+
+
+def audit_pearson_error(*args, **kwargs):


I have conflict with this pipe, I think it's too general and this shouldn't raise a ValueError I think this should raise the same exception that started the rti_error_handler_task you are also passing the whole kwargs and args that could have sensitive data

I changed the exception raised. No way, I think that the data audited is similar to the rti import data.
So in terms in sensitive in my opinion is not much difference from this
Also you could compare the audit_pearson_error eg

This is for CddRequest and EadRequest feat: add native address fields

this removes the keys ["pipeline_index", "launch_validation_error_pipeline"] to dont finish the next pipeline error_validation_task.

johanseto · 2024-06-24T21:26:56Z

@andrey-canon I made this 7a20113 to remove sensitive kwargs if needed in the audit step.

andrey-canon · 2024-06-26T16:52:02Z

eox_nelp/pearson_vue/rti_backend.py

+                executed_pipeline_kwargs = remove_keys_from_dict(self.backend_data, ["pipeline_index"])
+                tasks = importlib.import_module("eox_nelp.pearson_vue.tasks")
+                tasks.rti_error_handler_task.delay(
+                    failed_step_pipeline=func.__name__,
+                    exception_data=pearson_error.__dict__,
+                    **executed_pipeline_kwargs,
+                )
+                break


This was your previous answer, but the code changed so the thread is not the same

I like to have the same kwarg that the original pipeline had. Eg to send an email you would need the profile_metadata to know the user...
Then an error pipeline like social core way, you could use it like kwargs.get("profile_metadata")...

The only problem, is for example that the audit_pipe error saves everything but that I could change it.

and my suggestion is the same hahahaha, you like to have the same kwargs, on the other hand, I don't like to have that because it's not easy to track which arguments you are sharing and what methods needs, so if you need the profile_metadata make that explicit

tasks.rti_error_handler_task.delay( failed_step_pipeline=func.__name__, exception_data=pearson_error.__dict__, profile_metadata=self.backend_data.get(profile_metadata), )

So if a create a new pipe is clear the information that is available, if we share everything I have to analyze every pipe to know which possible arguments are available, another option is to create a context,

context = { "profile_metadata": self.backend_data.get(profile_metadata), "exam_data": self.backend_data.get("exam_metadata") } tasks.rti_error_handler_task.delay( failed_step_pipeline=func.__name__, exception_data=pearson_error.__dict__, context=context, )

in this case as developer I just have to check the context to know the available information

I would let you know the intention of who needs the data??. And the dynamic kwargs behaviour.

Let's Imagine a case: I want to use profile_metadata in the error pipeline.

As the error_task is triggered when any step fails I don't know the kwargs executed(pipeline_step) in the initial RTI pipeline.
For eg

def get_pipeline(self): """ Returns the RTI pipeline, which is a list of functions to be executed. """ return [ handle_course_completion_status, get_user_data, get_exam_data, build_cdd_request, validate_cdd_request, build_ead_request, validate_ead_request, check_service_availability, import_candidate_demographics, import_exam_authorization, ]

Here we have pipe with different inputs and outputs.
So I would add all kwargs definition like:

tasks.rti_error_handler_task.delay( failed_step_pipeline=func.__name__, exception_data=pearson_error.__dict__, course_id=self.backend_data.get("course_id"), user_id=self.backend_data.get("user_id") profile_metadata=self.backend_data.get("profile_metadata"), exam_metadata=self.backend_data.get("exam_metadata"), ead_request=self.backend_data.get("ead_request"), cdd_request=self.backend_data.get("cdd_request"), transaction_type=self.backend_data.get("transaction_type"), ... )

And also here you have to add any other possible return of the pipeline steps(new steps)...

Or only create a pipeline with

if not kwargs.get("profile_metadata"): return

Now, using **kwargs, when launching the error pipeline you only specify that this pipeline is like the continuation of the first pipeline so it would have the same kwargs.(executed-dynamic~)

andrey-canon · 2024-06-26T17:45:30Z

eox_nelp/pearson_vue/pipeline.py

+    audit_action = "Pearson Vue Exception"
+    audit_action = f"{audit_action}~{exception_data['exception_type']}"


Suggested change

audit_action = "Pearson Vue Exception"

audit_action = f"{audit_action}~{exception_data['exception_type']}"

audit_action = f"Pearson Vue Exception~{exception_data['exception_type']}"

andrey-canon · 2024-06-26T17:59:04Z

eox_nelp/pearson_vue/pipeline.py


 from eox_nelp.api_clients.pearson_rti import PearsonRTIApiClient
 from eox_nelp.edxapp_wrapper.student import anonymous_id_for_user
+from eox_nelp.pearson_vue import exceptions


Suggested change

from eox_nelp.pearson_vue import exceptions

from eox_nelp.pearson_vue import exceptions as pearson_vue_exceptions

andrey-canon · 2024-06-26T18:09:11Z

eox_nelp/pearson_vue/pipeline.py

+
+    @audit_method(action=audit_action)
+    def raise_audit_pearson_exception(exception_data, **audit_kwargs):
+        raise pearson_exception(exception_data, audit_kwargs)


Here you are passing two positional arguments so if the pearson_exception is PearsonAttributeError that equivalent to pearson_exception(request_type=exception_data, attribute_error=audit_kwargs), does that make sense ?
id f the exception_data contains all the original data that should be pearson_exception(**exception_data),

eox_nelp/pearson_vue/exceptions.py

andrey-canon · 2024-06-26T18:43:34Z

eox_nelp/pearson_vue/rti_backend.py

+            except PearsonBaseError as pearson_error:
+                # clean kwargs to dont finish next pipeline launch.
+                executed_pipeline_kwargs = remove_keys_from_dict(self.backend_data, ["pipeline_index"])
+                tasks = importlib.import_module("eox_nelp.pearson_vue.tasks")


just asking why not from eox_nelp.pearson_vue.tasks import rti_error_handler_task

At the top of the file is a circular python import error.
And I could add there from eox_nelp.pearson_vue.tasks import rti_error_handler_task but I want to avoid
import-outside-toplevel

refactor: init exception also from exception_dict chore: pylint changes

eox_nelp/pearson_vue/pipeline.py

Co-authored-by: Andrey Cañon <[email protected]>

andrey-canon · 2024-06-28T18:01:15Z

eox_nelp/pearson_vue/data_classes.py

+"""
+Module to add data_classes related Pearson Vue Integration
+"""
+# pylint: disable=missing-class-docstring


andrey-canon · 2024-06-28T18:02:21Z

eox_nelp/pearson_vue/exceptions.py

+        """Init pearson exception.Is mandatory the exception_reasons.
+        You could init using pipe_frame
+        Or init using exception_dict representation with **kwargs.
+        That representation should have the following shape:
+         exception_dict  = {
+                'exception_type': 'validation-error',
+                'pipe_args_dict': {
+                    "cdd_request": {}
+                },
+                'pipe_function': 'validate_cdd_request',
+                'exception_reason': "error: ['String to short.']"
+            }


Please update this

andrey-canon · 2024-06-28T18:11:55Z

eox_nelp/pearson_vue/tests/test_pipeline.py

-        """ Test ead_request is built with profile_metadata and exam_metadata.
-            Expected behavior:
-            - The result is the expected value.
+    def setUp(self):


Add a blank line before this line

andrey-canon · 2024-06-28T18:24:43Z

eox_nelp/pearson_vue/tests/test_pipeline.py

-        """ Test cdd_request is built with profile_metadata.
-            Expected behavior:
-            - The result is the expected value.
+    def setUp(self):


Add a blank line before this line

andrey-canon · 2024-06-28T18:24:46Z

eox_nelp/pearson_vue/tests/test_pipeline.py

-        """ Test ead_request is built with profile_metadata and exam_metadata.
-            Expected behavior:
-            - The result is the expected value.
+    def setUp(self):


Add a blank line before this line

github-actions bot added test size/m m lines label labels Jun 15, 2024

johanseto self-assigned this Jun 15, 2024

johanseto requested a review from andrey-canon June 17, 2024 17:25

andrey-canon requested changes Jun 18, 2024

View reviewed changes

andrey-canon mentioned this pull request Jun 18, 2024

Jlc/pearson vue/validators approach #154

Closed

4 tasks

johanseto force-pushed the jlc/add-pydantic-dataclasses branch from f033834 to 5fceafc Compare June 19, 2024 15:37

github-actions bot added size/l and removed size/m m lines label labels Jun 19, 2024

johanseto force-pushed the jlc/add-pydantic-dataclasses branch from 5fceafc to 9844c63 Compare June 19, 2024 15:39

johanseto changed the base branch from master to jlc/refactor-cdd-request-pipeline June 19, 2024 15:40

github-actions bot added size/m m lines label django_plugin size/l and removed size/l size/m m lines label labels Jun 19, 2024

johanseto force-pushed the jlc/add-pydantic-dataclasses branch from b7d98ef to 692bc33 Compare June 19, 2024 19:04

johanseto changed the title ~~feat: validators for cdd and ead~~ feat: validators for cdd and ead and error pipeline Jun 19, 2024

johanseto requested a review from andrey-canon June 19, 2024 20:43

johanseto changed the title ~~feat: validators for cdd and ead and error pipeline~~ feat: validators for cdd,ead, and error pipeline Jun 19, 2024

johanseto commented Jun 19, 2024

View reviewed changes

eox_nelp/pearson_vue/tasks.py Outdated Show resolved Hide resolved

johanseto changed the base branch from jlc/refactor-cdd-request-pipeline to master June 19, 2024 21:15

johanseto force-pushed the jlc/add-pydantic-dataclasses branch from 07d0ce0 to a01630a Compare June 19, 2024 21:15

andrey-canon requested changes Jun 20, 2024

View reviewed changes

johanseto requested a review from andrey-canon June 21, 2024 14:02

andrey-canon requested changes Jun 21, 2024

View reviewed changes

johanseto force-pushed the jlc/add-pydantic-dataclasses branch from c2415cf to 54ded90 Compare June 24, 2024 19:18

feat: add pydantic dataclassed for cdd ^ ead

33fdf8a

This is for CddRequest and EadRequest feat: add native address fields

johanseto and others added 15 commits June 24, 2024 14:39

feat: add error_validation_task test =)

89b2d5b

fix: add validation step to rti backends

7cd649b

fix: pipeline starts with wrong pipeline_index

80d39ee

this removes the keys ["pipeline_index", "launch_validation_error_pipeline"] to dont finish the next pipeline error_validation_task.

feat: keep validation_exception in error_pipeline

36b7a4b

chore: fix docstring

cc7d0fe

chore: improve method name

4046899

refactor: error handling with pearson exceptions

8670080

feat: pr recommend remove unuseful block code

908e22f

refactor: send exception_data in kwarg

5e02a8e

refactor: change the error raised

c84f5e9

feat: audit with name action variable

cbd4288

feat: add skip pipe if not exception_data

ae1c5da

fix: assertNoLogs is only available py3.10+

522a5de

feat: raise same exception based on exception_data

c01a829

refactor: hidden_kwargs 2 rm sensitive keys audit

7a20113

johanseto force-pushed the jlc/add-pydantic-dataclasses branch from 54ded90 to 7a20113 Compare June 24, 2024 19:41

johanseto requested a review from andrey-canon June 24, 2024 21:27

andrey-canon requested changes Jun 26, 2024

View reviewed changes

refactor: manage exception with dict representation

c755803

refactor: init exception also from exception_dict chore: pylint changes

andrey-canon reviewed Jun 28, 2024

View reviewed changes

eox_nelp/pearson_vue/pipeline.py Outdated Show resolved Hide resolved

johanseto and others added 4 commits June 28, 2024 10:53

chore: update eox_nelp/pearson_vue/pipeline.py

1b03234

Co-authored-by: Andrey Cañon <[email protected]>

chore: remove unnused utils

d718aca

feat: pr recommendation init exc improvemente

a9e1fc6

feat: add test for exception file

0f59377

johanseto force-pushed the jlc/add-pydantic-dataclasses branch from 00f0fb0 to 0f59377 Compare June 28, 2024 16:51

github-actions bot removed the django_plugin label Jun 28, 2024

andrey-canon approved these changes Jun 28, 2024

View reviewed changes

chore: docstrings improvements

5f61fed

johanseto merged commit f95f96b into master Jun 28, 2024
7 checks passed



		@audit_method(action="PearsonVue Error validating data")
		def audit_validation_error(args, *kwargs):



		@shared_task(bind=True)
		def error_validation_task(self, pipeline_index=0, **kwargs):

	def error_validation_task(self, pipeline_index=0, **kwargs):
	def handle_error_task(self, pipeline_index=0, **kwargs):

	class ErrorValidationDataImport(RealTimeImport):
	class ErrorRealTimeImportHandler(RealTimeImport):

		audit_action = "Pearson Vue Exception"
		audit_action = f"{audit_action}~{exception_data['exception_type']}"

	audit_action = "Pearson Vue Exception"
	audit_action = f"{audit_action}~{exception_data['exception_type']}"
	audit_action = f"Pearson Vue Exception~{exception_data['exception_type']}"

	from eox_nelp.pearson_vue import exceptions
	from eox_nelp.pearson_vue import exceptions as pearson_vue_exceptions

feat: validators for cdd,ead, and error pipeline #177

feat: validators for cdd,ead, and error pipeline #177

Conversation

johanseto commented Jun 15, 2024 • edited Loading

Description

Testing instructions

After

Additional information

Checklist for Merge

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johanseto commented Jun 19, 2024

andrey-canon 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

johanseto commented Jun 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johanseto Jun 21, 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

johanseto commented Jun 24, 2024

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

johanseto commented Jun 15, 2024 •

edited

Loading

johanseto Jun 21, 2024 •

edited

Loading