[Cases] Refactor UI user actions

[cnasikas]

Summary

When a user views a single case all user actions are being shown. A user action can be the edit of a title, a new comment, the addition or deletion of a tag, and etc. To visualize the user actions we use the EuiCommentList. The UserActions component is responsible for showing all user actions to the user. This component iterates the user actions and a big if/else is responsible for checking which user action to render. This PR refactors the component to use the factory pattern to render user actions. Each user action has a builder which is responsible for returning a EuiComment. The UserActions component, uses a map and calls the builder of each user action to accumulate all needed user actions.

Related issue: #115730

Checklist

Delete any items that are not applicable to this PR.

• Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
• Unit or functional tests were updated or added to match the most common scenarios
• This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
• This was checked for cross-browser compatibility

For maintainers

• This was checked for breaking API changes and was labeled appropriately

[elasticmachine]

Pinging @elastic/response-ops (Team:ResponseOps)

[elasticmachine]

Pinging @elastic/response-ops-cases (Feature:Cases)

[cnasikas]

@elasticmachine merge upstream

[academo]

I keep seeing this same @ts-ignore comment in many other tests and I am not a big fan of using them when we can avoid them. Could you perhaps create a common test helper builder that handles this and call it in these cases? that way we don't put so many @ts-ignore across the code.

[academo]

My main concern on the current pattern is the typescript types for the builders. Could you create specific types for the builders?

[academo]

could you turn this into a ternary? It makes it easier to read.

[cnasikas]

I like better this pattern, to be honest. With a ternary, I have to return null and I found it more difficult to read.

[academo]

@cnasikas I understand that but keep in mind it is more common for react to have ternaries returning null than having expressions like this. I won't push for the change but I find it better to follow code conventions like that one.

[academo]

I'll go ahead and approve the PR though keep in mind my actual knowledge of the code is limited and I just ran a basic sanity check.

My main concern is the lack of types for the action builders, if you could add those would make the PR better.

[jonathan-buttner]

Nice work, left a few questions

[jonathan-buttner]

I'm not sure of the best practice here but that seems like a lot of parameters haha. If we used classes on the frontend we could group these fields into classes but I'm not sure what the equivalent is. Do you think grouping these by sub objects would be helpful? That way common fields are grouped together.

[cnasikas]

I think in the front-end, or better in the React ecosystem (hooks, etc), the functional paradigm is more appropriate. Any proposal on the grouping of sub-objects?

[jonathan-buttner]

Gotcha, makes sense, hmm maybe grouping the comment stuff together, something like this

export interface UserActionBuilderArgs {
  comments: {
    data: Comment[];
    refs: React.MutableRefObject<
      Record<string, AddCommentRefObject | UserActionMarkdownRefObject | null | undefined>
    >;
    loadingIds: string[];
    selectedOutlineId: string;
    handleOutline: (id: string) => void;
    handleSave: ({ id, version }: { id: string; version: string }, content: string) => void;
  };
  navigation: {
    onShowAlertDetails: (alertId: string, index: string) => void;
    actionsNavigation?: ActionsNavigation;
    getRuleDetailsHref?: RuleDetailsNavigation['href'];
    onRuleDetailsClick?: RuleDetailsNavigation['onClick'];
  };
  alerts: {
    loading: boolean;
    data: Record<string, Ecs>;
  };
  markdown: {
    manageEditIds: string[];
    handleManageEditId: (id: string) => void;
  };
  caseData: Case;
  userAction: CaseUserActions;
  caseServices: CaseServices;
  index: number;
  userCanCrud: boolean;
  handleManageQuote: (quote: string) => void;
}

We could probably pull the definition of each object out into their own interface. This puts the burden of grouping the objects on the caller, so maybe that's not helpful 🤷‍♂️

[jonathan-buttner]

Thanks for the changes! I tested it locally and the user actions look good!

[kibana-ci]

💚 Build Succeeded

• Buildkite Build
• Commit: b983a7a
• Storybooks Preview

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
cases	376	389	+13

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
cases	310.0KB	311.0KB	+1.0KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
cases	81.7KB	81.7KB	-43.0B

Unknown metric groups

ESLint disabled line counts

id	before	after	diff
cases	72	71	-1

Total ESLint disabled count

id	before	after	diff
cases	90	89	-1

History

• 💔 Build #17246 failed c856497
• 💔 Build #17235 failed f0ee443
• 💚 Build #16912 succeeded 0d70da8
• 💛 Build #16810 was flaky 657c847
• 💔 Build #16783 failed a3bd04f

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @cnasikas