🐛 fix(autocomplete): tags not generated for preselected options

[gitdallas]

closes jira 1315

[codecov]

Codecov Report

Attention: 20 lines in your changes are missing coverage. Please review.

Comparison is base (e269b74) 41.05% compared to head (7a832ca) 41.01%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1403      +/-   ##
==========================================
- Coverage   41.05%   41.01%   -0.05%     
==========================================
  Files         137      137              
  Lines        4338     4333       -5     
  Branches     1043     1045       +2     
==========================================
- Hits         1781     1777       -4     
  Misses       2469     2469              
+ Partials       88       87       -1     

Flag	Coverage Δ
client	41.01% <25.92%> (-0.05%)	⬇️
server	∅ <ø> (∅)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
...s/components/application-form/application-form.tsx	59.60% <35.71%> (ø)
client/src/app/components/Autocomplete.tsx	31.85% <15.38%> (-1.72%)	⬇️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[ibolton336]

Just noticed that the app can have multiple tags with the same name from different sources. This is causing duplicate tags to render in the UI

Wondering if this enhancement would fall under #1321 or if we would need to be able to display tag by category AND source in this dropdown.

[gitdallas]

@ibolton336 - how would this work in practice? should there be tags with the same id/name and a different source? how would the labels/tags look in this case?

[ibolton336]

@ibolton336 - how would this work in practice? should there be tags with the same id/name and a different source? how would the labels/tags look in this case?

@JustinXHale

[ibolton336]

@gitdallas I am thinking that any tag that is not of source type "" (empty string aka "manual") should be hidden from this view. Maybe there is a read only field that might display tags associated to an app by archetype / assessment / analysis. IIRC this screen only lets you select "manual" source tags so in turn I think we should only be displaying "manual" source tags as selectable/deselectable.

[gitdallas]

@ibolton336 - the tags value in application-form doesn't seem to have a source type in my local environment:

but i added a filter on them so anything with a truthy tag.source value will be excluded

[ibolton336]

Sounds good @gitdallas - Looks like we have a "source" optional property on the TagRef so at least we are covered there. The Tags from the tags api won't have a source since they aren't associated with an app/archetype yet.

[ibolton336]

Suggested change

	const selections = value
	.map(
	(formTag) =>
	tags?.find((tagRef) => tagRef.name === formTag.name)
	)
	.map((matchingTag) =>
	matchingTag ? matchingTag.name : undefined
	)
	.filter((e) => e !== undefined) as string[];
	const tagNames = new Set(tags?.map((tag) => tag.name));
	const selections = value.flatMap((formTag) =>
	formTag.source === "" && tagNames.has(formTag.name)
	? [formTag.name]
	: []
	);

[ibolton336]

Played around with this a bit and borrowed Mikes suggestion of using a Set/has here instead of creating a map and using includes since set narrows the tags to unique values & boosts performance. Explicitly checking for the empty string source on the form tag on lookup should give us what we want since empty string source here means "manually selected tag" for a tag on an application object (a formTag in this case) .

[gitdallas]

I ended up making tagOptions a set of strings (tag names) from the beginning and using that. Also as the onChange event maps the form values to tags (which seems to have undefined source instead of ""), I merge a {source:""} object when returning from getTagRef. Without this, after making any changes to the value we'd filter them out if they didn't have a source prop in the tags array.

[ibolton336]

Noticing that onSubmit, any tags that are filtered out from the selections on line 448 are not being added back in to the payload.

[ibolton336]

Actually, looks like even when we patch the object with the "archetype tags" not included, the hub just adds them back anyway. Need to check with Sam on this.

[ibolton336]

Sam confirmed that there is no way for us to overwrite those non-manual tags by updating the application object directly so we should be good on what we are passing to the put request here. There seems to be some ongoing discussion/weirdness around this since the non-manual tags are "virtual" tags being inherited from other membership criteria etc.

We do need to make sure we are filtering out any pre-existing, non-manual tags in the selectable tag list so that we can't accidentally create a new "manual" tag for a tag that already exists on the application from a different source.

[gitdallas]

I'm unsure if there is a change you're asking for or if you are saying to wait.

Right now it is using tags (filtered to only include undefined or "" source value) as the available options and not letting the user add new tags that don't exist outside the options.

[ibolton336]

Understood - My concern was that after we are filtering out the tags on an app with a source other than "", we never add them back in to the tags payload on submit so we'd be overwriting those tags.

[ibolton336]

Right now it is using tags (filtered to only include undefined or "" source value) as the available options and not letting the user add new tags that don't exist outside the options.

The change I think we might temporarily need is to check to see if any tags in the "tagOptions" list you've created match up with any other tags on the application (archetype source or other non-manual tags) and remove any matching available tag from the list. The tags api fetch response won't include any tags with other sources . Those sources are only on tagRefs for tags that have been associated with an application or archetype on their respective api object. We would have to cross check the response from the tags fetch api with what exists on the application in this case.

[ibolton336]

Example:

const filteredTagOptions = new Set(
  [...tagOptions].filter(
    (tagOption) => {
      const matchingAppTag = application.tags.find((appTag) => appTag.name === tagOption);
      if (!matchingAppTag) return true;
      if (matchingAppTag.source === '') return true;
      return false;
    }
  )
);

[sjd78]

Sounds good @gitdallas - Looks like we have a "source" optional property on the TagRef so at least we are covered there. The Tags from the tags api won't have a source since they aren't associated with an app/archetype yet.

I'm looking at this PR today while rebasing #1408 on this change.

A few things are quite confusing.

First, the list of tags generated from the tagCategories are just Tag and not TagRef. The way the tags array is being kept in state via an effect isn't the best approach. The way tagOptions set is generated is confusing, and breaks if you have the tags just be a Tag[]. It's much easier to do this to get a full unique list of tags:

const tags = (tagCategories || []).flatMap((tc) => tc.tags || []);
const tagOptions = new Set(tags.map((t) => t.name));

Second, the Application.tags field is, in fact, a TagRef[]. This means that we need to do something to convert the selected tag names to a TagRef[] for the rest api payload object. Right now looks like the Autocomplete onChange handler is doing the transforms. Maybe moving to use ItemsSelect may be helpful (client/src/app/components/items-select/items-select.tsx). Assuming the update api endpoint will just deal with updating source: "" fields, that should be all we need to send in the payload...

Any further filtering of the set of tags to display etc should probably just be done in the same general area as where tags and tagOptions are handled...

  const applicationManualTags =
    new Set(application?.tags?.filter((t) => t.source === "").map((t) => t.name) ?? []);

  const applicationSourcedTags =
    new Set(application?.tags?.filter((t) => t.source !== "").map((t) => t.name) ?? []);

  const manualTagOptions = new Set(
    tags.map((t) => t.name).filter((name) => !applicationSourcedTags.has(name))
  );

Then drive the select box with manualTagOptions and adjust the payload generation accordingly.

[ibolton336]

Agreed that this could use another pass @sjd78. This issue was blocking QE so wanted to get something in for them so that they can fix their automated tests. You raise some good points.