feat: surface NPD Tooling in the Project Connections tab - #10
Merged
Conversation
Extend the Project form's dashboard via the override_doctype_dashboards hook to add a 'Tooling' group linking NPD Tooling. NPD Tooling links to Project through its 'project' field, matching the Project dashboard's default fieldname, so the connection count resolves automatically. The override is additive and idempotent. Verified on v15 and v16 (72/72).
Replace the override_doctype_dashboards hook with a custom DocType Link on Project, created idempotently in after_install (and removed in before_uninstall). This is the declarative, UI-visible Frappe mechanism for Connections and matches the app's existing after_install pattern for Project customizations. The custom (custom=1) link is preserved across bench migrate (verified), and its link_fieldname 'project' matches the Project dashboard's default fieldname so the connection count resolves automatically. Removes utils/dashboard_overrides.py and its hook; tests now assert the link surfaces in Project's dashboard data and that creation is idempotent.
after_install adds the connection on fresh installs, but existing sites that already applied the earlier tooling patch won't get it from that (already-logged) patch. Add a dedicated patch so bench migrate ensures the connection on existing installs. Reuses the idempotent helper; verified on v15 and v16 (removed the link, migrated, patch re-added it).
Durability (spec review): custom=1 only protects a DocType Link from Customize Form rewrites, not from a doctype re-import — when project.json changes, Frappe reloads it via delete_doc(for_reload=True), which drops every DocType Link row on Project, custom ones included. The one-shot patch could not recover from that once logged. Replace it with an after_migrate hook that re-asserts the connection on every migrate (verified: deleted the link on a site whose Patch Log already contained the old patch, migrated, connection restored and visible in Connections). Idempotency guard: match on parent + link_doctype instead of pinning custom=1, so a site carrying the same connection as a standard (custom=0) row from fixtures no longer gets a duplicate listed in Connections. Uninstall still deletes only the custom rows this app creates. Standards review: - Call add_project_tooling_connection() from after_install() directly rather than nesting it in create_tooling_setup(), so the already-shipped create_tooling_item_group patch keeps its original meaning; split the uninstall counterpart out as remove_project_tooling_connection(). Both docstrings are accurate again. - Extract PROJECT_TOOLING_LINK as the single spelling of the connection's identity, shared by the install guard, uninstall and tests — the guard and the idempotency assertion can no longer disagree. - Give TestProjectDashboardConnection setUp/tearDown so it stops leaving a DocType Link on the core Project doctype behind; move it to the end of the file and widen the module docstring to cover it. Full app suite 72/72 on v15; pre-commit clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Frappe's Linters workflow runs the frappe-semgrep-rules, which block frappe-manual-commit. The commit was redundant anyway: migrate invokes after_migrate hooks from post_schema_updates, which is decorated @atomic and already commits on success and rolls back if the phase raises — so the manual commit was also partially defeating that rollback. Verified the connection still survives without it: deleted the link, ran bench migrate, confirmed the row and the Connections entry from a fresh console session. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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
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.
Follow-up to #9 (merged). Adds the NPD Tooling connection to the Project form's Connections tab.
Approach
Adds a custom
DocType Linkon Project (the declarative, UI-visible Frappe mechanism for Connections) rather than a dashboard hook:install/after_install.py(add_project_tooling_connection), removed inbefore_uninstall.py(remove_project_tooling_connection). This matches the app's existing pattern — all its Project customizations are code inafter_install.NPD Tooling.projectmatches the Project dashboard's defaultfieldname, so the connection count resolves automatically.after_migratehook on everybench migrate, which is what actually makes the connection durable — see below.Durability: why
after_migrateand not a patchcustom=1only protects aDocType LinkfromCustomize Formrewrites. It does not survive a doctype re-import: wheneverproject.jsonchanges (any ERPNext upgrade touching Project), Frappe reloads it viaimport_file.delete_old_doc→delete_doc(..., for_reload=True)→delete_from_table, which drops everyDocType Linkrow parented to Project, custom ones included. Ordinary migrates skip the re-import on a hash match, which is why a naive "created → migrated → still there" check passes.A one-shot patch cannot recover from this — once it is in the Patch Log it never runs again, so the connection would be gone permanently.
install/after_migrate.pyre-runs the idempotent helper after every migrate instead (it runs after doctype sync and after patches, so it sees the post-reload state).Verified on a site whose Patch Log already contained the earlier one-shot patch: deleted the link, ran
bench migrate, and the connection was re-created and present inget_meta("Project").get_dashboard_data().Idempotency
The guard matches on
parent+link_doctypeonly — it deliberately does not pincustom=1. A site that carries the same connection as a standard (custom=0) row via fixtures orexport-customizationswould otherwise get a second link and list NPD Tooling twice in Connections. Uninstall is the mirror image: it deletes only thecustom=1rows this app creates, leaving a standard link the app did not put there alone.The link's identity is spelled once, as
PROJECT_TOOLING_LINKininstall/after_install.py, and shared by the install guard, the uninstall cleanup and the tests, so the three cannot drift apart.Why not
override_doctype_dashboards/export-customizationsbench export-customizationsfor Project bundles all of Project's standard links (Task, Timesheet, …) and every property setter on the site — not clean to ship. The programmatic custom link avoids that and stays consistent with the app.Files
install/after_install.py—PROJECT_TOOLING_LINK+add_project_tooling_connection(idempotent), called fromafter_installinstall/after_migrate.py— new; re-asserts the connection on every migratehooks.py— registersafter_migrate;override_doctype_dashboardsleft commented (unused)uninstall/before_uninstall.py—remove_project_tooling_connectionremoves the custom link on uninstalltests/test_npd_tooling.py— asserts the link surfaces in Project's dashboard data + idempotency, and cleans up after itself so it leaves noDocType Linkon the core Project doctypeNo patch and no
patches.txtchange: theafter_migratehook covers existing sites, and fresh installs get the connection fromafter_install.Verification
frappe.get_meta("Project").get_dashboard_data().bench migrate→ connection restored (see Durability above).pre-commit run --all-files→ exit 0. (The earlier revision of this branch was also verified 72/72 on v16; the post-review state has only been re-run on v15.)Note: on each site the connection appears once the app's hooks apply —
bench --site <site> migrate, which runs patches and theafter_migratehook.bench migratedoes not runafter_install; only a freshinstall-appdoes.