Api v2 delete global task #975
Conversation
…n_alert_filter_is_not_found
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis pull request refactors hook invocation signatures across the codebase to use positional arguments instead of keyword arguments, implements global task deletion functionality, consolidates database deletion operations through a centralized Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Areas requiring extra attention:
Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](/s/com/githubusercontent/avatars/G.https/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
source/app/datamgmt/datastore/datastore_db.py (1)
403-554: datastore_filter_tree now returns a tuple; ensure all callers are updated.
datastore_filter_treenow returns(path_tree, 'Success')and(None, str(e))on error, rather than justpath_tree:except Exception as e: return None, str(e) ... return path_tree, 'Success'This is a public contract change; any existing callers that expect a single return value will now get a tuple and likely break.
Please confirm that:
- All internal call sites have been updated to unpack
(tree, status)(or similar), and- The new status string is handled consistently (or consider returning a richer status object/enum if this is part of a broader pattern).
source/app/blueprints/rest/case/case_assets_routes.py (1)
407-420: Comment deletion misses asset/case binding, can break referential integrity.The new logic:
comment = get_comment(iris_current_user, com_id) if not comment: return response_error('You are not allowed to delete this comment') delete_asset_comment(cur_id, comment) call_modules_hook('on_postload_asset_comment_delete', com_id, caseid=caseid)has good intent (only the author can delete their comment) but misses a key check:
get_commentonly filters bycomment_idandcomment_user_id, not by asset or case.delete_asset_comment(cur_id, comment)(incase_assets_db) assumes the comment is linked to the given asset viaAssetComments. If it isn’t, the association row is not deleted, butdb_delete(comment)still removes thecommentsrow, leading to:
- A likely FK violation if
asset_comments.comment_idhas a foreign key, or- Orphaned
asset_commentsrows if the DB doesn’t enforce referential integrity.A safer pattern is to ensure the comment is both:
- Linked to the specified asset, and
- Owned by the current user.
You already have
get_case_asset_commentimported; we can re-use it:def case_comment_asset_delete(cur_id, com_id, caseid): - comment = get_comment(iris_current_user, com_id) - if not comment: - return response_error('You are not allowed to delete this comment') - - delete_asset_comment(cur_id, comment) - - call_modules_hook('on_postload_asset_comment_delete', com_id, caseid=caseid) + # Ensure the comment belongs to this asset + comment = get_case_asset_comment(cur_id, com_id) + if not comment or comment.comment_user_id != iris_current_user.id: + return response_error('You are not allowed to delete this comment') + + delete_asset_comment(cur_id, comment) + + call_modules_hook('on_postload_asset_comment_delete', com_id, caseid=caseid)This guarantees we only delete comments that are:
- Attached to the asset identified by
cur_id, and- Authored by the current user,
and avoids inconsistent
AssetComments/Commentsstate.
🧹 Nitpick comments (3)
source/app/datamgmt/case/case_db.py (1)
269-278: Use.is_(None)for SQLAlchemy NULL comparisons.Line 277 uses
== Nonewhich can work but SQLAlchemy best practices recommend using.is_(None)for explicit NULL comparisons to ensure proper SQL generation across different database backends.Apply this diff:
return Cases.query.filter( Cases.owner_id == user_identifier, - Cases.close_date == None + Cases.close_date.is_(None) ).all()source/app/blueprints/rest/dashboard_routes.py (1)
36-41: Hook refactor and global task imports look good; minor consistency nit.
- Importing
list_user_cases,list_user_reviews,list_user_tasks, and global task helpers from their dedicated datamgmt modules is cleaner and aligns with the PR’s refactor.- Switching to positional
datawhen callingcall_modules_hookinadd_gtaskandgtask_deleteis correct and matches the helper’s signature.One small consistency nit:
gtask = call_modules_hook( 'on_postload_global_task_update', data=gtask, caseid=caseid)Everywhere else in this file you now pass
datapositionally. You may want to align this call for style/readability:- gtask = call_modules_hook( - 'on_postload_global_task_update', data=gtask, caseid=caseid) + gtask = call_modules_hook( + 'on_postload_global_task_update', gtask, caseid=caseid)Given the endpoint is being deprecated, the existing manual delete logic in
gtask_deleteis acceptable, but if you want to centralize behavior you could also delegate to the new global‑task delete helper later.Also applies to: 203-236, 255-265, 274-295
source/app/datamgmt/global_tasks.py (1)
21-29:delete_global_taskworks but could be more defensive and reuse existing helpers.The new helper:
def delete_global_task(task: GlobalTasks): GlobalTasks.query.filter(GlobalTasks.id == task.id).delete() db.session.commit()is straightforward, but a couple of points to consider:
- It assumes
taskis a non‑NoneGlobalTasksinstance; if a caller passesNone, this will raiseAttributeError. If callers are guaranteed to resolve/validate beforehand (e.g. via a dedicatedget/operationslayer that raisesObjectNotFoundError), that’s fine; otherwise, a defensive early return or explicit exception would make the contract clearer.- Elsewhere in the codebase, deletions are being centralized via
db_delete(element)inapp.datamgmt.db_operations. For consistency you might want to usedb_delete(task)here instead of a bulk query delete, unless you intentionally want to avoid loading the instance.Example defensive variant:
-def delete_global_task(task: GlobalTasks): - GlobalTasks.query.filter(GlobalTasks.id == task.id).delete() - db.session.commit() +def delete_global_task(task: GlobalTasks): + if task is None: + return + + db.session.delete(task) + db.session.commit()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (43)
.vulture.ignore(1 hunks)pyproject.toml(1 hunks)source/app/blueprints/pages/dashboard/dashboard_routes.py(1 hunks)source/app/blueprints/rest/alerts_routes.py(12 hunks)source/app/blueprints/rest/case/case_assets_routes.py(7 hunks)source/app/blueprints/rest/case/case_evidences_routes.py(2 hunks)source/app/blueprints/rest/case/case_ioc_routes.py(4 hunks)source/app/blueprints/rest/case/case_notes_routes.py(4 hunks)source/app/blueprints/rest/case/case_tasks_routes.py(2 hunks)source/app/blueprints/rest/case/case_timeline_routes.py(7 hunks)source/app/blueprints/rest/dashboard_routes.py(6 hunks)source/app/blueprints/rest/dim_tasks_routes.py(1 hunks)source/app/blueprints/rest/manage/manage_cases_routes.py(2 hunks)source/app/blueprints/rest/v2/global_tasks.py(3 hunks)source/app/business/alerts.py(3 hunks)source/app/business/assets.py(2 hunks)source/app/business/cases.py(5 hunks)source/app/business/comments.py(8 hunks)source/app/business/evidences.py(3 hunks)source/app/business/global_tasks.py(3 hunks)source/app/business/iocs.py(3 hunks)source/app/business/notes.py(3 hunks)source/app/business/reports/reports.py(3 hunks)source/app/business/tasks.py(4 hunks)source/app/datamgmt/alerts/alerts_db.py(3 hunks)source/app/datamgmt/case/case_assets_db.py(2 hunks)source/app/datamgmt/case/case_db.py(1 hunks)source/app/datamgmt/case/case_events_db.py(2 hunks)source/app/datamgmt/case/case_iocs_db.py(2 hunks)source/app/datamgmt/case/case_notes_db.py(3 hunks)source/app/datamgmt/case/case_rfiles_db.py(2 hunks)source/app/datamgmt/case/case_tasks_db.py(3 hunks)source/app/datamgmt/client/client_db.py(2 hunks)source/app/datamgmt/comments.py(3 hunks)source/app/datamgmt/datastore/datastore_db.py(4 hunks)source/app/datamgmt/db_operations.py(1 hunks)source/app/datamgmt/global_tasks.py(2 hunks)source/app/datamgmt/iris_engine/modules_db.py(3 hunks)source/app/datamgmt/manage/manage_case_templates_db.py(4 hunks)source/app/datamgmt/manage/manage_groups_db.py(2 hunks)source/app/iris_engine/module_handler/module_handler.py(4 hunks)tests/tests_rest_global_tasks.py(1 hunks)tests/tests_rest_module_tasks.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (40)
source/app/business/notes.py (1)
source/app/iris_engine/module_handler/module_handler.py (1)
call_modules_hook(492-580)
tests/tests_rest_global_tasks.py (2)
source/app/blueprints/rest/v2/global_tasks.py (2)
create(43-51)delete(61-68)tests/iris.py (3)
create(60-61)delete(69-70)get(63-64)
tests/tests_rest_module_tasks.py (1)
tests/iris.py (1)
get_module_identifier_by_name(156-162)
source/app/blueprints/rest/case/case_ioc_routes.py (1)
source/app/iris_engine/module_handler/module_handler.py (1)
call_modules_hook(492-580)
source/app/datamgmt/manage/manage_groups_db.py (1)
source/app/datamgmt/db_operations.py (1)
db_delete(27-29)
source/app/datamgmt/case/case_iocs_db.py (1)
source/app/datamgmt/db_operations.py (1)
db_delete(27-29)
source/app/datamgmt/alerts/alerts_db.py (1)
source/app/datamgmt/db_operations.py (1)
db_delete(27-29)
source/app/business/assets.py (1)
source/app/iris_engine/module_handler/module_handler.py (1)
call_modules_hook(492-580)
source/app/datamgmt/case/case_notes_db.py (1)
source/app/datamgmt/db_operations.py (2)
db_create(22-24)db_delete(27-29)
source/app/datamgmt/comments.py (2)
source/app/datamgmt/db_operations.py (1)
db_delete(27-29)source/app/models/comments.py (1)
Comments(33-48)
source/app/datamgmt/case/case_events_db.py (1)
source/app/datamgmt/db_operations.py (1)
db_delete(27-29)
source/app/business/tasks.py (3)
source/app/datamgmt/case/case_tasks_db.py (1)
list_user_tasks(347-373)source/app/iris_engine/module_handler/module_handler.py (1)
call_modules_hook(492-580)source/app/datamgmt/states.py (1)
update_tasks_state(94-95)
source/app/blueprints/rest/case/case_notes_routes.py (1)
source/app/iris_engine/module_handler/module_handler.py (1)
call_modules_hook(492-580)
source/app/blueprints/rest/case/case_timeline_routes.py (1)
source/app/iris_engine/module_handler/module_handler.py (1)
call_modules_hook(492-580)
source/app/datamgmt/case/case_tasks_db.py (3)
source/app/datamgmt/db_operations.py (2)
db_create(22-24)db_delete(27-29)source/app/models/models.py (3)
CaseTasks(459-482)TaskStatus(450-456)TaskAssignee(514-524)source/app/models/cases.py (1)
Cases(46-114)
source/app/blueprints/rest/dim_tasks_routes.py (1)
source/app/iris_engine/module_handler/module_handler.py (1)
call_modules_hook(492-580)
source/app/datamgmt/case/case_db.py (2)
source/app/models/cases.py (1)
Cases(46-114)source/app/models/models.py (1)
ReviewStatus(688-692)
source/app/blueprints/rest/case/case_evidences_routes.py (1)
source/app/iris_engine/module_handler/module_handler.py (1)
call_modules_hook(492-580)
source/app/iris_engine/module_handler/module_handler.py (1)
source/app/datamgmt/iris_engine/modules_db.py (1)
deregister_module_from_hook(262-269)
source/app/business/alerts.py (1)
source/app/iris_engine/module_handler/module_handler.py (1)
call_modules_hook(492-580)
source/app/business/iocs.py (2)
source/app/iris_engine/module_handler/module_handler.py (1)
call_modules_hook(492-580)source/app/datamgmt/case/case_iocs_db.py (1)
delete_ioc(83-96)
source/app/datamgmt/iris_engine/modules_db.py (1)
source/app/models/models.py (2)
IrisModuleHook(614-628)IrisHook(606-611)
source/app/blueprints/pages/dashboard/dashboard_routes.py (1)
source/app/datamgmt/case/case_tasks_db.py (1)
get_tasks_status(41-42)
source/app/business/reports/reports.py (1)
source/app/iris_engine/module_handler/module_handler.py (1)
call_modules_hook(492-580)
source/app/business/evidences.py (1)
source/app/iris_engine/module_handler/module_handler.py (1)
call_modules_hook(492-580)
source/app/business/comments.py (2)
source/app/iris_engine/module_handler/module_handler.py (1)
call_modules_hook(492-580)source/app/datamgmt/case/case_assets_db.py (1)
delete_asset_comment(362-368)
source/app/datamgmt/case/case_rfiles_db.py (1)
source/app/datamgmt/db_operations.py (2)
db_create(22-24)db_delete(27-29)
source/app/blueprints/rest/v2/global_tasks.py (3)
source/app/blueprints/rest/endpoints.py (3)
response_api_deleted(55-56)response_api_success(29-30)response_api_not_found(72-73)source/app/business/global_tasks.py (2)
global_tasks_get(42-46)global_tasks_delete(49-55)source/app/datamgmt/global_tasks.py (2)
get_global_task(56-79)delete_global_task(27-29)
source/app/datamgmt/manage/manage_case_templates_db.py (1)
source/app/iris_engine/module_handler/module_handler.py (1)
call_modules_hook(492-580)
source/app/blueprints/rest/case/case_assets_routes.py (3)
source/app/datamgmt/comments.py (1)
get_comment(141-145)source/app/iris_engine/module_handler/module_handler.py (1)
call_modules_hook(492-580)source/app/datamgmt/case/case_assets_db.py (1)
delete_asset_comment(362-368)
source/app/datamgmt/client/client_db.py (3)
source/app/datamgmt/db_operations.py (1)
db_delete(27-29)source/app/models/errors.py (1)
ElementInUseError(49-51)source/app/models/customers.py (1)
Client(32-44)
source/app/blueprints/rest/case/case_tasks_routes.py (1)
source/app/iris_engine/module_handler/module_handler.py (1)
call_modules_hook(492-580)
source/app/business/global_tasks.py (3)
source/app/datamgmt/global_tasks.py (2)
delete_global_task(27-29)get_global_task_by_identifier(82-85)source/app/iris_engine/module_handler/module_handler.py (1)
call_modules_hook(492-580)source/app/iris_engine/utils/tracker.py (1)
track_activity(28-64)
source/app/blueprints/rest/alerts_routes.py (1)
source/app/iris_engine/module_handler/module_handler.py (1)
call_modules_hook(492-580)
source/app/business/cases.py (6)
source/app/datamgmt/case/case_db.py (4)
case_db_exists(78-80)list_user_cases(269-278)case_db_save(235-249)list_user_reviews(252-266)source/app/models/cases.py (2)
Cases(46-114)ReviewStatusList(199-204)source/app/iris_engine/module_handler/module_handler.py (1)
call_modules_hook(492-580)source/app/datamgmt/manage/manage_cases_db.py (1)
delete_case(403-458)source/app/iris_engine/utils/tracker.py (1)
track_activity(28-64)source/app/models/errors.py (1)
BusinessProcessingError(22-32)
source/app/datamgmt/datastore/datastore_db.py (1)
source/app/datamgmt/db_operations.py (2)
db_create(22-24)db_delete(27-29)
source/app/blueprints/rest/dashboard_routes.py (5)
source/app/datamgmt/case/case_db.py (2)
list_user_reviews(252-266)list_user_cases(269-278)source/app/datamgmt/case/case_tasks_db.py (2)
get_tasks_status(41-42)list_user_tasks(347-373)source/app/datamgmt/global_tasks.py (2)
list_global_tasks(32-53)get_global_task(56-79)source/app/iris_engine/module_handler/module_handler.py (1)
call_modules_hook(492-580)source/app/blueprints/rest/endpoints.py (1)
endpoint_deprecated(76-86)
source/app/datamgmt/case/case_assets_db.py (3)
source/app/datamgmt/db_operations.py (1)
db_delete(27-29)source/app/models/comments.py (2)
Comments(33-48)AssetComments(84-92)source/app/blueprints/rest/v2/assets_routes/comments.py (1)
delete(98-108)
source/app/blueprints/rest/manage/manage_cases_routes.py (1)
source/app/iris_engine/module_handler/module_handler.py (1)
call_modules_hook(492-580)
source/app/datamgmt/global_tasks.py (2)
source/app/models/models.py (2)
GlobalTasks(527-548)TaskStatus(450-456)source/app/blueprints/rest/v2/global_tasks.py (2)
delete_global_task(90-91)delete(61-68)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Database migration tests
- GitHub Check: End to end tests
- GitHub Check: Generate graphQL documentation
- GitHub Check: Test API
- GitHub Check: Test API
- GitHub Check: Database migration tests
- GitHub Check: End to end tests
- GitHub Check: Generate graphQL documentation
🔇 Additional comments (61)
source/app/blueprints/pages/dashboard/dashboard_routes.py (1)
27-27: Import refactoring verified as complete.The refactor is properly executed across the codebase. No stray imports from the old
app.datamgmt.dashboard.dashboard_dblocation remain, and all four usages ofget_tasks_statushave been updated to the new location inapp.datamgmt.case.case_tasks_db(dashboard_routes.py, rest/dashboard_routes.py, rest/case/case_tasks_routes.py, and pages/case/case_tasks_routes.py). The function behavior is unchanged and the refactoring aligns with the stated objective to reorganize persistence layer code by domain.source/app/business/alerts.py (1)
88-88: LGTM! Standardized hook calling convention.The migration from keyword arguments to positional arguments for
call_modules_hookinvocations is correct and consistent across all five call sites in this file. The changes align with the function signature and maintain existing behavior.Also applies to: 138-138, 141-141, 144-144, 164-164
source/app/blueprints/rest/manage/manage_cases_routes.py (1)
191-191: LGTM! Hook invocations correctly standardized.The refactor from keyword-argument style (
data=case) to positional-argument passing aligns with the function signature and the project-wide standardization described in the PR objectives. All four invocations correctly pass the payload object as the second positional argument.Also applies to: 225-225, 229-229, 236-236
source/app/business/reports/reports.py (3)
31-31: Preload hook standardization looks good.The change from keyword to positional argument for
report_idaligns with the repository-wide hook invocation standardization.
50-57: Postload hook standardization looks good.The variable rename from
_datatodataand passing it as a positional argument aligns with the standardized calling convention.
63-63: Preload hook standardization looks good.The change from keyword to positional argument for
report_idaligns with the repository-wide hook invocation standardization.source/app/datamgmt/client/client_db.py (1)
25-25: LGTM: Centralized deletion pattern applied correctly.The refactor from direct ORM deletions to the
db_deleteutility maintains identical behavior while improving consistency across the codebase.Also applies to: 144-144, 151-151
source/app/datamgmt/db_operations.py (1)
27-29: LGTM: Clean deletion utility.The
db_deletefunction provides a consistent interface for deletions, mirroring the existingdb_createpattern.source/app/datamgmt/iris_engine/modules_db.py (1)
23-23: LGTM: Centralized logging.Switching to the centralized
loggerimport improves consistency.Also applies to: 245-245
source/app/datamgmt/case/case_rfiles_db.py (1)
23-23: LGTM: Consistent deletion pattern.The refactor correctly applies the centralized
db_deleteutility.Also applies to: 182-182
source/app/datamgmt/manage/manage_groups_db.py (1)
19-19: LGTM: Centralized deletion applied.The refactor correctly uses the
db_deleteutility for group deletion.Also applies to: 209-209
source/app/iris_engine/module_handler/module_handler.py (2)
413-417: LGTM: Helpful documentation.The extended docstring clarifies the purpose of the
caseidparameter in task retrieval.
35-35: Verify transaction commit in deregister_module_from_hook.The refactor delegates hook deregistration to
deregister_module_from_hook, but that function (insource/app/datamgmt/iris_engine/modules_db.pylines 262-269) doesn't commit the deletions. Ensure the missing commit is added.Also applies to: 402-403
source/app/business/iocs.py (1)
59-59: LGTM: Hook invocation standardized.The refactor from keyword (
data=...) to positional arguments forcall_modules_hookaligns with the PR-wide standardization effort.Also applies to: 84-84, 97-97, 101-101
source/app/datamgmt/case/case_tasks_db.py (2)
25-25: LGTM: Centralized deletion pattern.The refactor correctly applies the
db_deleteutility for comment deletion.Also applies to: 316-316
347-373: LGTM: Well-structured query.The function correctly joins multiple tables and filters out completed/canceled tasks.
source/app/business/assets.py (1)
50-50: LGTM! Hook invocation standardization applied correctly.The refactoring consistently updates all three hook calls to use positional arguments instead of keyword
data=arguments, aligning with the broader codebase standardization effort.Also applies to: 62-62, 65-65
source/app/business/tasks.py (2)
26-26: LGTM! Import reorganization improves modularity.Moving
list_user_tasksfromdashboard_dbtocase_tasks_dbproperly consolidates task-related data access functions into their domain-specific module.
41-41: LGTM! Hook invocations standardized correctly.All four hook calls consistently updated to use positional arguments for the data payload, maintaining the
caseidkeyword argument where applicable.Also applies to: 45-45, 57-57, 89-89
source/app/business/evidences.py (1)
38-38: LGTM! Hook invocation refactoring applied consistently.All four evidence lifecycle hooks updated to use positional arguments for the payload, with
caseidproperly passed as a keyword argument where needed.Also applies to: 55-55, 70-70, 72-72
source/app/datamgmt/case/case_notes_db.py (1)
25-25: LGTM! Deletion operations properly centralized.Refactoring to use the
db_deletehelper function standardizes database deletion operations across the codebase, improving maintainability and consistency.Also applies to: 70-70, 431-431
source/app/business/notes.py (1)
65-65: LGTM! Hook calls standardized correctly.All four note lifecycle hooks updated to pass payloads as positional arguments, maintaining consistency with the broader refactoring effort.
Also applies to: 92-92, 107-107, 109-109
source/app/datamgmt/case/case_db.py (1)
252-266: LGTM! User reviews listing function implemented correctly.The function properly filters cases by reviewer and excludes terminal review statuses ('Reviewed', 'Not reviewed'), which aligns with the typical workflow for pending reviews.
source/app/datamgmt/manage/manage_case_templates_db.py (1)
192-192: LGTM! Case template hook invocations standardized.All four hook calls (two for tasks, two for notes) properly updated to use positional arguments for payloads during case template population, maintaining consistency with the codebase refactoring.
Also applies to: 203-203, 224-226, 238-238
.vulture.ignore (1)
1-5: LGTM! External API properly documented.Correctly documents methods used by the
iris-module-interfacepackage, preventing false-positive unused code warnings while maintaining clear documentation of the external dependency.tests/tests_rest_module_tasks.py (1)
31-41: LGTM! Test structure is clear and validates case identifier presence.The test correctly verifies that the case identifier appears in the task list response, which aligns with the PR objective to prevent removal of this field.
source/app/blueprints/rest/alerts_routes.py (1)
372-378: LGTM! Hook invocations correctly refactored to use positional arguments.The changes consistently update
call_modules_hookinvocations from keyword argumentdata=to positional argument, aligning with the function signature and the broader codebase refactoring.source/app/blueprints/rest/case/case_timeline_routes.py (1)
103-103: LGTM! Consistent hook invocation refactoring.All hook calls in this file now use positional arguments for the data parameter, consistent with the standardized calling convention.
source/app/business/comments.py (2)
118-118: LGTM! Hook invocations standardized to positional arguments.All
call_modules_hookinvocations now use positional arguments for the data parameter, consistent with the codebase-wide refactoring.
306-306: LGTM! Updated to match new delete_asset_comment signature.The call now correctly passes the
commentobject instead ofcomment_id, aligning with the updateddelete_asset_commentsignature that expects a Comments object (as seen in case_assets_db.py).pyproject.toml (1)
42-54: LGTM! Import linter contracts refined to match architectural refactoring.The updated contracts appropriately reflect the reorganized module boundaries:
- Contract 1 broadens coverage to all of
app.datamgmtwhile narrowing the forbidden scope toaccess_controls- Contract 2 specifically targets
case_assets_dbto preventiris_userimportsThese changes ensure the persistence layer maintains proper separation from the API layer.
source/app/blueprints/rest/dim_tasks_routes.py (1)
142-142: LGTM! Hook invocation standardized to positional arguments.The change correctly uses positional arguments for
hook_nameandobj_targets, consistent with the standardized calling convention across the codebase.source/app/datamgmt/case/case_events_db.py (2)
21-21: LGTM! Imported db_delete for centralized deletion.Adding
db_deleteto the imports enables the centralized deletion pattern used at line 174.
174-174: LGTM! Deletion centralized through db_delete utility.Replacing direct
db.session.delete()anddb.session.commit()calls withdb_delete(comment)aligns with the codebase-wide refactoring to centralize deletion operations.source/app/datamgmt/case/case_iocs_db.py (2)
21-21: LGTM! Imported db_delete for centralized deletion.Adding
db_deleteto the imports enables the centralized deletion pattern used at line 278.
278-278: LGTM! Deletion centralized through db_delete utility.Replacing direct
db.session.delete()anddb.session.commit()calls withdb_delete(comment)aligns with the codebase-wide refactoring to centralize deletion operations.source/app/datamgmt/comments.py (4)
18-18: LGTM!The Optional type hint is appropriate for the new
get_commentfunction that returnsNonewhen no comment is found.
23-23: LGTM!Importing the centralized
db_deletehelper promotes consistency across the codebase.
110-110: LGTM!Using the centralized
db_deleteutility is a good refactor that ensures consistent deletion behavior across the codebase.
141-145: LGTM!The
get_commentfunction properly scopes comment retrieval to the requesting user, which is important for authorization. The filtering by bothcomment_idandcomment_user_idensures users can only retrieve their own comments.source/app/blueprints/rest/case/case_evidences_routes.py (1)
192-192: LGTM!The refactor to pass hook data as a positional argument instead of using the
data=keyword is consistent with thecall_modules_hooksignature and aligns with similar changes across the codebase.Also applies to: 228-228
source/app/business/global_tasks.py (3)
25-25: LGTM!Importing the necessary functions from the new
global_tasksmodule enables the delete functionality.
36-36: LGTM!Refactoring to pass the global_task as a positional argument is consistent with the standardized calling convention for
call_modules_hook.
49-55: LGTM!The
global_tasks_deletefunction follows the established pattern:
- Pre-delete hook for extension points
- Actual deletion via the persistence layer
- Post-delete hook for notifications
- Activity tracking
This is consistent with similar delete operations elsewhere in the codebase.
source/app/blueprints/rest/case/case_tasks_routes.py (1)
233-233: LGTM!Refactoring hook invocations to use positional arguments aligns with the standardized calling convention across the codebase.
Also applies to: 273-273
tests/tests_rest_global_tasks.py (1)
51-64: LGTM!The new tests provide good coverage for the delete functionality:
test_delete_global_task_should_return_204verifies successful deletion returns the correct HTTP statustest_get_global_task_should_return_404_when_deletedensures deleted tasks are no longer retrievableThese tests follow the existing patterns and validate the expected REST behavior.
source/app/blueprints/rest/case/case_ioc_routes.py (1)
182-182: LGTM!All
call_modules_hookinvocations have been consistently updated to use positional arguments, aligning with the standardized calling convention across the codebase.Also applies to: 194-194, 307-307, 345-345
source/app/blueprints/rest/case/case_notes_routes.py (1)
141-141: LGTM!All hook invocations have been refactored to pass data as positional arguments, maintaining consistency with the standardized calling convention.
Also applies to: 210-210, 429-429, 465-465
source/app/datamgmt/alerts/alerts_db.py (2)
75-75: LGTM!Adding
db_deleteto the imports enables consistent use of the centralized deletion utility.
1285-1285: LGTM!Both deletion call sites now use the centralized
db_deleteutility, promoting consistency across the codebase.Also applies to: 1424-1424
source/app/datamgmt/datastore/datastore_db.py (1)
252-253: db_delete in tight loops changes transactional semantics and may hurt performance.Using
db_deletehere centralizes deletion, but becausedb_deletecommits on each call, the behavior differs from the typical pattern of deleting many rows and committing once:
- In
datastore_iter_deletionanddatastore_delete_files_of_path, you now potentially commit once per path/file.- In
datastore_delete_file, you commit immediately for the single file (which is fine, but different from batching with other work).Implications:
- More round‑trips and contention if many files/paths are deleted in one request.
- Loss of “all‑or‑nothing” behavior when multiple deletions are logically part of one operation (you can end up with some files deleted and others not if a later delete fails).
If the intent is to keep atomic multi‑step deletions, consider:
- Introducing a non‑committing helper (e.g.,
db_delete_nocommit) and doing a singledb.session.commit()at the end of the outer operation, or- Passing a flag to
db_deleteto control committing, and usingcommit=Falsein loops.Example refactor sketch for the loop (conceptual):
- db_delete(dsf_list_item) + db.session.delete(dsf_list_item) ... - return + db.session.commit() + return[uggest_recommended_refactor]
Also applies to: 264-271, 328-343
source/app/blueprints/rest/case/case_assets_routes.py (1)
47-48: Hook invocation refactor to positionaldatais consistent.Across asset creation, bulk upload, update, commenting, and comment delete hooks you’ve switched from
data=...to passing the payload as the second positional argument:
on_preload_asset_create(single and CSV upload)on_postload_asset_createon_preload_asset_updateon_postload_asset_commentedon_postload_asset_comment_deleteThis matches
call_modules_hook(hook_name, data, caseid=...)and keeps the call style consistent with the rest of the codebase. No functional issues spotted here.Also applies to: 170-171, 247-248, 257-258, 309-310, 379-380, 417-418
source/app/blueprints/rest/v2/global_tasks.py (5)
31-35: Imports for delete behavior are appropriate and usedThe added imports
response_api_deletedandglobal_tasks_deleteare correctly referenced in the new delete flow and keep REST and business responsibilities separated.
55-57: read() naming cleanup is correct and behavior-preservingUsing
global_taskfor both retrieval and serialization improves clarity and remains consistent withglobal_tasks_getand the schema dump. No functional issues.
61-68: DELETE operation wiring and error handling look soundThe new
delete()method correctly reusesglobal_tasks_getfor existence checking, delegates deletion toglobal_tasks_delete, returnsresponse_api_deleted()on success, and mapsObjectNotFoundErrortoresponse_api_not_found(). This matches the established pattern and HTTP semantics.
76-85: Route renames to global_task terminology are consistentThe
create_global_taskandget_global_taskendpoints are cleanly mapped to the corresponding operations and keepac_api_requiresin place; naming now aligns with the resource (global_tasks).
88-91: New DELETE /global-tasks/int:identifier endpoint is correctly hooked upThe DELETE route is registered on the same blueprint, uses an
intidentifier consistent withglobal_tasks_get, and delegates toGlobalTasksOperations.deleteunder the same access control decorator. This completes the REST surface for global task deletion.source/app/business/cases.py (4)
31-35: Refactored imports to case_db and models are coherentImporting
case_db_exists,list_user_cases,case_db_save,list_user_reviews,Cases, andReviewStatusListfrom their domain-specific modules matches how they’re used later (existence checks, listing, saving, and review status handling). This improves cohesion of persistence-related logic.Also applies to: 58-59
136-136: on_postload_case_create hook call simplified without behavior change
call_modules_hook('on_postload_case_create', case)drops an unnecessary extra positional argument while still passing the createdcaseas the hook data, consistent with the hook function signature. No functional impact.
152-157: Case delete hooks now use positional data and explicit caseidPassing
case_identifieras the positional data argument andcaseid=case_identifierfor bothon_preload_case_deleteandon_postload_case_deletekeeps hook data explicit and aligned with other callers, while preserving prior behavior (data and caseid were already the same value).
186-191: Alert post-update hooks now use positional data consistentlyBoth
on_postload_alert_updateandon_postload_alert_resolution_updatenow pass thealertobject positionally withcaseid=case.case_id, matching the call_modules_hook contract and broader positional-argument refactor without altering semantics.
Implementation of
DELETE /api/v2/global-tasks/{identifier}POST /global/tasks/delete/<int:cur_id>.vulture.ignoreapp.datamgmt.dashboard.dashboard_dbinto other files of the persistence layer to group requests by tablecall_modules_hookGET /dim/tasks/list/{count}to ensure the case identifier is present in the response (and prevent a code cleanup I had wanted to perform)This PR goes with the documentation PR#98 in
iris-doc-src.Summary by CodeRabbit
Release Notes
New Features
Improvements
Tests