feat: add audit for succesfull imports #192

johanseto · 2024-07-08T23:05:19Z

Description

add audit to pipelines

Testing instructions

Before

After

Additional information

Include anything else that will help reviewers and consumers understand the change.

Does this change depend on other changes elsewhere?
Any special concerns or limitations? For example: deprecations, migrations, security, or accessibility.
Link to other information about the change, such as Jira issues, GitHub issues, or Discourse discussions.

Checklist for Merge

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

andrey-canon

why you modified current pipeline steps instead of adding a general pipeline step that allows to audit the whole process ?

johanseto · 2024-07-09T14:45:58Z

why you modified current pipeline steps instead of adding a general pipeline step that allows to audit the whole process ?

I think the scope was only for the successful import, do you think I would audit all the pipeline execution?

andrey-canon · 2024-07-10T17:30:00Z

eox_nelp/pearson_vue/pipeline.py

@@ -613,3 +614,43 @@ def get_enrollment_from_id(enrollment_id, enrollment=None, **kwargs):  # pylint:
            pass

    return {}
+
+
+def save_audit_pipeline(pipeline_backend, last_step, cdd_import=None, ead_import=None, **kwargs):


As you mentioned this is not a pipeline step, so I don't understand why you included this in the pipeline file, anyway I wondering why you split the logic into this method and audit pipeline decorator

andrey-canon · 2024-07-10T17:36:14Z

eox_nelp/pearson_vue/pipeline.py

+        cdd_import (dict, optional): Defaults to None.
+        ead_import (dict, optional):  Defaults to None.


Why ? this implementation is too specific, you are assuming in advanced that the backend will implement cdd and ead pipelines steps and that is not true, probably your answer will include that there are some default values but again this code was written to work with something specific instead of a general solution

andrey-canon · 2024-07-10T17:38:20Z

eox_nelp/pearson_vue/pipeline.py

+    def executed_pipeline(
+        pipeline_backend,
+        last_step,
+        cdd_import,
+        ead_import,
+        **kwargs


Suggested change

def executed_pipeline(

pipeline_backend,

last_step,

cdd_import,

ead_import,

**kwargs

def executed_pipeline(

backend_name,

last_pipeline_step,

backend_data,

andrey-canon · 2024-07-10T17:40:00Z

eox_nelp/pearson_vue/pipeline.py

+        logger.info(
+            "Succesfull imports. cdd_import: %s. ead_import: %s",
+            cdd_import,
+            ead_import,
+        )


Suggested change

logger.info(

"Succesfull imports. cdd_import: %s. ead_import: %s",

cdd_import,

ead_import,

)

Since this is not a general implementation

andrey-canon · 2024-07-10T17:42:53Z

eox_nelp/pearson_vue/rti_backend.py

+            return func(self, *args, **kwargs)
+        finally:
+            if self.use_audit_pipeline and not self.backend_data.get("catched_pearson_error"):
+                save_audit_pipeline(


This is just a simple log, why you didn't include the logic here ?

andrey-canon · 2024-07-10T17:43:24Z

eox_nelp/pearson_vue/rti_backend.py

+                save_audit_pipeline(
+                    pipeline_backend=self.__class__.__name__,
+                    last_step=self.get_pipeline()[self.backend_data.get("pipeline_index", 0)].__name__,
+                    **self.backend_data


Suggested change

**self.backend_data

backend_data=self.backend_data

andrey-canon · 2024-07-10T17:49:59Z

eox_nelp/pearson_vue/rti_backend.py

@@ -53,15 +78,17 @@ class AbstractBackend(ABC):
        handle_error(exception: Exception, failed_step_pipeline: str): Handles errors during pipeline execution.
    """

-    def __init__(self, **kwargs):
+    def __init__(self, use_audit_pipeline=True, **kwargs):


The advantage of this is that you can change the backend behavior based on the initialization, but do we need that?, it's not better stable backends whose execution or behavior is always the same ?

Suggested change

def __init__(self, use_audit_pipeline=True, **kwargs):

use_audit_pipeline=True

def __init__(self, **kwargs):

Yes, but then I must apply the decorator to the specific pipeline backend. Otherwise I would get something like this.
https://edunext.stage.nelp.gov.sa/admin/eox_audit_model/auditmodel/32026/change/

You can see the error pipeline is also audited.

what happen if you override that in the subclass ?

andrey-canon · 2024-07-10T17:51:27Z

eox_nelp/pearson_vue/rti_backend.py

    validate_cdd_request,
    validate_ead_request,
 )


+def audit_pipeline(func):


could you please move this to a decorators.py file ?

andrey-canon

why did you remove some things like the use_audit_pipeline and the catched_pearson_error?

andrey-canon · 2024-07-12T17:18:40Z

eox_nelp/pearson_vue/pipeline.py

@@ -227,7 +227,7 @@ def import_candidate_demographics(cdd_request, **kwargs):  # pylint: disable=unu
    response = api_client.import_candidate_demographics(payload)

    if response.get("status", "error") == "accepted":
-        return response
+        return {"cdd_import": response}


do you think we will need this ?

andrey-canon · 2024-07-12T17:18:48Z

eox_nelp/pearson_vue/pipeline.py

@@ -276,7 +276,7 @@ def import_exam_authorization(ead_request, **kwargs):  # pylint: disable=unused-
    response = api_client.import_exam_authorization(payload)

    if response.get("status", "error") == "accepted":
-        return response
+        return {"ead_import": response}


Same comments as above

johanseto · 2024-07-12T18:19:04Z

why did you remove some things like the use_audit_pipeline and the catched_pearson_error?

The use audit because don't you recommend using the override in the run_pipeline method?

andrey-canon · 2024-07-12T18:26:58Z

why did you remove some things like the use_audit_pipeline and the catched_pearson_error?

The use audit because don't you recommend using the override in the run_pipeline method?

Nop, there was a misunderstanding, my previous recommendation was to test use_audit_pipeline as attribute class and then override that in the subclass

class AbstractBackend(ABC):
    use_audit_pipeline=True

class ErrorRealTimeImportHandler(AbstractBackend):
    use_audit_pipeline=False

github-actions bot added test size/s labels Jul 8, 2024

johanseto assigned andrey-canon Jul 8, 2024

johanseto requested a review from andrey-canon July 8, 2024 23:05

andrey-canon reviewed Jul 9, 2024

View reviewed changes

johanseto force-pushed the jlc/audit-successfull-import branch from 95f0023 to abbc564 Compare July 9, 2024 22:10

github-actions bot added size/m m lines label and removed size/s labels Jul 9, 2024

johanseto force-pushed the jlc/audit-successfull-import branch from b364213 to 8505e35 Compare July 10, 2024 13:03

andrey-canon requested changes Jul 10, 2024

View reviewed changes

feat: use decorator for audit backend

ef9a5b4

johanseto force-pushed the jlc/audit-successfull-import branch from 8505e35 to ef9a5b4 Compare July 12, 2024 01:01

github-actions bot added size/s and removed test size/m m lines label labels Jul 12, 2024

andrey-canon reviewed Jul 12, 2024

View reviewed changes

johanseto added 2 commits July 12, 2024 15:14

fix: audit could not serialize classes(self)

d54e4d4

feat: add test based in use_audit_backend

54d0888

github-actions bot added the test label Jul 12, 2024

andrey-canon approved these changes Jul 12, 2024

View reviewed changes

johanseto marked this pull request as ready for review July 12, 2024 21:32

johanseto merged commit a640038 into master Jul 12, 2024
7 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add audit for succesfull imports #192

feat: add audit for succesfull imports #192

johanseto commented Jul 8, 2024 •

edited

Loading

andrey-canon left a comment

johanseto commented Jul 9, 2024

andrey-canon Jul 10, 2024

andrey-canon Jul 10, 2024

andrey-canon Jul 10, 2024

andrey-canon Jul 10, 2024

andrey-canon Jul 10, 2024

andrey-canon Jul 10, 2024

andrey-canon Jul 10, 2024

johanseto Jul 10, 2024

johanseto Jul 10, 2024

andrey-canon Jul 10, 2024

andrey-canon Jul 10, 2024

andrey-canon left a comment

andrey-canon Jul 12, 2024

andrey-canon Jul 12, 2024

johanseto commented Jul 12, 2024

andrey-canon commented Jul 12, 2024

		cdd_import (dict, optional): Defaults to None.
		ead_import (dict, optional): Defaults to None.

feat: add audit for succesfull imports #192

feat: add audit for succesfull imports #192

Conversation

johanseto commented Jul 8, 2024 • edited Loading

Description

Testing instructions

Before

After

Additional information

Checklist for Merge

andrey-canon left a comment

Choose a reason for hiding this comment

johanseto commented Jul 9, 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

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

johanseto commented Jul 12, 2024

andrey-canon commented Jul 12, 2024

johanseto commented Jul 8, 2024 •

edited

Loading