department-of-veterans-affairs / va.gov-cms

Editor-centered management for Veteran-centered content.
https://prod.cms.va.gov
GNU General Public License v2.0
99 stars 69 forks source link

VA Benefits Taxonomy Build Ph 1: Implement structure of fields in new taxonomy #13070

Closed wesrowe closed 1 year ago

wesrowe commented 1 year ago

Description

User story

AS A PW developer I WANT to stand up the basic implementation SO THAT I and team members can check for surprises in a review instance.

Engineering notes / background

Source of truth for what should be implemented is the VA Benefit Taxonomy spec (Google sheet; please request an Excel copy if you need one.)

Analytics considerations

n/a

Quality / testing notes

No QA, this is a rough pass.

Acceptance criteria

The ACs below apply only to rows 4-17 to all rows of the spec spreadsheet (the ones finalized in the meetings on 3/29 and 4/6)

NOT in scope:

Team

Please check the team(s) that will do this work.

swirtSJW commented 1 year ago

I just updated the ACs to reflect revisions and workflow.

wesrowe commented 1 year ago

@swirtSJW, we just discussed internally and decided to punt the workflow ACs to a future ticket so that we can leverage Facilities' work on adding the workflow module to taxonomies. Hopefully that makes sense to you.

jilladams commented 1 year ago

Re: https://github.com/department-of-veterans-affairs/va.gov-cms/issues/12383#issuecomment-1490949417

The VA Benefit API ID can NOT be modified after a Benefit is published

The Services taxonomy has this paradigm in effect already, with permissions management too. @swirtSJW is the person to talk to about how that works, I believe.

wesrowe commented 1 year ago

Note that I focused the ACs on the fields that we reviewed in yesterday's meeting.

chri5tia commented 1 year ago

Dumping my concerns about using taxonomy vs. content type for this here. As I find satisfactory answers in conversations past and present, I'll check them off.

Notes

chri5tia commented 1 year ago

Decisions we need to make:

swirtSJW commented 1 year ago

Editors can not go to add > content because taxonomy is part of structure, usually closed off to editors except in rare cases of websites that allow ad-hoc creation of taxonomy terms on the content add form (tagging).

Yes this is true, but this is not for most editors, this is a very select set. Also in most of those cases node content translates to actual pages. Benefits terms would not be FE pages, only ways to connect one thing to another.

Taxonomy does not have workflow draft, publish state, etc

They can, and they will. This is part of the build, not a blocker.

What problem are we trying to solve by using taxonomy instead of a content type? What are the pros and cons stress/time/technical-debt-wise of re-engineering Taxonomy to function as a content type?

Mainly, we are not trying to have it serve as a content type. We are trying to have it make connections to other content types, which is what taxonmy was built to do.

How long has the Service taxonomy been in use on our site and what is the editorial workflow for that at the moment? Who creates the terms, (role), how is the content change-logged, how is it used?

Many years. There is minimal editorial workflow on it currently because Dave C has been the only editor of it.

Notes

Both types create their own front-facing pages (front-end of the back end). For example: https://va-gov-cms.ddev.site/health-care/adaptive-sports is the page for the Taxonomy Vocabulary "VA Services" and the term "Adaptive Sports"

We have not instances where current taxonomy is creating a front end page. The page you reference has no front end counterpart. There is no https://www.va.gov/health-care/adaptive-sports

chri5tia commented 1 year ago

@swirtSJW These clarifications are helpful. It sounds like I can come to you with any blockers for building or conceptualizing this direction.

chri5tia commented 1 year ago

@swirtSJW Any thoughts on creating this as a module instead of importing field configs? It might be nice to have the module framework to house any special tweaks and hooks, or other features down the road relating to benefits. I don't see any pattern currently used to manage content types and field configs with a custom module, so maybe there's a good reason for that.

I like the idea of being able to look at or write a yaml for field configs for something like this.

chri5tia commented 1 year ago

4/3 checkpoint notes:

Pertaining to fields not yet reviewed/approved (after line 17 in spec):

Roadblocks:

Still open as of 4/5 checkpoint:

chri5tia commented 1 year ago

Things for next check-in, to do, etc.:

Screenshot 2023-04-05 at 1 12 05 PM

chri5tia commented 1 year ago

4/5 checkpoint notes

Other items discussed on 4/3 progress can be viewed in this comment

jilladams commented 1 year ago

Proposed Taxonomy description to show on the Taxonomy admin page:

Single source of truth for VA Benefits including official names, related legislation, related / pre-requisite benefits, application / eligibility notes, and related VA Forms.

This gets saved in code and cannot actually be modified in the Drupal UI (though it makes you think it can). So edits should be made prior to PR merge if at all possible.

chri5tia commented 1 year ago

Pushed API lockdown code and custom va_gov_benefits module code. Also config changes as of the 3/5 checkpoint and most of the changes from the approval meeting today. Going over more finely especially with respect to translation.

Working on re-labeling the "Name" field to "Official Benefit name" and locking it down.

davidmpickett commented 1 year ago

Notes:

Screenshot 2023-04-07 095048

Screenshot 2023-04-07 095343

davidmpickett commented 1 year ago

screencapture-cms-i7n92wfzswpe358hivrspihf2ntzshz

davidmpickett commented 1 year ago

Why can't I add wre as a co considered benefit to clatawrir?

Screenshot 2023-04-07 103218

chri5tia commented 1 year ago

It may either be because I have it preventing circular references, which in this case is probably not a good thing, or a bug with the module I used to generate content. I noticed things with the devel generate being a little weird but I'll double check the entity reference settings.

All that dummy content will get reset when I push some changes. We may each want to go through and create a test item and then go back and link to each other's tests to rule out a quirk with devel.

But most likely, it was due to one of these settings, which I am turning off: Screenshot 2023-04-07 at 1 06 03 PM

chri5tia commented 1 year ago
  • Logged in as a Content Admin I can't add an Official Benefit name. Per the spec, I should be able to. Maybe that was an intentional change, just noting this discrepancy

screencapture-cms-i7n92wfzswpe358hivrspihf2ntzshz

Dave C. Mentioned not wanting to allow this name to be changed in favor of archiving and recreating a new term. Let me know your thoughts. I might have gotten lost in translation for the final spec.

chri5tia commented 1 year ago

Something I missed is enforcing uniqueness for the taxonomy term e.g. Official benefit name. I noticed that we are also able to create more than one VA Service with the same name as well. It may be worth installing this module if we think it's something that would be used in more than one or two places in the site: https://www.drupal.org/project/taxonomy_unique

swirtSJW commented 1 year ago

If we need to enforce uniqueness we should be using https://www.drupal.org/project/allow_only_one like we do elsewhere in the codebase to identify what property(ies) make something unique and prevent violations of that.

swirtSJW commented 1 year ago

Allow only one is a field that can assess mulitple properties to prevent duplicates image

chri5tia commented 1 year ago

@swirtSJW I was trying to figure out how to apply this to the core taxonomy term name field, which isn't managed in the UI. Any ideas?

Update: I need to prevent the term name from being duplicated. I am not understanding what this service is doing on the VA Services end. You are still able to create duplicate VA Services.

Screenshot 2023-04-10 at 3 40 05 PM

Taxonomy Unique module does this:

Screenshot 2023-04-08 at 6 59 34 PM

I don't want to add another module if we don't need it so is it possible to use the existing pattern in this way?

chri5tia commented 1 year ago

Close out notes:

Added field group headers to the spec

Phase 1 4/7 check point items:

Other fields signed off.

Create tickets for these extracted technical pieces:

(Jill edited this comment 4/11, 1:40pm)

jilladams commented 1 year ago

@chri5tia is "Change label and help text of Taxonomy name property (Official benefit name)" still an open item? I thought I saw during demo that that was handled.

Otherwise, @wesrowe FYI - open items here we need sign off from you that we are ok to ticket those pieces separately and close this out.

chri5tia commented 1 year ago

That is done. If we don't already have a ticket for it, we don't need to. I'll rearrange my notes. There may be a better way to do it, and don't want to lose sight of that, but I can just put a note in the phase 4 ticket to revisit it on a code level as part of a fine review.

chri5tia commented 1 year ago

Demo: https://cms-i7n92wfzswpe358hivrspihf2ntzshzp.ci.cms.va.gov/admin/structure/taxonomy/manage/va_benefits_taxonomy/overview

chri5tia commented 1 year ago

In hind-sight, this was an 8 or 13 because it was more than laying the tracks.

wesrowe commented 1 year ago

Meets ACs for Ph 1. Closing.

swirtSJW commented 1 year ago

@swirtSJW I was trying to figure out how to apply this to the core taxonomy term name field, which isn't managed in the UI. Any ideas?

Update: I need to prevent the term name from being duplicated. I am not understanding what this service is doing on the VA Services end. You are still able to create duplicate VA Services.

I don't want to add another module if we don't need it so is it possible to use the existing pattern in this way?

This should be covered by image

I am pretty sure there is logic in that to handle name vs title even if the UI does not make that clear. If not it would be an easy patch.