Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions api/howler/actions/demote.py
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ def execute(
*hit_helper.demote_hit(escalation=escalation),
odm_helper.update("howler.assessment", None),
odm_helper.update("howler.rationale", None),
odm_helper.update("howler.assignment", None),
odm_helper.update("howler.triaged", None),
],
)
else:
Expand All @@ -94,7 +94,7 @@ def execute(
ds.hit.update_by_query(
query,
[
*hit_helper.assess_hit(assessment, rationale, user=(user if user else "automation")),
*hit_helper.assess_hit(assessment, rationale),
odm_helper.update(
"howler.assignment",
user.get("uname", "automation") if user else "automation",
Expand Down
8 changes: 2 additions & 6 deletions api/howler/actions/promote.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@
AssessmentEscalationMap,
Escalation,
)
from howler.odm.models.user import User
from howler.utils.str_utils import sanitize_lucene_query

OPERATION_ID = "promote"
Expand All @@ -27,7 +26,6 @@ def execute(
escalation: Escalation = Escalation.ALERT,
assessment: Optional[str] = None,
rationale: Optional[str] = None,
user: Optional[User] = None,
**kwargs,
):
"""Promote a hit.
Expand Down Expand Up @@ -75,7 +73,7 @@ def execute(
*hit_helper.promote_hit(escalation=escalation),
odm_helper.update("howler.assessment", None),
odm_helper.update("howler.rationale", None),
odm_helper.update("howler.assignment", None),
odm_helper.update("howler.triaged", None),
],
)
else:
Expand All @@ -90,9 +88,7 @@ def execute(
)
return report

ds.hit.update_by_query(
query, hit_helper.assess_hit(assessment, rationale, user=(user if user else "automation"))
)
ds.hit.update_by_query(query, hit_helper.assess_hit(assessment, rationale))

report.append(
{
Expand Down
19 changes: 10 additions & 9 deletions api/howler/helper/hit.py
Original file line number Diff line number Diff line change
Expand Up @@ -24,14 +24,11 @@ def assess_hit(
assessment: Optional[str] = None,
rationale: Optional[str] = None,
hit: Optional[Union[dict[str, Any], Hit]] = None,
*,
user: User | str,
**kwargs,
) -> list[OdmUpdateOperation]:
"""Update the assessment and esclation of a hit

Args:
user (User | str): The user making the assessment
assessment (Optional[str], optional): The assessment to set the hit to. Defaults to None.
hit (Optional[Union[dict[str, Any], Hit]], optional): The hit to update. Defaults to None.

Expand All @@ -55,6 +52,14 @@ def assess_hit(
if assessment is None and rationale:
rationale = None

if assessment is None:
# reset the timestamp and set state to in progress if removing assessment (re-assessing)
triaged_timestamp = None
status = Status.IN_PROGRESS
else:
triaged_timestamp = "NOW"
status = Status.RESOLVED

Comment on lines +58 to +62
logger.debug(
"Updating assessment of %s to %s",
hit["howler"]["id"] if hit else "unknown",
Expand All @@ -66,16 +71,12 @@ def assess_hit(
escalation,
)

if assessment is None:
assessor_id = None
else:
assessor_id = user.get("uname", user.get("username", None)) if isinstance(user, User) else user

return [
odm_helper.update("howler.assessment", assessment),
odm_helper.update("howler.escalation", escalation),
odm_helper.update("howler.rationale", rationale, silent=True),
odm_helper.update("howler.assessor", assessor_id, silent=True),
odm_helper.update("howler.triaged", triaged_timestamp),

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't directly related, but also set howler.status to resolved.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Context: howler.status is set properly when hit is assessed through a transition, but it isn't set when calling assess_hit directly (from promote / demote actions).

To match the the only transition out of resolved, if an assessment is removed by assess_hit we can set howler.status to in progress.

With the current workflow implementation, if an action sets the status, it takes priority over the transition defined destination status. I think it should be the other way around or even fail if values don't match. Personally, if there's a destination defined in the transition states, I expect this to be the behaviour, with any status changes in actions only being a fallback.

odm_helper.update("howler.status", status),
]


Expand Down
8 changes: 7 additions & 1 deletion api/howler/helper/workflow.py
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,13 @@ def transition(self, current_status: str, transition: str, **kwargs) -> list[Odm

updates_dict[update.key] = update

if self.status_prop not in updates_dict and _transition.get("dest", False):
if _transition.get("dest", False):
if self.status_prop in updates_dict and updates_dict[self.status_prop].value != _transition["dest"]:
raise WorkflowException(
f"Transition {transition} attempted to update the status {self.status_prop} with \
a different value than the transition destination."
)

updates_dict[self.status_prop] = OdmUpdateOperation(
ESCollection.UPDATE_SET,
self.status_prop,
Expand Down
2 changes: 1 addition & 1 deletion api/howler/odm/helper.py
Original file line number Diff line number Diff line change
Expand Up @@ -178,10 +178,10 @@ def generate_useful_hit( # noqa: C901

hit.howler.assessment = None
hit.howler.rationale = None
hit.howler.triaged = None
hit.howler.status = "open"
hit.howler.assignment = "unassigned"
hit.howler.escalation = choice([Escalation.HIT, Escalation.ALERT])
hit.howler.assessor = None

if randint(1, 10) > 9:
hit.howler.expiry = datetime.now() + timedelta(days=randint(1, 60))
Expand Down
8 changes: 2 additions & 6 deletions api/howler/odm/models/howler_data.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
# mypy: ignore-errors
from datetime import datetime
from typing import Optional

from howler import odm
Expand Down Expand Up @@ -180,12 +181,6 @@ class HowlerData(odm.Model):
description="Unique identifier of the assigned user.",
default=DEFAULT_ASSIGNMENT,
)
assessor: Optional[str] = odm.Optional(
odm.Keyword(
description="The most recent person to assess a hit",
default=None,
)
)
data: list[str] = odm.List(
odm.Keyword(description="Raw telemetry records associated with this hit."),
default=[],
Expand Down Expand Up @@ -248,6 +243,7 @@ class HowlerData(odm.Model):
)
)
)
triaged: Optional[datetime] = odm.Optional(odm.Date(description="Timestamp at which the hit was triaged."))
comment: list[Comment] = odm.List(
odm.Compound(Comment),
default=[],
Expand Down
1 change: 0 additions & 1 deletion api/howler/odm/random_data.py
Original file line number Diff line number Diff line change
Expand Up @@ -595,7 +595,6 @@ def create_hits(ds: HowlerDatastore, hit_count: int = 200):
hit.howler.id,
[
*assess_hit(
user=user,
assessment=choice(Assessment.list()),
rationale=get_random_string(),
hit=hit,
Expand Down
2 changes: 1 addition & 1 deletion api/howler/services/hit_service.py
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ def get_hit_workflow() -> Workflow:
"source": [Status.OPEN, Status.IN_PROGRESS],
"transition": HitStatusTransition.ASSESS,
"dest": Status.RESOLVED,
"actions": [assess_hit],
"actions": [assess_hit, assign_hit],
}
),
Transition(
Expand Down
44 changes: 30 additions & 14 deletions api/test/integration/api/test_hit_transition.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import datetime
import json
from typing import Any, Optional
from typing import Any, Literal, Optional

import pytest

Expand Down Expand Up @@ -42,11 +43,8 @@ def datastore(datastore_connection: HowlerDatastore):
wipe_hits(datastore_connection)


def test_full_transition_flow(datastore: HowlerDatastore, login_session): # noqa: C901
"""Test that /api/v1/hit/<id>/transitions/start endpoint performs the correct transition"""
session, host = login_session

assert datastore.hit.get(HIT_ID).howler.status == Status.OPEN
@pytest.fixture(scope="module")
def transition_data(datastore: HowlerDatastore) -> list[dict[str, Any]]:

def check_assignment(user: str):
def check():
Expand All @@ -66,23 +64,30 @@ def check():

return check

def check_assessor(user: str):
def check_triaged(assessment_time: Literal["NOW"] | None):
def check():
assert datastore.hit.get(HIT_ID).howler.assessor == user
tolerance = datetime.timedelta(seconds=10)

triaged_timestamp = datastore.hit.get(HIT_ID).howler.triaged
if assessment_time is None:
assert triaged_timestamp is None
else:
assert triaged_timestamp is not None
assert abs(triaged_timestamp - datetime.datetime.now(datetime.timezone.utc)) < tolerance

return check

transition_data: list[dict[str, Any]] = [
return [
{
"transition": HitStatusTransition.ASSESS,
"data": {"assessment": Assessment.AMBIGUOUS},
"dest": Status.RESOLVED,
"check": [check_assessor("admin")],
"check": [check_assignment("admin"), check_triaged("NOW")],
},
{
"transition": HitStatusTransition.RE_EVALUATE,
"dest": Status.IN_PROGRESS,
"check": [check_assessment(None), check_assignment("admin"), check_assessor(None)],
"check": [check_assessment(None), check_assignment("admin"), check_triaged(None)],
},
{
"transition": HitStatusTransition.RELEASE,
Expand Down Expand Up @@ -148,12 +153,16 @@ def check():
"transition": HitStatusTransition.ASSESS,
"data": {"assessment": Assessment.AMBIGUOUS},
"dest": Status.RESOLVED,
"check": [check_assessment(Assessment.AMBIGUOUS), check_assessor("admin")],
"check": [
check_assessment(Assessment.AMBIGUOUS),
check_assignment("admin"),
check_triaged("NOW"),
],
},
{
"transition": HitStatusTransition.RE_EVALUATE,
"dest": Status.IN_PROGRESS,
"check": [check_assessment(None), check_assignment("admin"), check_assessor(None)],
"check": [check_assessment(None), check_assignment("admin"), check_triaged(None)],
},
{
"transition": HitStatusTransition.RELEASE,
Expand All @@ -168,6 +177,13 @@ def check():
},
]


def test_full_transition_flow(transition_data, datastore, login_session):
"""Test that /api/v1/hit/<id>/transitions/start endpoint performs the correct transition"""
session, host = login_session

assert datastore.hit.get(HIT_ID).howler.status == Status.OPEN

for data in transition_data:
checks = data.pop("check", None)
_, version = datastore.hit.get(HIT_ID, as_obj=False, version=True)
Comment thread
cccs-hxp marked this conversation as resolved.
Expand All @@ -186,7 +202,7 @@ def check():
for c in checks:
c()

datastore.hit.get(HIT_ID).howler.status == data["dest"]
assert datastore.hit.get(HIT_ID).howler.status == data["dest"]

# hit: Hit = datastore.hit.get(HIT_ID, as_obj=False)
# assert hit["howler"]["status"] == Status.IN_PROGRESS
Expand Down
70 changes: 69 additions & 1 deletion api/test/unit/helper/test_workflow.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@
from howler.datastore.operations import OdmUpdateOperation
from howler.helper.workflow import Transition, Workflow, WorkflowException

DUMMY_WORKFLOW_STATE_KEY = "prop"

DUMMY_WORKFLOW_TRANSITIONS = [
Transition({"transition": "first", "source": "state1", "dest": "state2", "actions": []}),
Transition(
Expand All @@ -17,8 +19,52 @@
]


@pytest.fixture(scope="module")
def workflow_with_state_setting_action():
return Workflow(
DUMMY_WORKFLOW_STATE_KEY,
[
*DUMMY_WORKFLOW_TRANSITIONS,
Transition(
{
"transition": "legal-set-state",
"source": "state1",
"dest": "state3",
"actions": [
lambda **kwargs: [
OdmUpdateOperation(ESCollection.UPDATE_SET, DUMMY_WORKFLOW_STATE_KEY, "state3")
]
],
}
),
],
)


@pytest.fixture(scope="module")
def workflow_with_invalid_set_state_action():
return Workflow(
DUMMY_WORKFLOW_STATE_KEY,
[
*DUMMY_WORKFLOW_TRANSITIONS,
Transition(
{
"transition": "illegal-set-state",
"source": "state1",
"dest": "state3",
"actions": [
lambda **kwargs: [
OdmUpdateOperation(ESCollection.UPDATE_REMOVE, DUMMY_WORKFLOW_STATE_KEY, "state2")
]
],
}
),
],
)


def test_workflow():
workflow: Workflow = Workflow("prop", DUMMY_WORKFLOW_TRANSITIONS)
workflow: Workflow = Workflow(DUMMY_WORKFLOW_STATE_KEY, DUMMY_WORKFLOW_TRANSITIONS)

assert len(workflow.transitions) == 2

Expand All @@ -37,6 +83,28 @@ def test_workflow():
assert updates_second[1].value == "state1"


def test_workflow_with_state_setting_action(workflow_with_state_setting_action):
workflow: Workflow = workflow_with_state_setting_action

assert len(workflow.transitions) == 3

# Run the "legal-set-state" transition
updates: list[OdmUpdateOperation] = workflow.transition("state1", "legal-set-state")
assert len(updates) == 1
assert updates[0].key == DUMMY_WORKFLOW_STATE_KEY
assert updates[0].value == "state3"


def test_workflow_with_invalid_set_state_action(workflow_with_invalid_set_state_action):
workflow: Workflow = workflow_with_invalid_set_state_action

assert len(workflow.transitions) == 3

# Run the "illegal-set-state" transition
with pytest.raises(WorkflowException):
workflow.transition("state1", "illegal-set-state")


def test_workflow_missing_transition_props():
with pytest.raises(WorkflowException):
Workflow(
Expand Down
Loading
Loading