Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added ADR around nested facet logic implementation #3020

Merged
merged 1 commit into from
Feb 21, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .adr-dir
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
docs/adr
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
# 1. Implement nested facets tagging for specialist documents as a single dropdown

Date: 2025-02-14

## Status

Proposed

## Related PRs

- [Implemented as part of this PR](https://github.com/alphagov/specialist-publisher/pull/2923)

## Context

Nested facets are required by a department as part of their new finder. Their existing finder contains:

- For filtering, a "Facet" dropdown and a "Sub facet" dropdown to filter for documents. Users are able to filter through a single facet value lookup at a time.
- The documents are tagged with multiple Facet and Sub facet values

We need to be able to replicate this behaviour with minimal efforts to meet a deadline.

## Decision

What we have implemented:

- On the publisher side for tagging, a single dropdown that merges the facets' values (main facet and sub facets, essentially leaf nodes).
- we allow main facets to be defined without subfacets. It would show up in the dropdown if that is the case. For the initial finder we are creating, they don't have any main facet value with no sub-facets.
- if the main facet has sub-facets, then you can only choose the sub-facets (leaf nodes). We don't allow tagging of main facet if the value has sub-facets available. Reason being we still have things to clarify about that behaviour (is it equivalent to tagging all subs, or no subs). Since the current finder doesn't require it, we de-scope this further down the line.
- When we send the document to Publishing API, we write logic into the document model to figure out whether the leaf node is a parent facet or sub-facet value in the dropdown, and split the metadata we send to Publishing API into its own facet and a separate sub-facet in `specialist-publisher/app/models/document.rb`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just checking whether what's written here is what's been implemented. The ADR implies (to me) that publishers can choose both "sub facets" and parent "facets" from the dropdown - but I'm only seeing subfacets in the dropdown on https://specialist-publisher.integration.publishing.service.gov.uk/trademark-decisions/new.

Copy link
Contributor Author

@minhngocd minhngocd Feb 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ChrisBAshton similar to point above - in our implementation:

  • we allow main facets to be defined without subfacets. It would show up in the dropdown if that is the case. Unfortunately for this finder, it doesn't have any main facets without sub facets.
  • what we don't allow, is if the main facet has sub-facets, then you can only choose the sub-facets (leaf nodes). Reason being we still have things to clarify about that behaviour (is it equivalent to tagging all subs, or no subs). Since the current finder doesn't require it, i thought de-scoping would be fine.

happy to discuss further

- Reasons for this being if we send them as separate facets, we can keep Publishing API and Search API functionality unchanged.
- When we load the edit page or new page again, the document is loaded from Publishing API, with the facet and sub-facet being separate facets, but the dropdown element expects a combined leaf node dropdown as mentioned in the first point, so we have to merge them back together again into the main facet field in `specialist-publisher/app/controllers/documents_controller.rb`.

## Consequences

Pros:

- Restrict user input to only valid options with a single dropdown.
- This supports both multiple select entries and single select entries.
- The workflow discussed as part of analysis document works.

Cons:

- Logic to merge the parent facet values, sub-facet values at form phase, split them at send phase, merge them again at load phase might be a little convoluted.

## Alternatives proposed

### Have tagging done as 2 separate facets with validation

Even on publisher side, surface all of a facet's parents facets as one dropdown, and all of the sub-facets as a separate dropdown. As outlined on [this commit](https://github.com/alphagov/specialist-publisher/commit/c7346db771df5ccafd5fff8f7f687a8211436f6a).

Pros:

- Consistency of 2 separate facets being displayed to users upon tagging and viewing metadata.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this consistency is really a pro, is it? These are two separate user groups so there isn't really any added value in having a similar UX

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yea agreed, i'm also not keen on having validation to cross check across the two fields - but we'd note it here in any case as part of the alternatives we'd considered.

- No change logic required to split or merge facets

Cons:

- Repeat of information. The sub-facets are also prefixed with parent facet in any case (e.g. to differentiate between multiple "Not Applicable" sub-facet entries). Users will have to tag both parent facet and the sub-facet with the prefix.
- There is no structural tie between the parent facet and the sub-facet when tagging. Nothing stopping the user from tagging mismatching pairs or not tagging a sub-facet.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Not tagging a sub-facet" is allowed though, isn't it? As per our assumptions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm I'm not sure we clarified between

  • Having a mixture of leaf nodes - hence tagging a main-facet because it doesn't have sub-facets
  • Tagging a main-facet only despite it also having sub-facets

I think in the future we do want to support tagging main-facet only despite having sub-facets, but we felt like we could clarify that later down the line, as the current MVP doesn't require it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh fair enough.

- In order to get around this, we would have to implement some validation on the backend to ensure users are adding correct pairs.

### Remove support for "mixed" leaf nodes and only support sub-facet entry only

The finder we are supporting as MVP do not require the need to support a mixture of top level parent facet values and sub-facet values. We could try and simplify by only having the dropdown of only sub-facets from a given facet.

Pros:

- No requirement to figure out whether a parameter value is a parent facet node or sub-facet node. We can assume all parameters of a nested-facet type field is a sub-facet value
- Consistency of saving and loading data as we are consistently working with sub-facets and the only logic required is to additionally tag parent-facet upon saving.

Cons:

- Does not support scenarios of "mix leaf node" where some facet values do not have sub-facets.
Loading