pg_timetable UI basic functionality#10152
Open
robe2 wants to merge 14 commits into
Open
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 | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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
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
robe2
force-pushed
the
pg_timetable_phase_1
branch
from
July 14, 2026 05:25
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
- 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
Author
|
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
- 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
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
- 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
Author
|
|
✅ 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
- 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
Author
|
@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 |
- utils is_pgtimetable_installed_on_server should return exception message and format of output should be same structure as the success case
Author
|
@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
- 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
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
…ndling. - Add negative tests
robe2
changed the title
Author
|
@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
Comment on lines
+8
to
+10
| pgTimeTable is a PostgreSQL-based scheduling agent that runs and manages what are called `Chains` | ||
| each chain is composed of a set of Tasks to run order by the sequence they should run. | ||
| A chain is assigned a single schedule using an extended cron-style format. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct grammatical errors and missing punctuation.
Add the missing period at the end of the first sentence and improve the phrasing in the second sentence for better readability.
📝 Proposed fix
-pgTimeTable is a PostgreSQL-based scheduling agent that runs and manages what are called `Chains`
-each chain is composed of a set of Tasks to run order by the sequence they should run.
+pgTimeTable is a PostgreSQL-based scheduling agent that runs and manages what are called `Chains`.
+Each chain is composed of a set of Tasks to be executed in the order they should run.
A chain is assigned a single schedule using an extended cron-style format.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| pgTimeTable is a PostgreSQL-based scheduling agent that runs and manages what are called `Chains` | |
| each chain is composed of a set of Tasks to run order by the sequence they should run. | |
| A chain is assigned a single schedule using an extended cron-style format. | |
| pgTimeTable is a PostgreSQL-based scheduling agent that runs and manages what are called `Chains`. | |
| Each chain is composed of a set of Tasks to be executed in the order they should run. | |
| A chain is assigned a single schedule using an extended cron-style format. |
🤖 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 `@docs/en_US/pgtimetable_chains.rst` around lines 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.
Comment on lines
+17
to
+28
| Switches on the *pgTimeTable Chain* dialog (accessed | ||
| through the *Properties* context menu) allow you to modify a chain or disable the whole chain | ||
| as well as modifying tasks within the chain. | ||
|
|
||
| When you highlight the name of a defined chain in the pgAdmin tree control, the | ||
| *Properties* tab of the main pgAdmin window will display details about the chain, | ||
| and the *Statistics* tab will display details about the chains's execution. | ||
| There are also statistics about each task. | ||
|
|
||
| pgTimeTable supports ability to have more than one job agent running at a time. | ||
| Job agents can be deployed on various platforms and binaries are readily available to | ||
| deploy on Linux, Windows, and Mac. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix typos and grammatical inconsistencies.
Fix the pluralization of "chain's", add the missing article "the", and slightly adjust the phrasing in the first paragraph.
📝 Proposed fix
Switches on the *pgTimeTable Chain* dialog (accessed
-through the *Properties* context menu) allow you to modify a chain or disable the whole chain
-as well as modifying tasks within the chain.
+through the *Properties* context menu) allow you to modify a chain or disable the whole chain,
+as well as modify tasks within the chain.
When you highlight the name of a defined chain in the pgAdmin tree control, the
*Properties* tab of the main pgAdmin window will display details about the chain,
-and the *Statistics* tab will display details about the chains's execution.
+and the *Statistics* tab will display details about the chain's execution.
There are also statistics about each task.
-pgTimeTable supports ability to have more than one job agent running at a time.
+pgTimeTable supports the ability to have more than one job agent running at a time.
Job agents can be deployed on various platforms and binaries are readily available to
deploy on Linux, Windows, and Mac.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| Switches on the *pgTimeTable Chain* dialog (accessed | |
| through the *Properties* context menu) allow you to modify a chain or disable the whole chain | |
| as well as modifying tasks within the chain. | |
| When you highlight the name of a defined chain in the pgAdmin tree control, the | |
| *Properties* tab of the main pgAdmin window will display details about the chain, | |
| and the *Statistics* tab will display details about the chains's execution. | |
| There are also statistics about each task. | |
| pgTimeTable supports ability to have more than one job agent running at a time. | |
| Job agents can be deployed on various platforms and binaries are readily available to | |
| deploy on Linux, Windows, and Mac. | |
| Switches on the *pgTimeTable Chain* dialog (accessed | |
| through the *Properties* context menu) allow you to modify a chain or disable the whole chain, | |
| as well as modify tasks within the chain. | |
| When you highlight the name of a defined chain in the pgAdmin tree control, the | |
| *Properties* tab of the main pgAdmin window will display details about the chain, | |
| and the *Statistics* tab will display details about the chain's execution. | |
| There are also statistics about each task. | |
| pgTimeTable supports the ability to have more than one job agent running at a time. | |
| Job agents can be deployed on various platforms and binaries are readily available to | |
| deploy on Linux, Windows, and Mac. |
🤖 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 `@docs/en_US/pgtimetable_chains.rst` around lines 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.
Comment on lines
+41
to
+42
| To enable viewing of the pgTimeTable node, enable it on the File->Preferences->Nodes. | ||
| It will only show if the schema *timetable* and associated tables exists in your maintenance database. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct grammar and navigation path syntax.
Adjust the menu path formatting and fix the plural/singular mismatch for "tables exist".
📝 Proposed fix
-To enable viewing of the pgTimeTable node, enable it on the File->Preferences->Nodes.
-It will only show if the schema *timetable* and associated tables exists in your maintenance database.
+To enable viewing of the pgTimeTable node, enable it under File -> Preferences -> Nodes.
+It will only show if the schema *timetable* and associated tables exist in your maintenance database.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| To enable viewing of the pgTimeTable node, enable it on the File->Preferences->Nodes. | |
| It will only show if the schema *timetable* and associated tables exists in your maintenance database. | |
| To enable viewing of the pgTimeTable node, enable it under File -> Preferences -> Nodes. | |
| It will only show if the schema *timetable* and associated tables exist in your maintenance database. |
🤖 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 `@docs/en_US/pgtimetable_chains.rst` around lines 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.
Comment on lines
+101
to
+104
| The format consists of a single field with 5 slots. As with regular cron | ||
| you can use a , to run multiple and wildcard for all. | ||
| E.g. `20 1,5 * * *` would run at 1:20 and 5:20 AM. | ||
| The timezone is always the timezone of the server pgTimetable database resides on. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Improve formatting and missing words.
Use explicit mentions and formatting for the cron wildcards and insert the missing "the" before "pgTimeTable database".
📝 Proposed fix
- The format consists of a single field with 5 slots. As with regular cron
- you can use a , to run multiple and wildcard for all.
- E.g. `20 1,5 * * *` would run at 1:20 and 5:20 AM.
- The timezone is always the timezone of the server pgTimetable database resides on.
+ The format consists of a single field with 5 slots. As with regular cron,
+ you can use a comma (`,`) for multiple values and an asterisk (`*`) as a wildcard for all.
+ E.g. `20 1,5 * * *` would run at 1:20 and 5:20 AM.
+ The timezone is always the timezone of the server the pgTimeTable database resides on.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| The format consists of a single field with 5 slots. As with regular cron | |
| you can use a , to run multiple and wildcard for all. | |
| E.g. `20 1,5 * * *` would run at 1:20 and 5:20 AM. | |
| The timezone is always the timezone of the server pgTimetable database resides on. | |
| The format consists of a single field with 5 slots. As with regular cron, | |
| you can use a comma (`,`) for multiple values and an asterisk (`*`) as a wildcard for all. | |
| E.g. `20 1,5 * * *` would run at 1:20 and 5:20 AM. | |
| The timezone is always the timezone of the server the pgTimeTable database resides on. |
🤖 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 `@docs/en_US/pgtimetable_chains.rst` around lines 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.
Comment on lines
+232
to
+236
| Refer to `pgTimeTable samples <https://cybertec-postgresql.github.io/pg_timetable/latest/samples/>`_ | ||
| for kind of parameters expected for each type. | ||
|
|
||
|
|
||
| Both the Chain and Tasks have a statistics tab, which shows results of past run of tasks. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix missing articles and plural forms.
📝 Proposed fix
Refer to `pgTimeTable samples <https://cybertec-postgresql.github.io/pg_timetable/latest/samples/>`_
-for kind of parameters expected for each type.
+for the kind of parameters expected for each type.
-Both the Chain and Tasks have a statistics tab, which shows results of past run of tasks.
+Both the Chain and Tasks have a statistics tab, which shows the results of past runs of tasks.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| Refer to `pgTimeTable samples <https://cybertec-postgresql.github.io/pg_timetable/latest/samples/>`_ | |
| for kind of parameters expected for each type. | |
| Both the Chain and Tasks have a statistics tab, which shows results of past run of tasks. | |
| Refer to `pgTimeTable samples <https://cybertec-postgresql.github.io/pg_timetable/latest/samples/>`_ | |
| for the kind of parameters expected for each type. | |
| Both the Chain and Tasks have a statistics tab, which shows the results of past runs of tasks. |
🤖 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 `@docs/en_US/pgtimetable_chains.rst` around lines 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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
I still need to create the help documentation before this is ready.
There are also other niceties I wanted to put in, but these aren't necessary for first version
Summary by CodeRabbit