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
97 stars 69 forks source link

RCA: VA forms are incorrectly showing up in VBA and VACO sections #9469

Closed kevwalsh closed 1 year ago

kevwalsh commented 2 years ago

Describe the defect

Certain Section listings are showing certain content that they shouldn't.

Content Manager views built based on Section permissions are showing content with a different Section. e.g. VBA section view is showing Forms content with the Octo Section.

User story

As an editor I want to see only content that I can edit, in my editor views so that I don't mess with things I can't / shouldn't touch.

To Reproduce

Steps to reproduce the behavior:

  1. Go to VBA
  2. See Forms listed
  3. Go to VACO
  4. See Forms listed

For a specific example form:

  1. Go to VBA, Filter by Content Type = VA Form
  2. Click through to a listed Form, e.g. https://prod.cms.va.gov/node/5714/edit
  3. Under Section, see Section is OCTO > Sitewide > Forms

Screenshots

Forms appear in VBA section

VBA___VA_gov_CMS

Section listed on example form

Screen Shot 2023-02-16 at 11 53 18 AM

Additional context

Not sure if this Forms specific, or a canary for something else that's going on.

When we import from Forms DB, we receive/set a "Form Administration" field that corresponds to the area where these forms are appearing for VBA / VACO. That could be implicated somehow.

ACs

CMS Team

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

msbtterswrth commented 2 years ago

I found this related d.o issue. Doesn't seem to be a d8 patch for this issue yet.

swirtSJW commented 2 years ago

Raised this with module maintainers here https://civicactions.slack.com/archives/C0ASJ7C8P/p1655313649513379

swirtSJW commented 2 years ago

Sadly they are both foggy on the issue. So we will have to contribute a patch.

EWashb commented 1 year ago

@jilladams PW has forms, right? I would like to remove our label and add to your backlog

jilladams commented 1 year ago

Good call, updated - fyi @wesrowe . This was previously refined and points added, but PW should review scope and confirm estimate. Moving back to "Needs refining."

wesrowe commented 1 year ago

Assigned to @dsasser for pre-refinement review.

jilladams commented 1 year ago

Suspicion: The forms that are appearing incorrectly in these views have that Administration assigned as the Form Administration, from the Forms DB CSV import. I think this may be implicated.

Example:

Backend Refinement:

Keeping the 5 estimate (more than a day, less than a week), and can timebox the effort to root cause.

Pulling into sprint for @chri5tia since she has capacity. Goal is to get to root cause and then double check if 5 sufficient for the fix as well.

jilladams commented 1 year ago

Ticket was added to sprint opportunistically. However, #12731 is a new ticket with higher priority re: quarterly goals, so: ejecting this, pulling that one in instead.

wesrowe commented 1 year ago

@jilladams, we're pulling this into sprint 79 opportunistically.

dsasser commented 1 year ago

Root cause analysis:

VA Forms have two "administration" fields:

  1. "Form Administration" a. Machine name: field_va_form_administration b. Field type: entity_reference. c. Referencing "Section" taxonomy terms (parents only, using entity reference selection View administration_section)
  2. "Section" a. Machine name: field_administration b. Field type: entity_reference c. Referencing "Section" taxonomy terms. Statically set to term id 196 (OCTO -> Sitewide content team -> Forms) for all forms

What this reveals is that VA Forms content has two entity reference fields which map to the same Vocabulary: Sections.

In the View controlling the display of section content (eg: /section/vba), a contextual filter (an argument to the query) is being added which joins data from the taxonomy_entity_index database table. This table collects mappings from entities (nodes in this case) to taxonomy terms. Because we are mapping two fields to the same Vocabulary in VA Forms, there will always be two entries for each of the form nodes:

Screenshot 2023-03-10 at 10 00 54 AM

Due to the query settings for the View (distinct, and aggregated), only one result is displayed for each node, which has a side-effect of masking the underlying problem.

Possible solutions (in no particular order)

1. Don't use an entity_reference field for "Form Administration" - XL (5-8ish points, more fragile, FE work, VA Forms query would need work)

This solution would essentially be to replace the existing 'field_va_form_administration' field, which is an entity reference field, with some other type of field that doesn't reference the same Section vocabulary as the 'field_administration' field.

One type of field we could use is a 'List (select)' field. In this scenario, the select list would be populated with a copy of the root level Section terms. Because it isn't an entity reference field, the taxonomy tables used in the View would not return two instances for the same node.

Side effects with this approach:

2. Views query alter - M (3ish points)

Performing a Views query alter to limit results using a field name. This would alter the query by filtering on the 'taxonomy_entity_index.field_name' column, limiting results to results with the fieldname 'field_administration' (or excluding fields with the name 'field_va_form_administration'.

Side effects with this approach:

3. Views filter - Large (5ish points, but: less tech debt)

Ideally, we could filter out certain fields from the section listings by way of configuration in Views. This would require one of several possible solutions, including either adding a field filter (no such filter is provided by the taxonomy_entity_index module), enhancing the existing argument code to do such filtering (probably not recommended as this would also hide the filtering logic)

Side effects with this approach:

We can then upstream these changes to the module, for the community.

jilladams commented 1 year ago

Backend refinement:

So: Option 3 wins. This ticketed converted to an RCA issue,and we'll file a new ticket for the fix, due to sprint boundary.