webapi: Feature/fwf 4504 task outcome configuration #2730

auslin-aot · 2025-05-06T11:57:15Z

User description

Issue Tracking

JIRA:
Issue Type: BUG/ FEATURE
https://aottech.atlassian.net/browse/FWF-4504

Changes

Created new database table: TaskOutcomeConfiguration to store workflow transition rules
Implemented POST endpoint /tasks/task-outcome-configuration: Enables saving new task outcome configurations
Implemented GET endpoint /tasks/task-outcome-configuration/<task-id>: Retrieves configuration by taskId

Screenshots

Checklist

Updated changelog
Added meaningful title for pull request

PR Type

Enhancement, Tests, Documentation

Description

Add DB table and ORM model
Implement POST and GET endpoints
Create TaskService and schema
Add unit tests for Task API

Changes walkthrough 📝

Relevant files

Configuration changes

1 files

5ecbbfc545ba_task_outcome_configuration_table.py `Create task_outcome_configuration DB migration`	+42/-0

Error handling

1 files

__init__.py `Add TASK_OUTCOME_NOT_FOUND error code`	+4/-0

Enhancement

7 files

tasks.py `Add TaskOutcomeConfiguration ORM model`	+43/-0
__init__.py `Register Task API namespace`	+2/-0
tasks.py `Implement TaskOutcome API endpoints`	+136/-0
__init__.py `Import TaskOutcomeConfiguration schema`	+1/-0
tasks.py `Define TaskOutcomeConfiguration schema`	+28/-0
__init__.py `Include TaskService in service exports`	+2/-0
tasks.py `Add TaskService for configuration management`	+66/-0

Tests

2 files

test_task.py `Add tests for TaskOutcome API`	+125/-0
base_test.py `Add task_outcome_config_payload helper`	+12/-0

Documentation

1 files

CHANGELOG.md `Document TaskOutcome configuration changes`	+3/-0

Dependencies

1 files

dev.txt `Pin snowballstemmer to avoid incompatibility`	+2/-1

Need help?
Type /help how to ... in the comments thread for any questions about PR-Agent usage.
Check out the documentation for more information.

github-actions · 2025-05-06T11:58:36Z

PR Reviewer Guide 🔍

(Review updated until commit `6457a44`)

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review Validation Mismatch The allowed values for `transition_map_type` in the schema (["select","radio","checkbox","text","number"]) do not match the API documentation and frontend expectations ("select","radio","input"), which may allow unsupported types or reject valid ones. transition_map_type = fields.Str( data_key="transitionMapType", validate=lambda x: x in ["select", "radio", "checkbox", "text", "number"], required=True, ) Default Value Syntax The `server_default="select"` may not be correctly quoted in SQL, risking a default SQL expression error; using `sa.text("'select'")` or explicit quoting is more reliable. sa.Column('task_transition_map', sa.JSON(), nullable=False, comment='Task transition configuration'), sa.Column('transition_map_type', sa.String(length=100), nullable=False, server_default="select", comment='Task transition configuration type'), Status Code Convention The `TASK_OUTCOME_NOT_FOUND` error code maps to HTTPStatus.BAD_REQUEST (400) instead of a more semantically appropriate 404 Not Found, which could confuse API consumers. TASK_OUTCOME_NOT_FOUND = ( "Task outcome configuration not found for the given task Id", HTTPStatus.BAD_REQUEST, )

github-actions · 2025-05-06T11:59:22Z

PR Code Suggestions ✨

Latest suggestions up to 6457a44

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	enforce composite uniqueness Enforce uniqueness on the composite key of `tenant` and `task_id` and remove the standalone unique index on `task_id` to support multi-tenant entries with the same task ID. Adjust the migration to make the composite index unique. forms-flow-api/migrations/versions/5ecbbfc545ba_task_outcome_configuration_table.py [32-33] -op.create_index('idx_task_id_and_tenant', 'task_outcome_configuration', ['tenant', 'task_id'], unique=False) -op.create_index(op.f('ix_task_outcome_configuration_task_id'), 'task_outcome_configuration', ['task_id'], unique=True) +op.create_index('idx_task_id_and_tenant', 'task_outcome_configuration', ['tenant', 'task_id'], unique=True) Suggestion importance[1-10]: 8 __ Why: The composite index should be made unique to support per-tenant task IDs and the standalone unique index on `task_id` would block this; fixing it prevents a critical multi-tenant constraint bug.	Medium
Possible issue	remove global task_id uniqueness Remove the standalone unique and index constraints on `task_id` so uniqueness is enforced per tenant via the composite index in `table_args`. This allows multiple tenants to have configurations for the same task. forms-flow-api/src/formsflow_api/models/tasks.py [16-18] task_id = db.Column( - db.String(100), nullable=False, comment="Task ID", unique=True, index=True + db.String(100), nullable=False, comment="Task ID" ) Suggestion importance[1-10]: 7 __ Why: Removing the `unique=True, index=True` on `task_id` aligns the model with the intended composite uniqueness in `__table_args__` and avoids cross-tenant collisions.	Medium
General	tighten transition type validation Replace the lambda validator with `validate.OneOf` to clearly define allowed types and include `"input"` instead of unsupported types. This provides better error messages and enforces the intended map types. forms-flow-api/src/formsflow_api/schemas/tasks.py [23-27] +from marshmallow import validate + transition_map_type = fields.Str( data_key="transitionMapType", - validate=lambda x: x in ["select", "radio", "checkbox", "text", "number"], + validate=validate.OneOf(["select", "radio", "input"]), required=True, ) Suggestion importance[1-10]: 6 __ Why: Replacing a lambda with `validate.OneOf` and restricting to the documented types (`select`, `radio`, `input`) improves validation accuracy and error messages.	Low

Previous suggestions

Suggestions up to commit 62c965a

Category	Suggestion	Impact
Possible issue	Use correct 404 status A missing resource typically maps to HTTP 404. Update the error code to `HTTPStatus.NOT_FOUND` so clients receive the correct status when the task outcome is not found. forms-flow-api/src/formsflow_api/constants/init.py [141-144] TASK_OUTCOME_NOT_FOUND = ( "Task outcome configuration not found for the given task Id", - HTTPStatus.BAD_REQUEST, + HTTPStatus.NOT_FOUND, ) Suggestion importance[1-10]: 8 __ Why: Returning HTTP 404 for a missing resource follows REST conventions and provides a more accurate status code than 400 for "not found" errors.	Medium
	Quote server default correctly Wrap the default value in a SQL expression so the database interprets it as a literal string, preventing accidental SQL parsing. Use `sa.text("'select'")` instead of a bare `"select"`. forms-flow-api/migrations/versions/5ecbbfc545ba_task_outcome_configuration_table.py [26] -sa.Column('transition_map_type', sa.String(length=100), nullable=False, server_default="select", comment='Task transition configuration type'), +sa.Column('transition_map_type', sa.String(length=100), nullable=False, server_default=sa.text("'select'"), comment='Task transition configuration type'), Suggestion importance[1-10]: 6 __ Why: Wrapping the DB default in `sa.text` ensures the literal string `'select'` is set at the database level and avoids unintended SQL parsing issues.	Low
	Add server_default to model field Ensure the database column has the same default at the schema level by adding `server_default`. This aligns the ORM model with the migration and avoids missing defaults on direct inserts. forms-flow-api/src/formsflow_api/models/tasks.py [23-28] transition_map_type = db.Column( db.String(100), nullable=False, default="select", + server_default="select", comment="Task transition configuration type", ) Suggestion importance[1-10]: 5 __ Why: Adding `server_default="select"` to the ORM column aligns the model with the migration and preserves the default on direct DB inserts, improving consistency.	Low

Suggestions up to commit eca9e06

Category	Suggestion	Impact
Possible issue	Quote default value correctly Wrap the default string in quotes using `sa.text` to ensure SQL interprets it as a literal and avoids keyword collisions. This guarantees the default value is correctly quoted in the migration. forms-flow-api/migrations/versions/5ecbbfc545ba_task_outcome_configuration_table.py [26] -sa.Column('transition_map_type', sa.String(length=100), nullable=False, server_default="select", comment='Task transition configuration type'), +sa.Column('transition_map_type', sa.String(length=100), nullable=False, server_default=sa.text("'select'"), comment='Task transition configuration type'), Suggestion importance[1-10]: 6 __ Why: Wrapping the column `server_default` in `sa.text("'select'")` ensures the SQL literal is correctly quoted and avoids potential keyword collisions.	Low
General	Remove redundant column default Remove the unnecessary Python-side `default` since the API schema always provides this value. This prevents drift between application defaults and database defaults. forms-flow-api/src/formsflow_api/models/tasks.py [23-28] transition_map_type = db.Column( db.String(100), nullable=False, - default="select", comment="Task transition configuration type", ) Suggestion importance[1-10]: 5 __ Why: The Python-side `default="select"` duplicates the database default from the migration and may drift, so removing it simplifies the ORM model without changing behavior.	Low
General	Correct return type annotation Update the return type annotation to accurately reflect that this method returns a serialized `dict` rather than a model instance. This makes the signature more precise. forms-flow-api/src/formsflow_api/services/tasks.py [18-20] def create_task_outcome_configuration( self, request_data: dict, **kwargs -) -> TaskOutcomeConfiguration \| None: +) -> dict: Suggestion importance[1-10]: 5 __ Why: The `create_task_outcome_configuration` method returns a serialized `dict` from `task_outcome_schema.dump`, so updating the annotation to `dict` makes the signature accurate.	Low

Suggestions up to commit 45bd55e

Category	Suggestion	Impact
General	Add composite unique constraint Enforce multi-tenant data isolation by removing the global uniqueness on `task_id` and adding a composite unique constraint on `(tenant, task_id)` in the model. This prevents conflicts across different tenants. forms-flow-api/src/formsflow_api/models/tasks.py [16-18] task_id = db.Column( - db.String(100), nullable=False, comment="Task ID", unique=True, index=True + db.String(100), nullable=False, comment="Task ID", index=True +) +__table_args__ = ( + Index("idx_task_id_and_tenant", "tenant", "task_id"), + db.UniqueConstraint("tenant", "task_id", name="uq_task_outcome_configuration_tenant_task_id"), ) Suggestion importance[1-10]: 7 __ Why: Adding a `(tenant, task_id)` unique constraint enforces multi-tenant isolation and prevents cross-tenant key conflicts.	Medium
	Enforce enum validation in API Restrict `transitionMapType` to the allowed set (`select`, `radio`, `input`) by specifying the `enum` parameter in the field definition. This adds automatic validation at the API layer. forms-flow-api/src/formsflow_api/resources/tasks.py [23-25] "transitionMapType": fields.String( - description="Task transition map type - select/input/radio", required=True + description="Task transition map type - select/input/radio", required=True, enum=["select", "radio", "input"] ), Suggestion importance[1-10]: 6 __ Why: Specifying `enum=["select", "radio", "input"]` adds schema-level validation for allowed transition types, improving API correctness.	Low
	Use text clause for default Use a SQL text clause for the server default to ensure the literal is correctly quoted when generating DDL. This prevents database errors due to unquoted default strings. Replace the raw string with `sa.text("'select'")`. forms-flow-api/migrations/versions/5ecbbfc545ba_task_outcome_configuration_table.py [26] -sa.Column('transition_map_type', sa.String(length=100), nullable=False, server_default="select", comment='Task transition configuration type'), +sa.Column('transition_map_type', sa.String(length=100), nullable=False, server_default=sa.text("'select'"), comment='Task transition configuration type'), Suggestion importance[1-10]: 5 __ Why: Using `sa.text("'select'")` ensures the default string is properly quoted in DDL, avoiding potential SQL errors.	Low

Suggestions up to commit 07cc62d

Category	Suggestion	Impact
Possible issue	Add updated timestamp column The migration table schema should include the `updated` timestamp column defined in `ApplicationAuditDateTimeMixin` to keep model and database in sync. Add an `updated` column right after `created`. forms-flow-api/migrations/versions/5ecbbfc545ba_task_outcome_configuration_table.py [29] sa.Column('created', sa.DateTime(timezone=True), nullable=False), +sa.Column('updated', sa.DateTime(timezone=True), nullable=True), Suggestion importance[1-10]: 8 __ Why: The migration is missing the `updated` audit column, so aligning the DB schema with `ApplicationAuditDateTimeMixin` ensures consistency and proper timestamp tracking.	Medium
General	Use server_default in model Switch from a client-side `default` to a `server_default` so that the database will automatically apply the `"select"` value on inserts without relying on the ORM. forms-flow-api/src/formsflow_api/models/tasks.py [23-28] transition_map_type = db.Column( db.String(100), nullable=False, - default="select", + server_default="select", comment="Task transition configuration type", ) Suggestion importance[1-10]: 5 __ Why: Switching from a client-side `default` to `server_default` makes the database apply the default without ORM intervention, improving consistency.	Low
General	Use SQL text for default Use a SQL literal expression for `server_default` so the default value is correctly emitted in the DDL. Wrapping in `sa.text` ensures proper quoting across databases. forms-flow-api/migrations/versions/5ecbbfc545ba_task_outcome_configuration_table.py [26] -sa.Column('transition_map_type', sa.String(length=100), nullable=False, server_default="select", comment='Task transition configuration type'), +sa.Column('transition_map_type', sa.String(length=100), nullable=False, server_default=sa.text("'select'"), comment='Task transition configuration type'), Suggestion importance[1-10]: 4 __ Why: Wrapping the `server_default` in `sa.text` can help emit the default value correctly across databases, but the impact is minor.	Low

github-actions · 2025-05-06T12:05:13Z

Persistent review updated to latest commit 535b5d0

github-actions · 2025-05-06T12:13:55Z

Persistent review updated to latest commit 997039f

github-actions · 2025-05-07T06:49:00Z

Persistent review updated to latest commit d5c713d

github-actions · 2025-05-07T07:31:51Z

Persistent review updated to latest commit 203f640

github-actions · 2025-05-07T07:37:19Z

Persistent review updated to latest commit c3be1c8

github-actions · 2025-05-07T08:27:03Z

Persistent review updated to latest commit 07cc62d

github-actions · 2025-05-07T08:27:11Z

Persistent review updated to latest commit 07cc62d

forms-flow-api/src/formsflow_api/models/tasks.py

forms-flow-api/src/formsflow_api/resources/tasks.py

github-actions · 2025-05-08T09:13:06Z

Persistent review updated to latest commit 45bd55e

…stemmer>=3

github-actions · 2025-05-08T10:35:48Z

Persistent review updated to latest commit eca9e06

arun-s-aot · 2025-05-13T17:34:21Z

@salabh-aot Please review this PR

salabh-aot · 2025-05-14T05:46:07Z

forms-flow-api/src/formsflow_api/schemas/tasks.py

+    task_transition_map = fields.Raw(
+        data_key="taskTransitionMap", required=True
+    )  # Accepts list, dict, string
+    transition_map_type = fields.Str(data_key="transitionMapType", required=True)


Is transition_map_type used as reference for input field type in frontend? If yes, it would be better to limit the choices, may be using an Enum. Adds an additional validation.

salabh-aot · 2025-05-14T05:46:15Z

forms-flow-api/src/formsflow_api/models/tasks.py

+        if tenant is not None:
+            query = query.filter_by(tenant=tenant)
+        task_outcome = query.first()
+        return task_outcome if task_outcome else None


Doesn't query.first() return None if there are no results? We can return task_outcome and avoid the if ... else ...

forms-flow-api/requirements/dev.txt

CHANGELOG.md

github-actions · 2025-05-14T06:02:46Z

Persistent review updated to latest commit 62c965a

arun-s-aot · 2025-05-14T09:47:22Z

@salabh-aot please check and approve if everything seems ok

salabh-aot

Looks good to me

github-actions · 2025-05-14T11:13:11Z

Persistent review updated to latest commit 6457a44

sonarqubecloud · 2025-05-14T11:13:42Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

FWF-4504: [Feature] Task outcome configuration model and APIs

4a69674

github-actions bot added the Review effort 2/5 label May 6, 2025

auslin-aot force-pushed the feature/fwf-4504-task-outcome-configuration-webapi branch from bbd69f8 to 535b5d0 Compare May 6, 2025 12:04

auslin-aot force-pushed the feature/fwf-4504-task-outcome-configuration-webapi branch from 535b5d0 to 997039f Compare May 6, 2025 12:12

github-actions bot added Review effort 3/5 and removed Review effort 2/5 labels May 6, 2025

FWF-4504: [Feature] Task outcome configuration model and APIs

d5c713d

auslin-aot force-pushed the feature/fwf-4504-task-outcome-configuration-webapi branch from 997039f to d5c713d Compare May 7, 2025 06:47

auslin-aot force-pushed the feature/fwf-4504-task-outcome-configuration-webapi branch from 203f640 to c3be1c8 Compare May 7, 2025 07:35

FWF-4504: [Feature] Add test case

07cc62d

auslin-aot force-pushed the feature/fwf-4504-task-outcome-configuration-webapi branch from c3be1c8 to 07cc62d Compare May 7, 2025 08:25

auslin-aot marked this pull request as ready for review May 7, 2025 08:25

auslin-aot requested review from a team as code owners May 7, 2025 08:25

shuhaib-aot approved these changes May 8, 2025

View reviewed changes

forms-flow-api/src/formsflow_api/models/tasks.py Outdated Show resolved Hide resolved

forms-flow-api/src/formsflow_api/resources/tasks.py Show resolved Hide resolved

FWF-4504: [Feature] Added detailed api doc & index on taskId, tenant

45bd55e

FWF-4504: [Bugfix] Fix flake8-docstring incompatibility with snowball…

eca9e06

…stemmer>=3

salabh-aot reviewed May 14, 2025

View reviewed changes

forms-flow-api/requirements/dev.txt Show resolved Hide resolved

salabh-aot reviewed May 14, 2025

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

FWF-4504: [Modification]PR comments

62c965a

salabh-aot approved these changes May 14, 2025

View reviewed changes

FWF-4504: [Added] Schema level validation for transition_map_type

6457a44

shuhaib-aot merged commit 9f0282e into AOT-Technologies:develop May 15, 2025
10 checks passed

webapi: Feature/fwf 4504 task outcome configuration #2730

webapi: Feature/fwf 4504 task outcome configuration #2730

Uh oh!

Conversation

auslin-aot commented May 6, 2025 • edited by github-actions bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

User description

Issue Tracking

Changes

Screenshots

Checklist

PR Type

Description

Changes walkthrough 📝

Uh oh!

github-actions bot commented May 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Reviewer Guide 🔍

(Review updated until commit 6457a44)

Uh oh!

github-actions bot commented May 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Code Suggestions ✨

Previous suggestions

Uh oh!

github-actions bot commented May 6, 2025

Uh oh!

github-actions bot commented May 6, 2025

Uh oh!

github-actions bot commented May 7, 2025

Uh oh!

github-actions bot commented May 7, 2025

Uh oh!

github-actions bot commented May 7, 2025

Uh oh!

github-actions bot commented May 7, 2025

Uh oh!

github-actions bot commented May 7, 2025

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented May 8, 2025

Uh oh!

github-actions bot commented May 8, 2025

Uh oh!

arun-s-aot commented May 13, 2025

Uh oh!

salabh-aot May 14, 2025

Choose a reason for hiding this comment

Uh oh!

salabh-aot May 14, 2025

Choose a reason for hiding this comment

Uh oh!

auslin-aot May 14, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented May 14, 2025

Uh oh!

arun-s-aot commented May 14, 2025

Uh oh!

salabh-aot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented May 14, 2025

Uh oh!

sonarqubecloud bot commented May 14, 2025

Quality Gate passed

Uh oh!

Uh oh!

Uh oh!

auslin-aot commented May 6, 2025 •

edited by github-actions bot

Loading

github-actions bot commented May 6, 2025 •

edited

Loading

(Review updated until commit `6457a44`)

github-actions bot commented May 6, 2025 •

edited

Loading