Skip to content

feat(ontology): add Cohort Discovery rule deletion, fix stale URIs and SWRL raw-editor sync - #148

Open
jeremiaspf wants to merge 1 commit into
databrickslabs:developfrom
jeremiaspf:feat/designer-cohort-delete-and-stale-uri
Open

feat(ontology): add Cohort Discovery rule deletion, fix stale URIs and SWRL raw-editor sync#148
jeremiaspf wants to merge 1 commit into
databrickslabs:developfrom
jeremiaspf:feat/designer-cohort-delete-and-stale-uri

Conversation

@jeremiaspf

Copy link
Copy Markdown

What

Three unrelated fixes in the Ontology Designer's Cohort Discovery, Expressions & Axioms, and SWRL screens, grouped together because they're small and touch neighboring code.

Cohort Discovery had no way to delete a rule from the UI. The backend delete endpoint already existed; query-cohorts.js gets _syncDeleteBtn/deleteActiveRule and _ontology_cohorts.html gets the "Delete rule" button (next to "Save rule", disabled until a rule is loaded) to actually call it.

Cohort Discovery also had the same stale-URI problem described in the reasoning/generation PRs, but more visibly: nothing on this screen reconstructed a class/property URI from its current Base URI, so a re-based domain could make an apparently correctly-configured rule show 0 members/0 edges with no obvious cause. Adds base-URI tracking and a _currentUri() helper, used everywhere the screen previously read uri/iri/id directly (class selection, lookup-by-URI, available data properties, compatible relationship properties between two entities, each hop's dropdown).

Expressions & Axioms editor now prefers an element's simple name over its stored URI when populating class/property dropdowns (previously the reverse). The backend (OntologyGenerator._resolve_uri, see the companion generation PR) already rebuilds the correct URI from a simple name against the current Base URI — sending the name instead of a potentially-stale URI avoids axioms carrying a stale URI forward in the first place.

SWRL rule editor: the entity filter for the rule graph now also includes direct subclasses of entities that participate in a business relationship, even when the subclass itself has none — previously such a subclass couldn't be referenced or typed against in a rule, despite being a reasonable thing to do. The raw-text editor's auto-sync from the visual graph selection is now suppressed while the editor is in raw mode, so typing an atom with no corresponding graph node (e.g. a derived class in a THEN clause) is no longer silently overwritten. Also switches the D3.js CDN to jsdelivr (same change as the companion frontend PR — this file needed other edits too, so it's included here rather than split further).

Why

The delete-rule gap and the stale-URI issue both make Cohort Discovery awkward to actually use day to day. The axioms/SWRL fixes are UX papercuts that were easy to fix alongside the above while in the same area of the codebase.

How to test

  1. Create a Cohort Discovery rule, save it, then use the new "Delete rule" button — it should remove the rule and the button should be disabled again with no rule loaded.
  2. Rename a domain's Base URI after creating classes/relationships used in a Cohort rule — before this fix, the rule shows 0 members/edges; after, it resolves correctly.
  3. Save an axiom in Expressions & Axioms referencing a class, rename the domain's Base URI, reopen the editor — the dropdown should still resolve the class correctly.
  4. In the SWRL editor, reference a subclass with no relationships of its own in a rule — it should now appear in the graph. Switch to raw-text mode and type an atom with no matching graph node — it should no longer get overwritten by the visual sync.

…d SWRL raw-editor sync

query-cohorts.js now tracks the loaded ontology's base_uri and reconstructs
any class/property URI from its local name against it, instead of trusting
the uri/iri/id field on the object (which can be stale after the domain is
re-based — see the companion reasoning/generation PRs for the same root
cause elsewhere). Previously nothing reconstructed these URIs on this
screen, so a re-based domain could make an apparently well-configured
Cohort Discovery rule show 0 members/edges.

Adds a "Delete rule" button to the Cohort Discovery designer (JS wiring
in query-cohorts.js, markup in _ontology_cohorts.html). The backend
delete endpoint already existed; there was simply no way to trigger it
from the design screen.

ontology-axioms.js now prefers an element's simple name over its stored
URI when populating the class/property dropdowns of the Expressions &
Axioms editor (previously the reverse). The backend
(OntologyGenerator._resolve_uri) always rebuilds the correct URI from a
simple name against the current Base URI, so this avoids axioms carrying
forward a stale stored URI.

ontology-swrl.js: the entity filter for the SWRL rule graph now also
includes direct subclasses of entities that participate in a business
relationship, even when the subclass itself has none — previously such a
subclass could not be referenced or typed against in a rule. The raw-text
editor's auto-sync from the visual graph selection is now suppressed
while the editor is in raw mode, so typing an atom that has no
corresponding graph node (e.g. a derived class in a THEN clause) is no
longer silently overwritten. Also switches the D3.js CDN from d3js.org to
jsdelivr (see the companion frontend PR for the full rationale).
@jeremiaspf
jeremiaspf requested a review from a team as a code owner August 22, 2026 09:27
@CLAassistant

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@benoitcayladbx

Copy link
Copy Markdown
Collaborator

Review — not ready to merge

Looked at the four-file diff against develop. Direction is right (Cohort delete belongs in the designer; stale URIs on this screen are a real footgun), but a few issues should land before merge. I did not merge.

Blockers

1. _currentUri() always mints hash URIs — that disagrees with the rest of the stack (query-cohorts.js)

const base = (this.baseUri || '').replace(/[#/]+$/, '');
if (base && name) return `${base}#${name}`;

Ontology.ensure_uris and OntologyGenerator._resolve_uri do base_uri + localName (after ensuring the base already ends in # or /). CohortBuilder._to_ontology_uri picks the separator from the live base (# if the base ends with #, else /). Tests in tests/units/dtwin/ use slash bases (http://acme/http://acme/Person).

This helper would emit http://acme#Person for that ontology. Re-saving a Cohort rule after a rebase would then persist the wrong URI, which is worse than the stale-URI bug.

Please join the same way as _resolve_uri / ensure_uris: do not strip the terminator and force #. Fallback: if the base has neither terminator, append # then concatenate the local name.

Also remap already-saved class_uri / hop via / target_class by local name on load. _currentUri only helps new dropdown picks; a stored stale URI still will not match this._currentUri(c) === hop.via, so _renderHopRow will blank the hop instead of rewriting it.

2. _dataPropsForClass behaviour change looks like a regression (query-cohorts.js)

Previously: ontology-level data properties, filtered by domain, with a fallback to the full list.

Now: nested cls.dataProperties || cls.properties || cls.attributes, and [] if _classByUri misses.

Two problems:

  • If the class lookup fails (stale URI on a loaded rule — see above), filters disappear instead of degrading.
  • Nested dataProperties is only guaranteed after Ontology.sync_class_data_properties. If a payload still only has top-level properties[] with domain, the where-clause dropdown goes empty.

Keep the reconstructed URI, but keep the previous source + fallback, or merge nested + ontology-level lists.

3. Spanish comment — .cursor/05 §English-only artifacts (ontology-swrl.js)

// Raw editor sync — solo si NO estamos en modo raw; ...

Rewrite that comment in English. Product UI copy can stay localised; comments cannot.

4. No tests for new behaviour (.cursor/08, src/.coding_rules.md §10 / §11.7)

This is user-visible JS with a real behavioural fork (_dataPropsForClass, _currentUri, SWRL connected-set, raw-mode guard). There is already a pattern of source-level pytest under tests/units/front/ (see test_manual_mapping_panel_host.py). Please add at least:

  • _currentUri: hash base, slash base, missing name, missing base
  • _dataPropsForClass: nested props vs ontology-level domain fallback
  • SWRL: raw-mode does not overwrite #swrlRawAntecedent / #swrlRawConsequent; inheritance one-hop inclusion
  • Cohort template: #cohortDeleteBtn exists and is disabled until a rule is loaded

5. Changelog (.cursorrules post-change routine)

No /changelogs/v0.8.0/ section. Add one in English (title, context, numbered file list, tests).

Should-fix before merge

  • CLA status is still pending on the PR (cla-assistant). We cannot merge until that is green — recheck if you already signed.
  • Delete UX: deleteRule already uses native confirm(). .cursor/05 §Frontend forbids alert/confirm/prompt; use showDeleteConfirm from front/static/global/js/utils.js. Worth fixing on this path now that Delete is a first-class designer action.
  • Axioms populateSelect: preferring name over uri is correct for in-domain classes and matches _resolve_uri. It will rewrite imported / foreign-namespace terms into the current base. If Expressions & Axioms can reference external IRIs, keep uri when it is not under the current base_uri.
  • SWRL inheritance: only direct subclasses of a connected parent are added (one pass). Call that out in the comment, or walk the subclass closure if grandchild types should also be legal in a rule.
  • CDN: jsdelivr for D3 matches the companion frontend PR — good, keep it identical (version pin). Don't land this file's CDN change if that companion is still churning the same URL.

Process / non-blocking

  • Three independent fixes in one commit (Cohort delete, Cohort stale URI, SWRL/axioms). Fine if we want them in 0.8.0 together; painful to revert. Prefer two commits (Cohort vs SWRL/axioms) next time.
  • Inline onclick="CohortModule.deleteActiveRule()" matches the existing New/Save buttons on this partial — not a new violation of .cursor/05 §Frontend (no new inline JS pattern).
  • btn-outline-danger is fine: .cursor/11 greys Bootstrap colour variants on purpose.
  • No session-data fields involved.
  • GitHub reports no CI checks on the head branch. Confirm Actions actually ran on the fork PR.

What looks good

  • Wiring delete through the existing DELETE /dtwin/cohorts/rules/:id instead of a new endpoint.
  • _syncDeleteBtn gated on activeRuleId.
  • Suppressing visual→raw overwrite while rawMode is on — that's the right source-of-truth split.
  • Including direct subclasses of connected entities in the SWRL graph — reasonable UX, once documented.

Suggested retest (after the URI join fix)

  1. Delete rule from the designer — confirm + list refresh + button disabled.
  2. Slash base_uri (http://example.org/ns/) and hash base_uri — dropdown values must match ensure_uris.
  3. Rebase domain, reload a previously saved rule — hops/filters should remap, not clear.
  4. Axiom dropdowns: in-domain class after rebase; if you have an imported class, its IRI must not be rewritten.
  5. SWRL: subtype with no own relationships is selectable; raw editor keeps typed atoms that are not graph nodes.

Happy to re-review once _currentUri matches _resolve_uri, _dataPropsForClass keeps a fallback, the Spanish comment is gone, and there is a changelog + a couple of front unit tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants