feat: pg_timetable UI basic functionality - #10152
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds pgAdmin browser support for pgTimeTable chains and tasks, including backend APIs, SQL templates, UI schemas, browser registration, styling, webpack wiring, preferences, documentation, and scenario-based regression tests. ChangespgTimeTable integration
Estimated code review effort: 5 (Critical) | ~120 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (2)
web/pgadmin/browser/server_groups/servers/pg_timetable/tasks/__init__.py-434-453 (1)
434-453: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winCopy form data before replacing
parameters.When
request.formis selected, Line 453 attempts to mutate anImmutableMultiDict, raisingTypeErrorfor form-based parameter updates.Proposed fix
- data = request.form if request.form else json.loads( + data = request.form.copy() if request.form else json.loads( request.data.decode('utf-8') )Based on learnings,
request.formonly needs copying when the handler subsequently mutates the mapping, as it does here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/pgadmin/browser/server_groups/servers/pg_timetable/tasks/__init__.py` around lines 434 - 453, Copy the selected request.form mapping before the parameters-processing block mutates data['parameters']. Update the data initialization around the request.form/json.loads branch so form input becomes a mutable mapping, while preserving the existing JSON parsing behavior and parameter normalization in the surrounding handler.Source: Learnings
web/pgadmin/browser/server_groups/servers/pg_timetable/static/js/pgt_chain.js-122-122 (1)
122-122: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCorrect the action data for the "Run now" context menu.
The
data: { action: 'create' }payload is meant for create-object menus. It should be changed to'run_now'(or removed) to avoid inadvertently triggering anycreateprivilege checks or validation semantics for an execution action.🐛 Proposed fix
- data: { action: 'create' }, + data: { action: 'run_now' },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/pgadmin/browser/server_groups/servers/pg_timetable/static/js/pgt_chain.js` at line 122, Update the Run now context menu configuration in pgt_chain.js so its data payload uses the run_now action instead of create, preventing execution from being treated as object creation.
🧹 Nitpick comments (6)
web/pgadmin/browser/server_groups/servers/pg_timetable/static/js/pgt_chain.ui.js (1)
76-84: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSeparate ASCII art from the
gettextstring.Wrapping a large block of ASCII art and HTML tags inside a single
gettextcall creates a brittle translation key. Translators might accidentally break the formatting, HTML tags, or the ASCII structure. Consider splitting the string so that only the descriptive phrases are passed togettext.♻️ Proposed refactor
- helpMessage: gettext(`<pre>----CRON-Style --- * * * * * command to execute --- ┬ ┬ ┬ ┬ ┬ --- │ │ │ │ │ --- │ │ │ │ └──── day of the week (0 - 7) (Sunday to Saturday)(0 and 7 is Sunday); --- │ │ │ └────── month (1 - 12) --- │ │ └──────── day of the month (1 - 31) --- │ └────────── hour (0 - 23) --- └──────────── minute (0 - 59)</pre>`), + helpMessage: `<pre>----CRON-Style +-- * * * * * command to execute +-- ┬ ┬ ┬ ┬ ┬ +-- │ │ │ │ │ +-- │ │ │ │ └──── ` + gettext('day of the week (0 - 7) (Sunday to Saturday)(0 and 7 is Sunday);') + ` +-- │ │ │ └────── ` + gettext('month (1 - 12)') + ` +-- │ │ └──────── ` + gettext('day of the month (1 - 31)') + ` +-- │ └────────── ` + gettext('hour (0 - 23)') + ` +-- └──────────── ` + gettext('minute (0 - 59)') + `</pre>`,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/pgadmin/browser/server_groups/servers/pg_timetable/static/js/pgt_chain.ui.js` around lines 76 - 84, Update the helpMessage definition in the chain UI configuration to keep the CRON ASCII layout and HTML preformatted structure outside gettext, while wrapping only the descriptive text phrases such as field names and ranges with gettext. Preserve the rendered formatting and all existing CRON guidance.web/pgadmin/browser/server_groups/servers/pg_timetable/static/js/pgt_chain.js (1)
68-73: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid unloading the tree node on execution.
Calling
t.unload(i)will abruptly collapse the node and clear its children from the tree, forcing the user to manually re-expand it to continue viewing tasks. Consider removingt.unload(i)entirely, as a "Run now" action generally does not require a structural tree reset.♻️ Proposed refactor
.then(({ data: res }) => { pgAdmin.Browser.notifier.success(res.info); - t.unload(i); }) .catch(function (error) { pgAdmin.Browser.notifier.pgRespErrorNotify(error); - t.unload(i); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/pgadmin/browser/server_groups/servers/pg_timetable/static/js/pgt_chain.js` around lines 68 - 73, Remove both t.unload(i) calls from the success and catch handlers of the execution flow, while preserving the existing success notification and error handling so running a task does not collapse or clear its tree node.web/pgadmin/browser/server_groups/servers/pg_timetable/tasks/static/js/pgt_chaintask.ui.js (1)
17-85: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDeduplicate
ParamSchemaand preserve validation logic.There are two identical inline definitions for the parameter schema. The first definition (lines 17-40) includes a
validatemethod, but the fallback definition inPgtChainTaskSchema(lines 74-85) omits the validation completely.Extracting a shared class eliminates the duplication and ensures validation is applied consistently.
♻️ Proposed refactor to extract the class
-export function getNodePgtChainTaskSchema(treeNodeInfo, itemNodeData) { - const paramSchema = new (class extends BaseUISchema { - constructor() { - super({ order_id: null, value: '' }); - } - get idAttribute() { return 'order_id'; } - get baseFields() { - return [ - { id: 'order_id', label: gettext('Order'), type: 'int', noEmpty: true, cell: 'int', width: 20 }, - { id: 'value', label: gettext('Value'), type: 'multiline', cell: 'text' }, - ]; - } - validate(state, setError) { - if (!state.order_id || state.order_id < 1) { - setError('order_id', gettext('Order must be a positive integer.')); - return true; - } - setError('order_id', null); - if (isEmptyString(state.value)) { - setError('value', gettext('Please enter a parameter value.')); - return true; - } - setError('value', null); - } - })(); +class ParamSchema extends BaseUISchema { + constructor() { + super({ order_id: null, value: '' }); + } + get idAttribute() { return 'order_id'; } + get baseFields() { + return [ + { id: 'order_id', label: gettext('Order'), type: 'int', noEmpty: true, cell: 'int', width: 20 }, + { id: 'value', label: gettext('Value'), type: 'multiline', cell: 'text' }, + ]; + } + validate(state, setError) { + if (!state.order_id || state.order_id < 1) { + setError('order_id', gettext('Order must be a positive integer.')); + return true; + } + setError('order_id', null); + if (isEmptyString(state.value)) { + setError('value', gettext('Please enter a parameter value.')); + return true; + } + setError('value', null); + } +} + +export function getNodePgtChainTaskSchema(treeNodeInfo, itemNodeData) { + const paramSchema = new ParamSchema(); return new PgtChainTaskSchema( { databases: () => getNodeListByName('database', treeNodeInfo, itemNodeData, { cacheLevel: 'database', cacheNode: 'database', }), paramSchema, }, { jstdbname: treeNodeInfo['server']['db'], } ); } export default class PgtChainTaskSchema extends BaseUISchema { constructor(fieldOptions = {}, initValues = {}) { super({ task_id: null, chain_id: null, task_name: '', task_order: 10, kind: 'SQL', command: '', database_connection: null, ignore_error: false, parameters: [], ...initValues, }); this.fieldOptions = { databases: [], - paramSchema: new (class extends BaseUISchema { - constructor() { - super({ order_id: null, value: '' }); - } - get idAttribute() { return 'order_id'; } - get baseFields() { - return [ - { id: 'order_id', label: gettext('Order'), type: 'int', noEmpty: true, cell: 'int', width: 20 }, - { id: 'value', label: gettext('Value'), type: 'multiline', cell: 'text' }, - ]; - } - })(), + paramSchema: new ParamSchema(), ...fieldOptions, }; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/pgadmin/browser/server_groups/servers/pg_timetable/tasks/static/js/pgt_chaintask.ui.js` around lines 17 - 85, Extract the duplicated parameter schema into a shared ParamSchema class or factory containing the constructor, idAttribute, baseFields, and validate logic from the first definition. Replace both inline definitions—within the schema-building function and PgtChainTaskSchema’s fieldOptions fallback—with that shared symbol so validation is consistently applied.web/pgadmin/browser/server_groups/servers/pg_timetable/tests/__init__.py (1)
13-15: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove unused stub test class.
This class appears to be an empty placeholder or dead code leftover from test development. Test modules typically leave
__init__.pyempty or use it exclusively for shared configurations and fixtures. Consider removing this stub to keep the test suite clean.♻️ Proposed fix
-class PgTimetableCreateTestCase(BaseTestGenerator): - def runTest(self): - return🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/pgadmin/browser/server_groups/servers/pg_timetable/tests/__init__.py` around lines 13 - 15, Remove the unused PgTimetableCreateTestCase class and its runTest stub from the test package initializer, leaving __init__.py empty unless it contains required shared setup.web/pgadmin/browser/server_groups/servers/pg_timetable/tests/test_pgt_chain_delete.py (2)
21-22: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCall
super().setUp()for test consistency and safety.Unlike other tests in this module, these files override
setUpwithout callingsuper().setUp(). This bypasses initialization logic inherited fromBaseTestGeneratorand introduces inconsistency.
web/pgadmin/browser/server_groups/servers/pg_timetable/tests/test_pgt_chain_delete.py#L21-L22: addsuper().setUp()at the beginning of thesetUpmethod.web/pgadmin/browser/server_groups/servers/pg_timetable/tests/test_pgt_chain_get_msql.py#L23-L24: addsuper().setUp()at the beginning of thesetUpmethod.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/pgadmin/browser/server_groups/servers/pg_timetable/tests/test_pgt_chain_delete.py` around lines 21 - 22, Both test setUp methods omit inherited initialization. Add super().setUp() as the first statement in setUp for web/pgadmin/browser/server_groups/servers/pg_timetable/tests/test_pgt_chain_delete.py (lines 21-22) and web/pgadmin/browser/server_groups/servers/pg_timetable/tests/test_pgt_chain_get_msql.py (lines 23-24), before calling pgt_utils.is_valid_server_to_run_pgtimetable.
40-52: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd
elsebranches to handle potential negative test cases.These test implementations lack an
elseblock to handle scenarios whereself.is_positive_testevaluates to false. While the current test data only contains positive scenarios, this omission makes the tests brittle: future negative test cases will either silently pass without invoking the API or unconditionally fail during assertions.
web/pgadmin/browser/server_groups/servers/pg_timetable/tests/test_pgt_chain_delete.py#L40-L52: add anelseblock to execute the expected failing API call and conditionally bypass the chain deletion assertion for negative tests.web/pgadmin/browser/server_groups/servers/pg_timetable/tests/test_pgt_chain_get_msql.py#L39-L45: add anelseblock to execute the API call and verify the anticipated error status.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/pgadmin/browser/server_groups/servers/pg_timetable/tests/test_pgt_chain_delete.py` around lines 40 - 52, Add negative-test handling in test_pgt_chain_delete.py lines 40-52 by invoking the expected failing delete API call when is_positive_test is false and skipping the deletion-presence assertion for that path. In test_pgt_chain_get_msql.py lines 39-45, add an else branch that performs the API request and verifies the expected error status.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@web/pgadmin/browser/server_groups/servers/pg_timetable/__init__.py`:
- Line 373: Update the chain update flow surrounding _process_ctasks so every
failed delete, update, insert, or parameter write is immediately propagated to
the caller instead of producing a successful response. Make _process_ctasks
return or raise on the first unsuccessful database status, and execute the
chain-field update together with all nested task processing within a single
transaction so no partial changes are committed.
- Around line 460-486: The _upsert_task_params method incorrectly replaces all
stored parameters when given a partial dictionary delta. Update its
dictionary-payload handling to apply added, changed, and deleted entries
independently, preserving unchanged parameters and removing only explicitly
deleted ones; ensure deleted-only payloads are processed instead of returning
early, while retaining full-list replacement behavior for complete parameter
lists.
- Around line 324-334: Update the transaction handling in the post-create flow
around execute_dict and the subsequent status check so a failed newly created
database lookup triggers a rollback before returning internal_server_error. Only
commit with END after status succeeds, preserving the existing successful lookup
behavior and preventing failed retries from leaving the chain persisted.
In
`@web/pgadmin/browser/server_groups/servers/pg_timetable/static/js/pgt_chain.ui.js`:
- Around line 109-116: Update the ADD_ROW branch in depChange to locate the
newly inserted task by matching actionObj.value.cid, rather than assuming it is
the last element of state.ctasks. Compute lastOrder using numerically parsed
task_order values, then assign the new task’s order to lastOrder + 10 without
overwriting another row.
In
`@web/pgadmin/browser/server_groups/servers/pg_timetable/tasks/sql/default/create.sql`:
- Around line 23-26: Update the chain lookup in the default task SQL to filter
timetable.chain by chain_name instead of name, while preserving the existing
data.name value, ordering, and limit behavior.
- Around line 5-13: Update the job_kind argument in the timetable.add_job call
to cast the 'SQL' value to timetable.command_kind instead of
timetable.task_kind; leave the task_kind usage for subsequent task inserts
unchanged.
- Around line 31-41: Update the INSERT statement in the task creation template
to use the timetable.task column kind instead of task_kind, and cast the task
kind value to timetable.command_kind. Leave the remaining columns and values
unchanged.
In
`@web/pgadmin/browser/server_groups/servers/pg_timetable/tasks/sql/default/update.sql`:
- Around line 5-44: Update the SQL template’s chain and task operations to match
the established schema: use chain_name instead of name, pass conn to every
qtLiteral filter, insert the required task_name column, and use kind with
timetable.command_kind instead of task_kind/timetable.task_kind. Before deleting
existing timetable.task rows, delete their associated timetable.parameter rows
for the same chain_id, preserving the existing task replacement flow.
In
`@web/pgadmin/browser/server_groups/servers/pg_timetable/tasks/static/js/pgt_chaintask.ui.js`:
- Around line 241-247: Move the empty-command validation in the task validation
function outside the state.kind === 'SQL' conditional so it runs for SQL,
PROGRAM, and BUILTIN tasks. Preserve the existing kind-specific error message
selection, set the command error for empty values, clear it for non-empty
values, and return validation failure when appropriate.
In
`@web/pgadmin/browser/server_groups/servers/pg_timetable/tasks/tests/test_pgt_task_delete.py`:
- Around line 47-59: Update the test method’s is_positive_test branch so task
verification and the assertFalse(is_present) check run only after a successful
positive deletion. Add the standard negative-test else branch used by other API
tests, including the appropriate mock-based validation, while preserving the
existing list and single-task deletion calls.
In
`@web/pgadmin/browser/server_groups/servers/pg_timetable/templates/macros/pgt_chaintask.macros`:
- Around line 46-50: Update the SELECT list in the pgt_chaintask macro to use
the has_connstr argument: select t.database_connection when the column is
available, otherwise return a typed NULL. Preserve the existing output alias and
surrounding task fields so older pg_timetable versions do not reference the
missing column.
- Around line 2-15: Update the INSERT column and value lists in the
pgt_chaintask macro to include database_connection only when has_connstr is
true, keeping the lists aligned for older pg_timetable versions. When
has_connstr is false, omit both the column and value entirely rather than
emitting NULL without a comma, while preserving the existing ignore_error
handling.
- Around line 18-29: Update the task-field detection and assignment in the macro
so database_connection is included only when has_connstr is true. Ensure
has_task_fields does not become true for database_connection alone when the
connection column is unavailable, and omit that column from the UPDATE SET
clause in that case.
In
`@web/pgadmin/browser/server_groups/servers/pg_timetable/templates/pgt_chain/sql/default/delete.sql`:
- Around line 4-5: Update the chain deletion SQL to remove associated rows from
timetable.parameter before deleting matching timetable.task rows, following the
deletion order used by the DELETE macro in pgt_chaintask.macros and retaining
the existing chain_id filter.
In
`@web/pgadmin/browser/server_groups/servers/pg_timetable/templates/pgt_chaintask/sql/default/create.sql`:
- Around line 2-4: Remove the trailing semicolon emitted by the TASK.INSERT
macro in pgt_chaintask.macros so its expansion remains valid inside the WITH tid
AS (...) CTE in the create.sql template.
In
`@web/pgadmin/browser/server_groups/servers/pg_timetable/templates/pgt_chaintask/sql/default/nodes.sql`:
- Around line 2-3: Update the kind projection in nodes.sql to return the actual
kind value cast to text, matching the existing PROPERTIES macro, instead of
converting SQL versus non-SQL values into a boolean. Keep the task_id, chain_id,
task_name, and task_order projections unchanged.
In
`@web/pgadmin/browser/server_groups/servers/pg_timetable/tests/test_pgt_chain_get_nodes.py`:
- Around line 23-29: Add super().setUp() as the first operation in setUp for
both test cases:
web/pgadmin/browser/server_groups/servers/pg_timetable/tests/test_pgt_chain_get_nodes.py
lines 23-29 and
web/pgadmin/browser/server_groups/servers/pg_timetable/tests/test_pgt_chain_sql.py
lines 23-29. Preserve the existing server validation and skip behavior after
BaseTestGenerator.setUp() initializes the shared test state.
In `@web/pgadmin/browser/server_groups/servers/pg_timetable/tests/utils.py`:
- Around line 102-153: Update is_pgtimetable_installed_on_server so its
exception path returns the same (flag, message) tuple contract as the explicit
failure paths, after printing the traceback. Return False with an appropriate
installation-check message so callers can unpack the result and skip gracefully
when a database error occurs.
- Around line 156-320: Ensure every connection acquired in
create_pgtimetable_chain, delete_pgtimetable_chain, verify_pgtimetable_chain,
create_pgtimetable_task, delete_pgtimetable_task, and verify_pgtimetable_task is
closed on both success and exception paths. Track the connection after
get_db_connection and move cleanup into a finally block, guarding for
acquisition failures while preserving each function’s existing return and
error-reporting behavior.
---
Minor comments:
In
`@web/pgadmin/browser/server_groups/servers/pg_timetable/static/js/pgt_chain.js`:
- Line 122: Update the Run now context menu configuration in pgt_chain.js so its
data payload uses the run_now action instead of create, preventing execution
from being treated as object creation.
In `@web/pgadmin/browser/server_groups/servers/pg_timetable/tasks/__init__.py`:
- Around line 434-453: Copy the selected request.form mapping before the
parameters-processing block mutates data['parameters']. Update the data
initialization around the request.form/json.loads branch so form input becomes a
mutable mapping, while preserving the existing JSON parsing behavior and
parameter normalization in the surrounding handler.
---
Nitpick comments:
In
`@web/pgadmin/browser/server_groups/servers/pg_timetable/static/js/pgt_chain.js`:
- Around line 68-73: Remove both t.unload(i) calls from the success and catch
handlers of the execution flow, while preserving the existing success
notification and error handling so running a task does not collapse or clear its
tree node.
In
`@web/pgadmin/browser/server_groups/servers/pg_timetable/static/js/pgt_chain.ui.js`:
- Around line 76-84: Update the helpMessage definition in the chain UI
configuration to keep the CRON ASCII layout and HTML preformatted structure
outside gettext, while wrapping only the descriptive text phrases such as field
names and ranges with gettext. Preserve the rendered formatting and all existing
CRON guidance.
In
`@web/pgadmin/browser/server_groups/servers/pg_timetable/tasks/static/js/pgt_chaintask.ui.js`:
- Around line 17-85: Extract the duplicated parameter schema into a shared
ParamSchema class or factory containing the constructor, idAttribute,
baseFields, and validate logic from the first definition. Replace both inline
definitions—within the schema-building function and PgtChainTaskSchema’s
fieldOptions fallback—with that shared symbol so validation is consistently
applied.
In `@web/pgadmin/browser/server_groups/servers/pg_timetable/tests/__init__.py`:
- Around line 13-15: Remove the unused PgTimetableCreateTestCase class and its
runTest stub from the test package initializer, leaving __init__.py empty unless
it contains required shared setup.
In
`@web/pgadmin/browser/server_groups/servers/pg_timetable/tests/test_pgt_chain_delete.py`:
- Around line 21-22: Both test setUp methods omit inherited initialization. Add
super().setUp() as the first statement in setUp for
web/pgadmin/browser/server_groups/servers/pg_timetable/tests/test_pgt_chain_delete.py
(lines 21-22) and
web/pgadmin/browser/server_groups/servers/pg_timetable/tests/test_pgt_chain_get_msql.py
(lines 23-24), before calling pgt_utils.is_valid_server_to_run_pgtimetable.
- Around line 40-52: Add negative-test handling in test_pgt_chain_delete.py
lines 40-52 by invoking the expected failing delete API call when
is_positive_test is false and skipping the deletion-presence assertion for that
path. In test_pgt_chain_get_msql.py lines 39-45, add an else branch that
performs the API request and verifies the expected error status.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 03920183-3ea0-4e00-914e-ce36423e0ffc
⛔ Files ignored due to path filters (6)
web/pgadmin/browser/server_groups/servers/pg_timetable/static/img/coll-pgt_chain.svgis excluded by!**/*.svgweb/pgadmin/browser/server_groups/servers/pg_timetable/static/img/pgt_chain-disabled.svgis excluded by!**/*.svgweb/pgadmin/browser/server_groups/servers/pg_timetable/static/img/pgt_chain.svgis excluded by!**/*.svgweb/pgadmin/browser/server_groups/servers/pg_timetable/tasks/static/img/coll-pgt_chaintask.svgis excluded by!**/*.svgweb/pgadmin/browser/server_groups/servers/pg_timetable/tasks/static/img/pgt_chaintask-disabled.svgis excluded by!**/*.svgweb/pgadmin/browser/server_groups/servers/pg_timetable/tasks/static/img/pgt_chaintask.svgis excluded by!**/*.svg
📒 Files selected for processing (53)
web/pgadmin/browser/server_groups/servers/__init__.pyweb/pgadmin/browser/server_groups/servers/pg_timetable/__init__.pyweb/pgadmin/browser/server_groups/servers/pg_timetable/static/css/pgt_chain.cssweb/pgadmin/browser/server_groups/servers/pg_timetable/static/js/pgt_chain.jsweb/pgadmin/browser/server_groups/servers/pg_timetable/static/js/pgt_chain.ui.jsweb/pgadmin/browser/server_groups/servers/pg_timetable/tasks/__init__.pyweb/pgadmin/browser/server_groups/servers/pg_timetable/tasks/sql/default/check_extension.sqlweb/pgadmin/browser/server_groups/servers/pg_timetable/tasks/sql/default/create.sqlweb/pgadmin/browser/server_groups/servers/pg_timetable/tasks/sql/default/update.sqlweb/pgadmin/browser/server_groups/servers/pg_timetable/tasks/static/js/pgt_chaintask.jsweb/pgadmin/browser/server_groups/servers/pg_timetable/tasks/static/js/pgt_chaintask.ui.jsweb/pgadmin/browser/server_groups/servers/pg_timetable/tasks/template/pgt_chaintask/css/pgt_chaintask.cssweb/pgadmin/browser/server_groups/servers/pg_timetable/tasks/tests/__init__.pyweb/pgadmin/browser/server_groups/servers/pg_timetable/tasks/tests/tasks_test_data.jsonweb/pgadmin/browser/server_groups/servers/pg_timetable/tasks/tests/test_pgt_task_add.pyweb/pgadmin/browser/server_groups/servers/pg_timetable/tasks/tests/test_pgt_task_delete.pyweb/pgadmin/browser/server_groups/servers/pg_timetable/tasks/tests/test_pgt_task_get.pyweb/pgadmin/browser/server_groups/servers/pg_timetable/tasks/tests/test_pgt_task_get_msql.pyweb/pgadmin/browser/server_groups/servers/pg_timetable/tasks/tests/test_pgt_task_get_nodes.pyweb/pgadmin/browser/server_groups/servers/pg_timetable/tasks/tests/test_pgt_task_get_statistics.pyweb/pgadmin/browser/server_groups/servers/pg_timetable/tasks/tests/test_pgt_task_put.pyweb/pgadmin/browser/server_groups/servers/pg_timetable/tasks/tests/utils.pyweb/pgadmin/browser/server_groups/servers/pg_timetable/templates/macros/pgt_chaintask.macrosweb/pgadmin/browser/server_groups/servers/pg_timetable/templates/pgt_chain/css/pgt_chain.cssweb/pgadmin/browser/server_groups/servers/pg_timetable/templates/pgt_chain/sql/default/create.sqlweb/pgadmin/browser/server_groups/servers/pg_timetable/templates/pgt_chain/sql/default/delete.sqlweb/pgadmin/browser/server_groups/servers/pg_timetable/templates/pgt_chain/sql/default/nodes.sqlweb/pgadmin/browser/server_groups/servers/pg_timetable/templates/pgt_chain/sql/default/properties.sqlweb/pgadmin/browser/server_groups/servers/pg_timetable/templates/pgt_chain/sql/default/run_now.sqlweb/pgadmin/browser/server_groups/servers/pg_timetable/templates/pgt_chain/sql/default/stats.sqlweb/pgadmin/browser/server_groups/servers/pg_timetable/templates/pgt_chain/sql/default/tasks.sqlweb/pgadmin/browser/server_groups/servers/pg_timetable/templates/pgt_chain/sql/default/update.sqlweb/pgadmin/browser/server_groups/servers/pg_timetable/templates/pgt_chaintask/css/pgt_chaintask.cssweb/pgadmin/browser/server_groups/servers/pg_timetable/templates/pgt_chaintask/sql/default/create.sqlweb/pgadmin/browser/server_groups/servers/pg_timetable/templates/pgt_chaintask/sql/default/delete.sqlweb/pgadmin/browser/server_groups/servers/pg_timetable/templates/pgt_chaintask/sql/default/nodes.sqlweb/pgadmin/browser/server_groups/servers/pg_timetable/templates/pgt_chaintask/sql/default/properties.sqlweb/pgadmin/browser/server_groups/servers/pg_timetable/templates/pgt_chaintask/sql/default/stats.sqlweb/pgadmin/browser/server_groups/servers/pg_timetable/templates/pgt_chaintask/sql/default/update.sqlweb/pgadmin/browser/server_groups/servers/pg_timetable/tests/__init__.pyweb/pgadmin/browser/server_groups/servers/pg_timetable/tests/pgt_timetable_test_data.jsonweb/pgadmin/browser/server_groups/servers/pg_timetable/tests/test_pgt_chain_add.pyweb/pgadmin/browser/server_groups/servers/pg_timetable/tests/test_pgt_chain_delete.pyweb/pgadmin/browser/server_groups/servers/pg_timetable/tests/test_pgt_chain_get.pyweb/pgadmin/browser/server_groups/servers/pg_timetable/tests/test_pgt_chain_get_msql.pyweb/pgadmin/browser/server_groups/servers/pg_timetable/tests/test_pgt_chain_get_nodes.pyweb/pgadmin/browser/server_groups/servers/pg_timetable/tests/test_pgt_chain_get_statistics.pyweb/pgadmin/browser/server_groups/servers/pg_timetable/tests/test_pgt_chain_put.pyweb/pgadmin/browser/server_groups/servers/pg_timetable/tests/test_pgt_chain_sql.pyweb/pgadmin/browser/server_groups/servers/pg_timetable/tests/utils.pyweb/pgadmin/browser/server_groups/servers/pg_timetable/utils.pyweb/webpack.config.jsweb/webpack.shim.js
3dceea8 to
926e98a
Compare
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (4)
web/pgadmin/browser/server_groups/servers/pg_timetable/tasks/static/js/pgt_chaintask.ui.js (1)
182-248: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winDuplicate: command-emptiness validation only runs for
kind === 'SQL'.Still unresolved from a previous review: the
isEmptyString(state.command)check (241-246) is nested insideif (state.kind === 'SQL')(185), soPROGRAM/BUILTINtasks with an empty command bypass validation entirely and can be saved without a command/internal-command selection.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/pgadmin/browser/server_groups/servers/pg_timetable/tasks/static/js/pgt_chaintask.ui.js` around lines 182 - 248, The command-emptiness validation in validate must apply to SQL, PROGRAM, and BUILTIN tasks, not only state.kind === 'SQL'. Move the isEmptyString(state.command) check and its setError handling outside the SQL-specific block, preserving the kind-specific error message and existing database_connection validation.web/pgadmin/browser/server_groups/servers/pg_timetable/__init__.py (3)
373-373: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftDuplicate:
_process_ctasksfailures aren't propagated.Already flagged previously and still unresolved — every DB status inside
_process_ctasks(deletes/updates/inserts/param writes) is discarded, so a failed nested task mutation still yields a successfulupdate()response.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/pgadmin/browser/server_groups/servers/pg_timetable/__init__.py` at line 373, Update the update flow around _process_ctasks so failures from every nested task mutation—deletes, updates, inserts, and parameter writes—are propagated to the caller instead of discarded. Ensure _process_ctasks returns or raises the underlying DB failure and that the surrounding update() path handles it, preventing a successful response when any nested mutation fails.
460-486: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftDuplicate: partial parameter deltas delete all existing parameters.
Already flagged previously and still unresolved —
_upsert_task_paramsdeletes every stored parameter for the task before re-inserting onlyadded/changedentries, so editing one parameter drops all unchanged ones, and a deleted-only payload silently no-ops (early return at line 462/466).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/pgadmin/browser/server_groups/servers/pg_timetable/__init__.py` around lines 460 - 486, The _upsert_task_params method incorrectly treats partial parameter deltas as a complete replacement. Update it to preserve existing parameters: apply added and changed entries without deleting unchanged records, remove entries explicitly marked deleted, and ensure deleted-only payloads are processed instead of returning early; retain the existing JSON/value serialization behavior.
324-334: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDuplicate:
ENDcommits before checking the post-create lookup status.This exact issue was already flagged in a previous review and remains unresolved:
self.conn.execute_void('END')at line 332 commits the newly-created chain before theNODES_SQLstatus is checked at line 333, so a lookup failure returns an error to the client after the chain has already been persisted.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/pgadmin/browser/server_groups/servers/pg_timetable/__init__.py` around lines 324 - 334, Move the self.conn.execute_void('END') call in the post-create lookup flow to occur only after the status check succeeds. Ensure a failed NODES_SQL lookup returns internal_server_error(errormsg=res) without committing the newly created chain, while successful lookups retain the commit behavior.
🧹 Nitpick comments (2)
web/pgadmin/browser/server_groups/servers/pg_timetable/tasks/static/js/pgt_chaintask.ui.js (1)
17-40: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate inline
paramSchemaclass definitions, one lacking validation.The factory function's local
paramSchema(17-40) hasorder_id/valuevalidation; the fallback default insidePgtChainTaskSchema's constructor (74-85) redefines the same fields without anyvalidate(). The fallback is only used if a caller instantiatesPgtChainTaskSchemadirectly without passingfieldOptions.paramSchema— worth extracting a single shared class/module to avoid the two definitions drifting.Also applies to: 74-85
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/pgadmin/browser/server_groups/servers/pg_timetable/tasks/static/js/pgt_chaintask.ui.js` around lines 17 - 40, The inline paramSchema definitions are duplicated, and PgtChainTaskSchema’s constructor fallback lacks the validation present in the factory schema. Extract the shared parameter schema into one reusable class or module-level symbol, then use it both for the factory’s paramSchema and the fallback in PgtChainTaskSchema, preserving order_id and value validation.web/pgadmin/browser/server_groups/servers/pg_timetable/__init__.py (1)
291-306: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winParameter-cleanup block duplicated across three methods.
This ~15-line block (JSON-detect +
_is_jsonmarking) is duplicated almost verbatim inChainTaskView.create()andChainTaskView.update()intasks/__init__.py. Consider extracting a shared helper (e.g._clean_task_parameters(params)) to avoid drift between the three copies.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/pgadmin/browser/server_groups/servers/pg_timetable/__init__.py` around lines 291 - 306, Extract the duplicated parameter-cleanup logic into a shared helper such as _clean_task_parameters(params), including JSON detection, _is_json marking, and normalization of non-dict values. Replace the inline blocks in the current method and ChainTaskView.create() and update() with calls to this helper, preserving their existing cleaned parameter behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@web/pgadmin/browser/server_groups/servers/pg_timetable/__init__.py`:
- Around line 68-78: Guard the has_connstr query result before indexing rows in
backend_supported() and check_precondition: verify status is successful and res
contains the expected rows structure, otherwise return the existing failure
outcome without accessing res['rows'][0]. Apply the same protection to the
corresponding tasks/__init__.py check_precondition implementation.
- Around line 416-435: Update the changed-task SQL construction around field_map
so database_connection is included only when has_connstr is true. Preserve
updates for all other mapped fields and retain the existing database_connection
handling when the column is available.
- Around line 545-548: Replace the pgAgent-specific preference lookup used by
the pgTimeTable statistics and task flows with a dedicated pgTimeTable row-limit
preference, updating both __init__.py locations and registering the new
preference with an appropriate default. Ensure all pgTimeTable truncation logic
reads the new symbol while leaving pgAgent’s pgagent_row_threshold unchanged.
- Around line 511-535: Update the preview flows in the timetable view methods
msql() and sql(), and in ChainTaskView.msql(), to preserve the _is_json marker
when decoding JSON request parameters. Ensure values identified as JSON by
create()/update() reach the SQL templates with _is_json set so they render as
::jsonb rather than to_jsonb(...::text), while leaving non-JSON parameters
unchanged.
In `@web/pgadmin/browser/server_groups/servers/pg_timetable/tasks/__init__.py`:
- Around line 407-412: Update the user-facing gettext error messages in the task
creation and update handlers to use pgTimeTable terminology, replacing “Job step
creation failed.” and “Job step update failed.” with wording that refers to task
creation and task update. Keep the existing gone(errormsg=...) handling
unchanged.
- Around line 203-212: Guard the execute_dict result in the timetable metadata
initialization block before accessing res['rows'][0]. Preserve the query status
returned by self.conn.execute_dict, handle failure consistently with
ChainModule.backend_supported and ChainView.check_precondition, and only
populate self.manager.db_info['timetable'] when the query succeeds.
- Around line 171-184: Rename the initializer method _init_ to Python’s
constructor __init__ in ChainTaskView, preserving the existing conn,
template_path, manager initialization and super().__init__(**kwargs) call so
they execute when instances are created.
In
`@web/pgadmin/browser/server_groups/servers/pg_timetable/tasks/tests/test_pgt_task_put.py`:
- Around line 52-60: Update the negative-test branch in the test flow around
api_put so response is assigned when self.mocking_required is false, by calling
tasks_utils.api_put(self) in an else branch before the existing status and error
assertions.
In
`@web/pgadmin/browser/server_groups/servers/pg_timetable/templates/pgt_chain/sql/default/run_now.sql`:
- Around line 1-8: Update the SQL around notify_chain_start to validate the
selected chain’s client_name before invoking it. When client_name is empty or
NULL, select an active worker or return a clear error; only call
timetable.notify_chain_start when a valid notification channel is available.
---
Duplicate comments:
In `@web/pgadmin/browser/server_groups/servers/pg_timetable/__init__.py`:
- Line 373: Update the update flow around _process_ctasks so failures from every
nested task mutation—deletes, updates, inserts, and parameter writes—are
propagated to the caller instead of discarded. Ensure _process_ctasks returns or
raises the underlying DB failure and that the surrounding update() path handles
it, preventing a successful response when any nested mutation fails.
- Around line 460-486: The _upsert_task_params method incorrectly treats partial
parameter deltas as a complete replacement. Update it to preserve existing
parameters: apply added and changed entries without deleting unchanged records,
remove entries explicitly marked deleted, and ensure deleted-only payloads are
processed instead of returning early; retain the existing JSON/value
serialization behavior.
- Around line 324-334: Move the self.conn.execute_void('END') call in the
post-create lookup flow to occur only after the status check succeeds. Ensure a
failed NODES_SQL lookup returns internal_server_error(errormsg=res) without
committing the newly created chain, while successful lookups retain the commit
behavior.
In
`@web/pgadmin/browser/server_groups/servers/pg_timetable/tasks/static/js/pgt_chaintask.ui.js`:
- Around line 182-248: The command-emptiness validation in validate must apply
to SQL, PROGRAM, and BUILTIN tasks, not only state.kind === 'SQL'. Move the
isEmptyString(state.command) check and its setError handling outside the
SQL-specific block, preserving the kind-specific error message and existing
database_connection validation.
---
Nitpick comments:
In `@web/pgadmin/browser/server_groups/servers/pg_timetable/__init__.py`:
- Around line 291-306: Extract the duplicated parameter-cleanup logic into a
shared helper such as _clean_task_parameters(params), including JSON detection,
_is_json marking, and normalization of non-dict values. Replace the inline
blocks in the current method and ChainTaskView.create() and update() with calls
to this helper, preserving their existing cleaned parameter behavior.
In
`@web/pgadmin/browser/server_groups/servers/pg_timetable/tasks/static/js/pgt_chaintask.ui.js`:
- Around line 17-40: The inline paramSchema definitions are duplicated, and
PgtChainTaskSchema’s constructor fallback lacks the validation present in the
factory schema. Extract the shared parameter schema into one reusable class or
module-level symbol, then use it both for the factory’s paramSchema and the
fallback in PgtChainTaskSchema, preserving order_id and value validation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9e476769-a57d-4b7a-878c-4d65e22b222e
⛔ Files ignored due to path filters (6)
web/pgadmin/browser/server_groups/servers/pg_timetable/static/img/coll-pgt_chain.svgis excluded by!**/*.svgweb/pgadmin/browser/server_groups/servers/pg_timetable/static/img/pgt_chain-disabled.svgis excluded by!**/*.svgweb/pgadmin/browser/server_groups/servers/pg_timetable/static/img/pgt_chain.svgis excluded by!**/*.svgweb/pgadmin/browser/server_groups/servers/pg_timetable/tasks/static/img/coll-pgt_chaintask.svgis excluded by!**/*.svgweb/pgadmin/browser/server_groups/servers/pg_timetable/tasks/static/img/pgt_chaintask-disabled.svgis excluded by!**/*.svgweb/pgadmin/browser/server_groups/servers/pg_timetable/tasks/static/img/pgt_chaintask.svgis excluded by!**/*.svg
📒 Files selected for processing (50)
web/pgadmin/browser/server_groups/servers/__init__.pyweb/pgadmin/browser/server_groups/servers/pg_timetable/__init__.pyweb/pgadmin/browser/server_groups/servers/pg_timetable/static/css/pgt_chain.cssweb/pgadmin/browser/server_groups/servers/pg_timetable/static/js/pgt_chain.jsweb/pgadmin/browser/server_groups/servers/pg_timetable/static/js/pgt_chain.ui.jsweb/pgadmin/browser/server_groups/servers/pg_timetable/tasks/__init__.pyweb/pgadmin/browser/server_groups/servers/pg_timetable/tasks/static/js/pgt_chaintask.jsweb/pgadmin/browser/server_groups/servers/pg_timetable/tasks/static/js/pgt_chaintask.ui.jsweb/pgadmin/browser/server_groups/servers/pg_timetable/tasks/template/pgt_chaintask/css/pgt_chaintask.cssweb/pgadmin/browser/server_groups/servers/pg_timetable/tasks/tests/__init__.pyweb/pgadmin/browser/server_groups/servers/pg_timetable/tasks/tests/tasks_test_data.jsonweb/pgadmin/browser/server_groups/servers/pg_timetable/tasks/tests/test_pgt_task_add.pyweb/pgadmin/browser/server_groups/servers/pg_timetable/tasks/tests/test_pgt_task_delete.pyweb/pgadmin/browser/server_groups/servers/pg_timetable/tasks/tests/test_pgt_task_get.pyweb/pgadmin/browser/server_groups/servers/pg_timetable/tasks/tests/test_pgt_task_get_msql.pyweb/pgadmin/browser/server_groups/servers/pg_timetable/tasks/tests/test_pgt_task_get_nodes.pyweb/pgadmin/browser/server_groups/servers/pg_timetable/tasks/tests/test_pgt_task_get_statistics.pyweb/pgadmin/browser/server_groups/servers/pg_timetable/tasks/tests/test_pgt_task_put.pyweb/pgadmin/browser/server_groups/servers/pg_timetable/tasks/tests/utils.pyweb/pgadmin/browser/server_groups/servers/pg_timetable/templates/macros/pgt_chaintask.macrosweb/pgadmin/browser/server_groups/servers/pg_timetable/templates/pgt_chain/css/pgt_chain.cssweb/pgadmin/browser/server_groups/servers/pg_timetable/templates/pgt_chain/sql/default/create.sqlweb/pgadmin/browser/server_groups/servers/pg_timetable/templates/pgt_chain/sql/default/delete.sqlweb/pgadmin/browser/server_groups/servers/pg_timetable/templates/pgt_chain/sql/default/nodes.sqlweb/pgadmin/browser/server_groups/servers/pg_timetable/templates/pgt_chain/sql/default/properties.sqlweb/pgadmin/browser/server_groups/servers/pg_timetable/templates/pgt_chain/sql/default/run_now.sqlweb/pgadmin/browser/server_groups/servers/pg_timetable/templates/pgt_chain/sql/default/stats.sqlweb/pgadmin/browser/server_groups/servers/pg_timetable/templates/pgt_chain/sql/default/tasks.sqlweb/pgadmin/browser/server_groups/servers/pg_timetable/templates/pgt_chain/sql/default/update.sqlweb/pgadmin/browser/server_groups/servers/pg_timetable/templates/pgt_chaintask/css/pgt_chaintask.cssweb/pgadmin/browser/server_groups/servers/pg_timetable/templates/pgt_chaintask/sql/default/create.sqlweb/pgadmin/browser/server_groups/servers/pg_timetable/templates/pgt_chaintask/sql/default/delete.sqlweb/pgadmin/browser/server_groups/servers/pg_timetable/templates/pgt_chaintask/sql/default/nodes.sqlweb/pgadmin/browser/server_groups/servers/pg_timetable/templates/pgt_chaintask/sql/default/properties.sqlweb/pgadmin/browser/server_groups/servers/pg_timetable/templates/pgt_chaintask/sql/default/stats.sqlweb/pgadmin/browser/server_groups/servers/pg_timetable/templates/pgt_chaintask/sql/default/update.sqlweb/pgadmin/browser/server_groups/servers/pg_timetable/tests/__init__.pyweb/pgadmin/browser/server_groups/servers/pg_timetable/tests/pgt_timetable_test_data.jsonweb/pgadmin/browser/server_groups/servers/pg_timetable/tests/test_pgt_chain_add.pyweb/pgadmin/browser/server_groups/servers/pg_timetable/tests/test_pgt_chain_delete.pyweb/pgadmin/browser/server_groups/servers/pg_timetable/tests/test_pgt_chain_get.pyweb/pgadmin/browser/server_groups/servers/pg_timetable/tests/test_pgt_chain_get_msql.pyweb/pgadmin/browser/server_groups/servers/pg_timetable/tests/test_pgt_chain_get_nodes.pyweb/pgadmin/browser/server_groups/servers/pg_timetable/tests/test_pgt_chain_get_statistics.pyweb/pgadmin/browser/server_groups/servers/pg_timetable/tests/test_pgt_chain_put.pyweb/pgadmin/browser/server_groups/servers/pg_timetable/tests/test_pgt_chain_sql.pyweb/pgadmin/browser/server_groups/servers/pg_timetable/tests/utils.pyweb/pgadmin/browser/server_groups/servers/pg_timetable/utils.pyweb/webpack.config.jsweb/webpack.shim.js
🚧 Files skipped from review as they are similar to previous changes (31)
- web/pgadmin/browser/server_groups/servers/pg_timetable/static/css/pgt_chain.css
- web/pgadmin/browser/server_groups/servers/pg_timetable/tests/init.py
- web/pgadmin/browser/server_groups/servers/pg_timetable/templates/pgt_chaintask/css/pgt_chaintask.css
- web/pgadmin/browser/server_groups/servers/pg_timetable/templates/pgt_chaintask/sql/default/stats.sql
- web/pgadmin/browser/server_groups/servers/pg_timetable/templates/pgt_chaintask/sql/default/properties.sql
- web/pgadmin/browser/server_groups/servers/pg_timetable/templates/pgt_chaintask/sql/default/delete.sql
- web/pgadmin/browser/server_groups/servers/pg_timetable/templates/pgt_chaintask/sql/default/update.sql
- web/webpack.config.js
- web/pgadmin/browser/server_groups/servers/pg_timetable/tests/test_pgt_chain_get_msql.py
- web/pgadmin/browser/server_groups/servers/pg_timetable/tasks/template/pgt_chaintask/css/pgt_chaintask.css
- web/pgadmin/browser/server_groups/servers/pg_timetable/templates/pgt_chain/sql/default/properties.sql
- web/pgadmin/browser/server_groups/servers/pg_timetable/templates/pgt_chain/sql/default/delete.sql
- web/pgadmin/browser/server_groups/servers/pg_timetable/tests/test_pgt_chain_delete.py
- web/pgadmin/browser/server_groups/servers/pg_timetable/templates/pgt_chain/sql/default/nodes.sql
- web/pgadmin/browser/server_groups/servers/pg_timetable/templates/pgt_chain/sql/default/stats.sql
- web/pgadmin/browser/server_groups/servers/pg_timetable/templates/pgt_chain/css/pgt_chain.css
- web/pgadmin/browser/server_groups/servers/pg_timetable/templates/pgt_chain/sql/default/tasks.sql
- web/pgadmin/browser/server_groups/servers/pg_timetable/static/js/pgt_chain.js
- web/pgadmin/browser/server_groups/servers/pg_timetable/utils.py
- web/pgadmin/browser/server_groups/servers/pg_timetable/tasks/tests/test_pgt_task_delete.py
- web/pgadmin/browser/server_groups/servers/pg_timetable/tasks/static/js/pgt_chaintask.js
- web/pgadmin/browser/server_groups/servers/init.py
- web/pgadmin/browser/server_groups/servers/pg_timetable/templates/pgt_chaintask/sql/default/create.sql
- web/pgadmin/browser/server_groups/servers/pg_timetable/tasks/tests/tasks_test_data.json
- web/pgadmin/browser/server_groups/servers/pg_timetable/tasks/tests/test_pgt_task_get_msql.py
- web/webpack.shim.js
- web/pgadmin/browser/server_groups/servers/pg_timetable/templates/macros/pgt_chaintask.macros
- web/pgadmin/browser/server_groups/servers/pg_timetable/templates/pgt_chain/sql/default/create.sql
- web/pgadmin/browser/server_groups/servers/pg_timetable/templates/pgt_chain/sql/default/update.sql
- web/pgadmin/browser/server_groups/servers/pg_timetable/static/js/pgt_chain.ui.js
- web/pgadmin/browser/server_groups/servers/pg_timetable/tests/pgt_timetable_test_data.json
|
New UI addition to support configuring max rows of pgTimeTable 
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/pgadmin/browser/server_groups/servers/pg_timetable/__init__.py (1)
172-181: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winUnguarded query status before indexing
res['rows'][0].The previous review identified this issue in both
backend_supported()andcheck_precondition(). It was properly fixed inbackend_supported()but remains unfixed here incheck_precondition(). Please verify thatstatusis successful before accessingres['rows'][0].🐛 Proposed fix
if 'timetable' not in self.manager.db_info: - _, res = self.conn.execute_dict(""" + status, res = self.conn.execute_dict(""" SELECT EXISTS( SELECT 1 FROM information_schema.columns WHERE table_schema='timetable' AND table_name='task' AND column_name='database_connection' ) has_connstr""") - - self.manager.db_info['timetable'] = res['rows'][0] + if not status: + return internal_server_error(errormsg=res) + self.manager.db_info['timetable'] = res['rows'][0]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/pgadmin/browser/server_groups/servers/pg_timetable/__init__.py` around lines 172 - 181, Update the query handling in check_precondition() before assigning self.manager.db_info['timetable'] from res['rows'][0]. Verify the execute_dict() status indicates success first, and preserve the existing assignment only for successful queries.
♻️ Duplicate comments (3)
web/pgadmin/browser/server_groups/servers/pg_timetable/__init__.py (3)
518-531: 🎯 Functional Correctness | 🟡 Minor | 💤 Low valueKeep
_is_jsonon the preview path.This issue remains unfixed from the previous review. The SQL preview flows (
msqlandsql) do not preserve the_is_jsonmarker when decoding JSON request parameters, which causes JSON parameters to incorrectly preview asto_jsonb(...::text)instead of::jsonb.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/pgadmin/browser/server_groups/servers/pg_timetable/__init__.py` around lines 518 - 531, Update the SQL preview parameter-decoding logic in msql and its corresponding sql flow to preserve the _is_json marker when JSON request parameters are decoded. Ensure JSON values continue through preview generation as ::jsonb rather than being converted to to_jsonb(...::text), while leaving non-JSON parameter handling unchanged.
379-379: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftPropagate nested task failures and update atomically.
This issue remains unfixed from the previous review.
_process_ctasks()ignores the database execution status for every delete, update, and insert operation. A failure in nested task operations will not be propagated to the caller, and the changes are not wrapped in a single transaction with the chain update.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/pgadmin/browser/server_groups/servers/pg_timetable/__init__.py` at line 379, Update the chain-processing flow around `_process_ctasks()` so every nested delete, update, and insert checks and propagates its database execution status to the caller. Execute `_process_ctasks()` and the associated chain update within one transaction, committing only when all operations succeed and rolling back on any failure.
467-476: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftDo not delete all parameters when processing a partial delta.
This issue remains unfixed from the previous review. When handling a dictionary payload containing only partial updates (
addedandchanged), unconditionally deleting all existing parameters for the task will inadvertently remove any unchanged parameters. Applyadded,changed, anddeletedindependently.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/pgadmin/browser/server_groups/servers/pg_timetable/__init__.py` around lines 467 - 476, Update _upsert_task_params so dictionary payloads are applied as deltas: insert added parameters, update changed parameters, and delete only the parameters listed in deleted. Do not delete all existing parameters when processing partial added or changed data, while preserving the existing full-replacement behavior for non-dictionary parameter inputs if applicable.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@web/pgadmin/browser/server_groups/servers/pg_timetable/__init__.py`:
- Around line 172-181: Update the query handling in check_precondition() before
assigning self.manager.db_info['timetable'] from res['rows'][0]. Verify the
execute_dict() status indicates success first, and preserve the existing
assignment only for successful queries.
---
Duplicate comments:
In `@web/pgadmin/browser/server_groups/servers/pg_timetable/__init__.py`:
- Around line 518-531: Update the SQL preview parameter-decoding logic in msql
and its corresponding sql flow to preserve the _is_json marker when JSON request
parameters are decoded. Ensure JSON values continue through preview generation
as ::jsonb rather than being converted to to_jsonb(...::text), while leaving
non-JSON parameter handling unchanged.
- Line 379: Update the chain-processing flow around `_process_ctasks()` so every
nested delete, update, and insert checks and propagates its database execution
status to the caller. Execute `_process_ctasks()` and the associated chain
update within one transaction, committing only when all operations succeed and
rolling back on any failure.
- Around line 467-476: Update _upsert_task_params so dictionary payloads are
applied as deltas: insert added parameters, update changed parameters, and
delete only the parameters listed in deleted. Do not delete all existing
parameters when processing partial added or changed data, while preserving the
existing full-replacement behavior for non-dictionary parameter inputs if
applicable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3663c453-b077-4a8b-86db-17101e58800a
📒 Files selected for processing (3)
web/pgadmin/browser/register_browser_preferences.pyweb/pgadmin/browser/server_groups/servers/pg_timetable/__init__.pyweb/pgadmin/browser/server_groups/servers/pg_timetable/tasks/__init__.py
🚧 Files skipped from review as they are similar to previous changes (1)
- web/pgadmin/browser/server_groups/servers/pg_timetable/tasks/init.py
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/pgadmin/browser/server_groups/servers/pg_timetable/templates/macros/pgt_chaintask.macros (1)
31-35: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winScope parameter mutations by both task and chain.
The task update/delete is chain-qualified, but parameter cleanup and insertion use only
task_id. A mismatchedchain_idcan therefore clear or replace another chain’s parameters even when the task operation itself is rejected or becomes a no-op.
web/pgadmin/browser/server_groups/servers/pg_timetable/templates/macros/pgt_chaintask.macros#L31-L35: require the task to belong tochain_idfor both parameter deletion and insertion.web/pgadmin/browser/server_groups/servers/pg_timetable/templates/macros/pgt_chaintask.macros#L39-L42: apply the same chain-qualified predicate before deleting parameter rows.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/pgadmin/browser/server_groups/servers/pg_timetable/templates/macros/pgt_chaintask.macros` around lines 31 - 35, Parameter mutations in the pgt_chaintask macro are scoped only by task_id; require the task to belong to chain_id for both the DELETE and INSERT operations at web/pgadmin/browser/server_groups/servers/pg_timetable/templates/macros/pgt_chaintask.macros lines 31-35, and apply the same chain-qualified predicate to the parameter-row deletion at lines 39-42.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@web/pgadmin/browser/server_groups/servers/pg_timetable/templates/macros/pgt_chaintask.macros`:
- Around line 10-13: Fix the comma placement in the INSERT values list around
the data.kind, data.database_connection, and data.ignore_error expressions.
Ensure commas appear only between emitted values, avoiding duplicate commas when
optional fields are present and trailing commas when they are absent.
---
Outside diff comments:
In
`@web/pgadmin/browser/server_groups/servers/pg_timetable/templates/macros/pgt_chaintask.macros`:
- Around line 31-35: Parameter mutations in the pgt_chaintask macro are scoped
only by task_id; require the task to belong to chain_id for both the DELETE and
INSERT operations at
web/pgadmin/browser/server_groups/servers/pg_timetable/templates/macros/pgt_chaintask.macros
lines 31-35, and apply the same chain-qualified predicate to the parameter-row
deletion at lines 39-42.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f45ed01e-29e8-4e96-acd8-99e2b838af7a
📒 Files selected for processing (4)
web/pgadmin/browser/server_groups/servers/pg_timetable/__init__.pyweb/pgadmin/browser/server_groups/servers/pg_timetable/tasks/__init__.pyweb/pgadmin/browser/server_groups/servers/pg_timetable/templates/macros/pgt_chaintask.macrosweb/pgadmin/browser/server_groups/servers/pg_timetable/templates/pgt_chaintask/sql/default/nodes.sql
🚧 Files skipped from review as they are similar to previous changes (3)
- web/pgadmin/browser/server_groups/servers/pg_timetable/templates/pgt_chaintask/sql/default/nodes.sql
- web/pgadmin/browser/server_groups/servers/pg_timetable/init.py
- web/pgadmin/browser/server_groups/servers/pg_timetable/tasks/init.py
|
|
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/pgadmin/browser/server_groups/servers/pg_timetable/tasks/static/js/pgt_chaintask.ui.js (1)
116-126: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winClear
database_connectionwhenkindchanges from SQL.The UI must explicitly wipe the
database_connectionstate when the task kind changes toPROGRAMorBUILTIN. Currently, the field is disabled but retains its underlying value, which means stale connection strings will be sent in the update payload.Based on learnings, if a task's
kindchanges away fromSQL, ensuredatabase_connectionis explicitly wiped by returningdatabase_connection: nullto prevent stale connection strings from persisting.🛡️ Proposed fix to add `depChange` handler
{ id: 'kind', label: gettext('Kind'), type: 'select', controlProps: { allowClear: false }, cell: 'select', options: [ { label: gettext('SQL'), value: 'SQL' }, { label: gettext('PROGRAM'), value: 'PROGRAM' }, { label: gettext('BUILTIN'), value: 'BUILTIN' }, ], + depChange: (state) => { + if (state && state.kind !== 'SQL') { + state.database_connection = null; + } + return state; + }, },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/pgadmin/browser/server_groups/servers/pg_timetable/tasks/static/js/pgt_chaintask.ui.js` around lines 116 - 126, Update the kind field configuration in the task form to add a depChange handler that returns database_connection: null whenever kind changes away from SQL, while preserving the existing value for SQL. Ensure the cleared value is included in the update payload for PROGRAM and BUILTIN tasks.Source: Learnings
♻️ Duplicate comments (1)
web/pgadmin/browser/server_groups/servers/pg_timetable/tasks/static/js/pgt_chaintask.ui.js (1)
246-257: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winFix
commandvalidation scope for non-SQL tasks.The
commandvalidation logic is incorrectly nested inside theif (state.kind === 'SQL')block. As a result, if a task is of kindPROGRAMorBUILTIN, an empty command will bypass validation and not trigger a UI error, allowing invalid tasks to be submitted.Move the check outside the
ifblock so it applies to all task kinds.🐛 Proposed fix
- } else { - setError('database_connection', null); - } - - if (isEmptyString(state.command)) { - setError('command', state.kind === 'SQL' ? gettext('Please specify the SQL to execute.') : gettext('Please specify the program to execute.')); - return true; - } else { - setError('command', null); - } - } - } + } else { + setError('database_connection', null); + } + } else { + setError('database_connection', null); + } + + if (isEmptyString(state.command)) { + let msg = state.kind === 'SQL' ? gettext('Please specify the SQL to execute.') : gettext('Please specify the program to execute.'); + if (state.kind === 'BUILTIN') { + msg = gettext('Please select an internal command.'); + } + setError('command', msg); + return true; + } else { + setError('command', null); + } + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/pgadmin/browser/server_groups/servers/pg_timetable/tasks/static/js/pgt_chaintask.ui.js` around lines 246 - 257, Move the isEmptyString(state.command) validation and its setError/return logic outside the state.kind === 'SQL' conditional in the relevant validation function, while retaining the database_connection handling inside that conditional. Ensure empty commands for SQL, PROGRAM, and BUILTIN tasks all set the command error and stop validation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@web/pgadmin/browser/server_groups/servers/pg_timetable/tasks/static/js/pgt_chaintask.ui.js`:
- Around line 116-126: Update the kind field configuration in the task form to
add a depChange handler that returns database_connection: null whenever kind
changes away from SQL, while preserving the existing value for SQL. Ensure the
cleared value is included in the update payload for PROGRAM and BUILTIN tasks.
---
Duplicate comments:
In
`@web/pgadmin/browser/server_groups/servers/pg_timetable/tasks/static/js/pgt_chaintask.ui.js`:
- Around line 246-257: Move the isEmptyString(state.command) validation and its
setError/return logic outside the state.kind === 'SQL' conditional in the
relevant validation function, while retaining the database_connection handling
inside that conditional. Ensure empty commands for SQL, PROGRAM, and BUILTIN
tasks all set the command error and stop validation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bf34baef-c5b2-477b-837d-6f934509e68d
📒 Files selected for processing (15)
web/pgadmin/browser/server_groups/servers/pg_timetable/__init__.pyweb/pgadmin/browser/server_groups/servers/pg_timetable/static/js/pgt_chain.ui.jsweb/pgadmin/browser/server_groups/servers/pg_timetable/tasks/__init__.pyweb/pgadmin/browser/server_groups/servers/pg_timetable/tasks/static/js/pgt_chaintask.ui.jsweb/pgadmin/browser/server_groups/servers/pg_timetable/tasks/tests/test_pgt_task_delete.pyweb/pgadmin/browser/server_groups/servers/pg_timetable/tasks/tests/test_pgt_task_get_msql.pyweb/pgadmin/browser/server_groups/servers/pg_timetable/templates/macros/pgt_chaintask.macrosweb/pgadmin/browser/server_groups/servers/pg_timetable/templates/pgt_chain/sql/default/create.sqlweb/pgadmin/browser/server_groups/servers/pg_timetable/templates/pgt_chaintask/sql/default/create.sqlweb/pgadmin/browser/server_groups/servers/pg_timetable/templates/pgt_chaintask/sql/default/nodes.sqlweb/pgadmin/browser/server_groups/servers/pg_timetable/tests/pgt_timetable_test_data.jsonweb/pgadmin/browser/server_groups/servers/pg_timetable/tests/test_pgt_chain_delete.pyweb/pgadmin/browser/server_groups/servers/pg_timetable/tests/test_pgt_chain_get_msql.pyweb/pgadmin/browser/server_groups/servers/pg_timetable/tests/test_pgt_chain_get_nodes.pyweb/pgadmin/browser/server_groups/servers/pg_timetable/tests/test_pgt_chain_sql.py
🚧 Files skipped from review as they are similar to previous changes (14)
- web/pgadmin/browser/server_groups/servers/pg_timetable/templates/pgt_chaintask/sql/default/nodes.sql
- web/pgadmin/browser/server_groups/servers/pg_timetable/tests/test_pgt_chain_get_msql.py
- web/pgadmin/browser/server_groups/servers/pg_timetable/templates/pgt_chaintask/sql/default/create.sql
- web/pgadmin/browser/server_groups/servers/pg_timetable/tests/test_pgt_chain_delete.py
- web/pgadmin/browser/server_groups/servers/pg_timetable/tasks/tests/test_pgt_task_delete.py
- web/pgadmin/browser/server_groups/servers/pg_timetable/static/js/pgt_chain.ui.js
- web/pgadmin/browser/server_groups/servers/pg_timetable/tasks/tests/test_pgt_task_get_msql.py
- web/pgadmin/browser/server_groups/servers/pg_timetable/tests/test_pgt_chain_sql.py
- web/pgadmin/browser/server_groups/servers/pg_timetable/templates/pgt_chain/sql/default/create.sql
- web/pgadmin/browser/server_groups/servers/pg_timetable/templates/macros/pgt_chaintask.macros
- web/pgadmin/browser/server_groups/servers/pg_timetable/tests/test_pgt_chain_get_nodes.py
- web/pgadmin/browser/server_groups/servers/pg_timetable/tests/pgt_timetable_test_data.json
- web/pgadmin/browser/server_groups/servers/pg_timetable/init.py
- web/pgadmin/browser/server_groups/servers/pg_timetable/tasks/init.py
|
@coderabbitai review |
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Action performedReview triggered.
|
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
Caution Review failedAn error occurred during the review process. Please try again later. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@web/pgadmin/browser/server_groups/servers/pg_timetable/tasks/tests/test_pgt_task_put.py`:
- Around line 57-58: In the fallback branch of the test function, update the
api_put call to use the imported tasks_utils module instead of the undefined
task_utils name, matching the module reference used elsewhere in the function.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4aef7434-50df-4dbd-8615-011ff180ee93
📒 Files selected for processing (4)
web/pgadmin/browser/server_groups/servers/pg_timetable/__init__.pyweb/pgadmin/browser/server_groups/servers/pg_timetable/tasks/tests/test_pgt_task_put.pyweb/pgadmin/browser/server_groups/servers/pg_timetable/templates/pgt_chain/sql/default/run_now.sqlweb/pgadmin/browser/server_groups/servers/pg_timetable/tests/utils.py
🚧 Files skipped from review as they are similar to previous changes (1)
- web/pgadmin/browser/server_groups/servers/pg_timetable/tests/utils.py
|
@coderabbitai review |
✅ Action performedReview finished.
|
robe2
changed the title
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/en_US/pgtimetable_chains.rst`:
- Around line 41-42: Update the pgTimeTable documentation sentence to format the
menu path consistently as a navigation path and change “tables exists” to
“tables exist,” preserving the existing meaning and wording otherwise.
- Around line 8-10: Update the pgTimeTable description by adding a period after
the first sentence and revising the second sentence for grammatical correctness
and clearer phrasing, while preserving its meaning about tasks running in
sequence.
- Around line 232-236: Update the documentation sentences around the Chain and
Tasks statistics description to use the correct articles and plural forms,
including “the kind of parameters” and grammatically correct wording for past
task runs. Preserve the existing meaning and links.
- Around line 17-28: Polish the introductory paragraphs in
pgtimetable_chains.rst: revise the first paragraph for grammatical consistency,
replace “chains's execution” with “chain's execution,” and change “supports
ability” to “supports the ability.” Preserve the documented behavior and
meaning.
- Around line 101-104: Update the cron syntax paragraph to explicitly format and
name the comma and wildcard characters, clarifying that commas schedule multiple
runs and wildcards match all values. In the timezone sentence, insert “the”
before “pgTimeTable database” while preserving the existing meaning and example.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 73a758dc-c62c-4cbe-906c-911b7ae20c60
⛔ Files ignored due to path filters (6)
docs/en_US/images/pgtimetable_chain_general.pngis excluded by!**/*.pngdocs/en_US/images/pgtimetable_chain_properties.pngis excluded by!**/*.pngdocs/en_US/images/pgtimetable_task_code.pngis excluded by!**/*.pngdocs/en_US/images/pgtimetable_task_general.pngis excluded by!**/*.pngdocs/en_US/images/pgtimetable_task_parameters.pngis excluded by!**/*.pngdocs/en_US/images/pgtimetable_task_statistics.pngis excluded by!**/*.png
📒 Files selected for processing (4)
docs/en_US/index.rstdocs/en_US/pgtimetable_chains.rstweb/pgadmin/browser/server_groups/servers/pg_timetable/tasks/tests/tasks_test_data.jsonweb/pgadmin/browser/server_groups/servers/pg_timetable/tasks/tests/test_pgt_task_delete.py
🚧 Files skipped from review as they are similar to previous changes (1)
- web/pgadmin/browser/server_groups/servers/pg_timetable/tasks/tests/tasks_test_data.json
robe2
changed the title
|
I will review post 9.17 release. |
|
We need to consider whether we really want to add support for extensions/tools like this. We're already stretched thin on resources, and there's a non-trivial overhead in adding a feature of this size (which in no small part was why we deprecated our own pgAgent). In other cases we've only added support for things like EDB Advanced Server and Greenplum after being very clear with EDB and Pivotal that they must commit to supporting and maintaining the code - and in the Pivotal case, it eventually got removed when they stopped doing so. I'm leaning towards saying we shouldn't add it - it opens the door to the next scheduler or other extensions to ask for us to add support for their tool (e.g. pg_cron), and it's more to maintain. As a sidenote, it's for things like this that pgAdmin was originally built with a fully pluggable architecture, allowing people to add support for third party projects easily, outside of the main codebase. No-one ever used it though, so that architecture has slowly eroded away in favour of cleaner, more efficient code. |
I wasn't sure how to use the pluggable code architecture. If you can provide some direction there that would be helpful. |
I thought you deprecated pgAgent, because you were getting rid of pgAgent entirely. This was a pretty useful piece for my clients and ability to manage it within pgAdmin. So I chose pg_timetable as a replacement because it has all the functionality pgAgent had plus some extra bells, and I like the Go language single executable mode, and follows the PostgreSQL licensing. The only thing it doesn't have is a nice UI to manage and it seemed not much work to swap out the pgAgent with pgTimeTable thus keeping my customers still happy.
As mentioned in my last note, I'm willing to support this because the agent piece and a nice UI for it is a critical component of my customer use cases. You should know me well enough with my PostGIS work to know I take these commitments seriously.
As far as pg_cron goes, yes I was thinking of a UI for that too, but pg_cron is simple enough it can do fine without a UI. The simplicity of pg_cron is also what makes it not suitable for a lot of my workloads, that are used for other things like website backups, notifications, multi-step jobs etc that need the special functionality of each OS agent.
It looks like a large portion of the plugin architecture was ripped out at cb635f6 I'd be willing to work on putting in enough of the plugin architecture to make this work as a plugin if such work would be of interest to pgAdmin dev group. |
|
Yes, the plugin architecture is largely no more - but, I don't think we should put it back for one feature. Given that you're committing to supporting it, having given it some thought I withdraw my objection. For the benefit of future readers of this however, that's primarily because @robe2 is well known in the PostgreSQL community, so we know this won't be a drive-by contribution. |
dpage
left a comment
dpage
left a comment
There was a problem hiding this comment.
Thanks for this, Regina; it's a substantial piece of work and the shape is right. The module slots into the browser cleanly, follows the pgAgent pattern closely, and the documentation is thorough. I've gone through it in full file context rather than just the diff, and verified the findings below by rendering the templates and running the linters rather than reasoning about them, so I'm fairly confident in them.
It isn't mergeable as it stands, though: check-python-style will fail, and there are several functional defects in the parameter and task-ordering paths, one of which loses data silently and one of which corrupts the user's SQL.
Must fix
1. check-python-style CI will fail: 24 pycodestyle violations.
Running pycodestyle --config=.pycodestyle over the branch gives 21 x E501 (worst is pg_timetable/__init__.py:368 at 185 characters), one E305 at pg_timetable/__init__.py:693, and the E114/E116 pair from item 2. Also tasks/__init__.py lines 360, 364, 435, 544, 594 and tests/utils.py:98.
2. Leftover scaffolding comment in web/pgadmin/browser/server_groups/servers/__init__.py:392.
# Explicitly pull in your new testme package routeNine-space indent, hence the E114/E116, and the wording is boilerplate from wherever it was copied. It just wants deleting.
3. pgt_chaintask/sql/default/create.sql:3 silently strips semicolons out of the task's SQL command.
The | replace(';', '') filter is applied to the entire rendered macro, quoted literals included. Rendering a task whose command is SELECT 1; VACUUM; produces:
'SELECT 1 VACUUM'::text,The TASK.INSERT macro no longer emits a trailing semicolon (it ends ) RETURNING task_id), so the filter is both unnecessary and destructive now. TASK.UPDATE doesn't do this, so creating a task and then editing it give you different commands.
4. The chain SQL tab returns a 500 for any chain that has tasks.
pgt_chaintask.macros:53 returns NULL AS parameters, so ChainView.sql() passes parameters=None into pgt_chain/sql/default/create.sql:31, which then evaluates task.parameters|length. Rendering that gives TypeError: object of type 'NoneType' has no len(). The pgt_chain_sql test passes only because create_pgtimetable_chain() builds a chain with no tasks in it.
5. Task parameters are never loaded, and saving destroys the ones that already exist.
Same NULL AS parameters in the PROPERTIES macro: the Parameters grid is always empty for an existing task, both in the task dialog and in the chain dialog's Tasks tab. So if you add one parameter to a task that already has three, the save path (TASK.UPDATE lines 31-39, and _upsert_task_params at pg_timetable/__init__.py:521) issues DELETE FROM timetable.parameter WHERE task_id = ... followed by an insert of only what the UI is holding, and the other three are gone. The macro needs to aggregate the parameters properly, perhaps a LEFT JOIN LATERAL over timetable.parameter returning a JSON array.
Related, and in the same function: _upsert_task_params returns early on if not parameters, so clearing every parameter from a task leaves the old rows in place.
6. task_order is DOUBLE PRECISION, not integer.
pg_timetable's ddl.sql declares task_order DOUBLE PRECISION NOT NULL, the point being that you can slot a task in between two others. The PR casts ::integer in every template, declares type: 'int' in pgt_chaintask.ui.js:112, and does int(row['task_order']) at tasks/__init__.py:281. Since SELECT '15.5'::integer is a hard error (invalid input syntax for type integer), any chain whose tasks were created by timetable.add_task with fractional ordering can't be edited or scripted at all, and the browser label truncates the value. This wants to be ::double precision throughout, with a numeric field in the UI.
7. An empty command passes validation for PROGRAM and BUILTIN tasks.
In pgt_chaintask.ui.js:250-255 the isEmptyString(state.command) check sits inside the if (state.kind === 'SQL') block, so only SQL tasks get validated. CodeRabbit raised precisely this on line 256 and the thread is marked resolved, but the code is unchanged. As timetable.task.command is NOT NULL, what you get is a task saved with an empty command rather than a clean error.
Should fix
8. has_connstr is dead code. pgt_chaintask.macros:48 accepts has_connstr and never references it; t.database_connection is selected unconditionally, so the SELECT still fails on pg_timetable versions without that column. This is another CodeRabbit thread resolved without a corresponding change. Either honour the flag in the macro, or remove the detection query, the manager.db_info['timetable'] caching and the argument from all fifteen call sites, because at the moment it buys nothing.
9. Chain update is implemented twice, and the two implementations disagree. ChainView.update() (pg_timetable/__init__.py:368 onwards) builds its own parameterised UPDATE in Python, whilst pgt_chain/sql/default/update.sql is only ever rendered by msql. The upshot is that the SQL preview never shows task additions, changes or deletions, and any future fix has to be made in both places.
10. Deletes aren't transactional. ChainView.delete() and ChainTaskView.delete() both loop over data['ids'] issuing one statement per object with no enclosing transaction, so a failure part way through leaves some objects dropped and some not. data['ids'] will also raise KeyError on a malformed body.
11. pg_timetable/__init__.py:325 uses END where it means ROLLBACK. END is a synonym for COMMIT, and it only behaves as a rollback here because the transaction is already aborted; two lines further down the equivalent situation is handled with an explicit ROLLBACK.
12. web/pgadmin/browser/server_groups/servers/pg_timetable/utils.py is an empty stub, containing a copyright header, a docstring and an unused from flask import render_template. It should just be deleted.
13. Missing documentation image. pgtimetable_chains.rst references images/pgtimetable_chain_tasks.png, which the PR doesn't add. Sphinx isn't run with -W so the build won't fail, but the page will render a broken image.
14. is_pgtimetable_installed_on_server() ignores the privilege result. At tests/utils.py:128-133 it checks result is None but not the boolean the query returns, so a user lacking INSERT/SELECT/UPDATE on timetable.chain gets True back and the tests fail rather than skipping.
15. Latent NameError in two tests. test_pgt_chain_put.py:44-51 and test_pgt_task_delete.py only assign response inside if self.mocking_required: but then use it unconditionally. It doesn't bite today because every negative scenario mocks, but it will as soon as one doesn't. test_pgt_task_put.py gained the else branch; these two were missed.
Suggestions
getNodePgtChainTaskSchemafetches the database list viagetNodeListByName('database', ...)and passesjstdbname, and no field uses either, so that's a wasted round trip every time the task dialog opens. Probably worth dropping until the phase 2 database dropdown lands.paramSchemais defined twice: once ingetNodePgtChainTaskSchemawith avalidate(), and once inline in the constructor without one.pgt_chain.js:24lists areturncodecolumn that no endpoint returns;properties.sqlexposes it asstatus.pgt_chain/sql/default/properties.sqlselectsel.last_runtwice, on lines 7 and 15.run_nowreports "Updated the next runtime to now.", but it actually sends apg_notify; if the chain names aclient_namewith no agent listening, nothing happens and the user is told it worked.- Both
statistics()docstrings still read "Returns the statistics for a particular database ... all the databases in that server." DECLARE cid integer; tid integer;in the chain create block, whilstchain_idandtask_idareBIGSERIAL.pgt_chaintask-disabled.svgships but is never referenced, which I appreciate is because taskliveis phase 2.
Notes
A green CI run doesn't tell us much here, because every new backend test calls skipTest unless pg_timetable is installed in the maintenance database, and it isn't on the GitHub runners, so the whole suite passes by skipping. It would be worth adding a pg_timetable install step to run-python-tests-pg.yml, or at the very least running the suite locally against a real installation before this goes in; several of the items above would have been caught that way.
On the CodeRabbit threads, for the record: the foreign key ones were false positives. Both timetable.task.chain_id and timetable.parameter.task_id carry ON UPDATE CASCADE ON DELETE CASCADE in pg_timetable's ddl.sql, so the explicit child deletes in pgt_chain/sql/default/delete.sql are redundant rather than required. chain_name is NOT NULL UNIQUE too, so the SELECT chain_id ... WHERE chain_name = ... lookup in the create path is safe.
No release notes entry is needed, incidentally; we now batch those up shortly before a release rather than asking for one per PR.
|
@dpage Many thanks for the review. I'll address these items and get back to you once I have fixes in place. |
patterned after pgAgent jobs WIP for pgadmin-org#10148 - PGTimetable Chains node underneath Servers if pg_timetable is installed in maintenance database - Tasks node under Chain - Ability to add/edit a Chain - Ability to add/edit a Task under a Chain - Generate create sql script for a Chain - Ability to trigger Run Now for a change - Basic text editor for Task parameters and ability to add/edit - Icons for PGTime table to represent chains and tasks - Kind drop down list containing SQL, PROGRAM, BUILTIN - Program, drop down list if kind is "BUILTIN" - Auto increment order id of both task and parameters. Increment by 1 for parameters and 10 for tasks - Stats tab for both chain and chain task - Make database connection not required for SQL and note it defaults to pgTimeTable database - last_run, next_run, currently_running_on to the chain readonly properties screen - Regression tests for pg_timetable - Add cron help preserve layout with html pre so easy to read
- Fix: Unguarded query status before indexing res['rows'][0] - Rollback when the post-create lookup fails. - Fix: database_connection update isn't gated by has_connstr - Add a pg_timetable specific row level threshold
- Address status failures on tuple updates in _process_ctasks(), _upsert_task_params(), _upsert_task_params() - Do not return ok on partial chain-field UPDATE failures - Unguarded query status before indexing res['rows'][0] in check_precondition() - kind should not be represented as boolean, should be cast to text - Only insert to database_connection column if has_connstr
- Fix the conditional commas in the INSERT values list of chain.tasks - _init_ is not __init__ — this initializer never runs. - Prevent updating non-existent database_connection column - Another unguarded check in pg_timetable init - Change terminology from pgAgent Jobs/steps to pgTimetable Chain/tasks - Make task ordering less fragile - Fix some issues with parameter update when reordering
- Force tasks to be sorted by task_order using custom label that prefixes with task_order - Regress tests add super().setUp() to 6 test files that were missing it: test_pgt_chain_delete.py, test_pgt_chain_get_msql.py, test_pgt_chain_get_nodes.py, test_pgt_chain_sql.py, test_pgt_task_delete.py, test_pgt_task_get_msql.py - Guard undefined fields when inserting chains
- Force tasks to be sorted by task_order using custom label that prefixes with task_order - Regress tests add super().setUp() to 6 test files that were missing it: test_pgt_chain_delete.py, test_pgt_chain_get_msql.py, test_pgt_chain_get_nodes.py, test_pgt_chain_sql.py, test_pgt_task_delete.py, test_pgt_task_get_msql.py - Guard undefined fields when inserting chains
- Close the connection if it exists even when there is a failure - Always assign a response output even if mocking is not required
- run_now - when no client_name specified in the chain, run on all active agents - if no active agent and no client_name specified in chain, raise an error - fix missing : - fix parsing / deparsing of json data in timetable.parameter
- utils is_pgtimetable_installed_on_server should return exception message and format of output should be same structure as the success case
- Change task label from "<task_order> <name of task>" to "<task_order>: <name of task>" - Change run_now so it doesn't run on all active agents. Should only run on one that is active. Change wording from active session to active agent
…ndling. - Add negative tests
- Fix pycodestyle violations E305, E501 failures
- Fix pycodestyle W503 warnings
- Remove unnecessary | replace(';', '') in pgt_chaintast create.sql
- Fix task order incrementer
- Fix task parameters updating by
returning parameters as json in pgt_chaintask.macros
- Make sure old parameter rows are cleared
- task_order in tasks should be cast as double precision, not integer
and should allow decimals in UI
- minor cleanup of task_order display in tree
- Require command be filled in for PROGRAM and BUILTIN (not just SQL)
c551e39 to
d7db151
Compare
- add missing image - refer to the pg_timetable agents as pg_timetable
8. Get rid of has_connstr, since pg_timetable 4.0, there has always been a database_connection column in timetable.tasks and pg_timetable v3 the structure is too different to support 9. Get rid of chaintask in update.sql and consolidate sql generation in __init__.py 12. Remove empty utils.py 13. Missing image addressed in prior commit 14. In tests check for lack of privilege on timetable tables 15. Fix tests missing else response calls
- Remove getNodeListByName since we don't have a database dropdown May add back later in a later phase - Get rid of redundant paramSchema - pgt_chain.js change returncode to status to match with the name in sql - Get rid of redundant el.last_run in chain properties.sql - run_now should state it is using notification instead of updating tables. Also add in notice which client was notified and if no client could be notified. Include tests to test new messaging. - Fix statistics docstrings copy-paste error. Should be talking about chains - Fix int/bigint datatype mismatch for chain_id and task_id BUG FIXES - Chain properties tab is not showing the true last run, should use finished for sorting and also account for running jobs. Also expose status. - Fix W503 python style issues
- Fix PDF building by sticking to just ASCII only characters to describe the cron. - Change Note about null schedule, it is actually treated as run every minute to my surprise
I think I've resolved all the issues you raised accept for logic to install pg_timetable and also I kept the icon for disabled tasks since that is probably one of the features I'll add first in phase 2. I did test locally though to make sure the tests work properly when pg_timetable is installed. I'll have to research more about adding a install step and will do that as a separate pull request. |
3 participants
This is targetting feature #10148 https://github.com/cybertec-postgresql/pg_timetable
This pull request has the following functionality:
if pg_timetable is installed in maintenance database
SQL,PROGRAM,BUILTINIncrement by 1 for parameters and 10 for tasks
Here are some sample screenshots:
pg_timetable Chains Browser
Chain context menu
Chain SQL Generated script
Chain Edit Screen General Tab
Chain Tasks Tab
Tasks Browser
Task Context Menu
Task General Tab
Task Built-in General
Create Built-In Task Code tab
Task SQL Code Tab
[x] I still need to create the help documentation before this is ready. DONE
There are also other niceties I wanted to put in, but these aren't necessary for first version. These I will add as a phase 2 once this first phase is accepted/committed.
794 Not yet added but I tested the pull request for it in pg_timetable per my request, ability to enable/disable individual tasks like pgAgent had.
For this one I'd need to confirm the timetable.task.live column exists.
Ability to cancel a running Chain.
Have a different icon to show that a Chain is currently running.
Built-in task having a template autofill that shows the basic json format of each built-in task
Drop down for local databases like what pgAgent had -- right now I just have the database connection field exposed as is
Links to the various sections to go to the pg_timetable website help
Having the task parameter value field into a json editor since it is stored as json. This one I think will require change of other parts of pgAdmin so might leave as a phase 3.
Summary by CodeRabbit