CRDCDH-2189 Compare Differences between Existing/New Data

[amattu2]

Overview

This PR introduces a low-level data comparison between two nodes of the same type, where one was previously submitted, and one is present in an active data submission.

Note

This comparison component utilizes the MUI skeleton to render placeholders while data is loading. This was the best solution I could think of, but it's inconsistent with the rest of the app.

I'm open to alternatives if this doesn't look good.

Change Details (Specifics)

• Create a component that handles a "side-by-side" comparison of two nodes
• Create a "repeater" component that can repeat it's children N-number of times
• Conditionally append the comparison based on the validation error code for "updating existing data" (M018)
• Add code to the ErrorMessage type and query it in QCResults API
• Fix a 508 bad element reference on the Error Details dialog

Related Ticket(s)

CRDCDH-2189 (Task)
CRDCDH-2149 (US)

[coveralls]

Pull Request Test Coverage Report for Build 13203556714

Details

• 49 of 55 (89.09%) changed or added relevant lines in 5 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.2%) to 60.247%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/content/dataSubmissions/QualityControl.tsx	4	10	40.0%

Totals
Change from base Build 13181319649:	0.2%
Covered Lines:	4334
Relevant Lines:	6757

---

💛 - Coveralls

[Alejandro-Vega]

Really great job with this, I personally really like the skeleton look. I left some tiny potential changes. Everything else looks super solid!

[Alejandro-Vega]

Optional, but just to add some readability, can we assign the error code to a variable or add a comment to describe what the error code "M018" is for?

[amattu2]

@Alejandro-Vega I'm thinking we could address this two ways, but I want your thoughts on it:

1. We abstract the error codes (M018 in this case, but more down the line) to a configuration file. e.g.

const ValidationErrorCodes = {
   UPDATING_DATA: "M018",
   // ...
};

2. We convert the ErrorMessage.code from just a string to a constrained string union, and just add comments on the type. e.g.

/**
 * Represents an error code associated with a validation result.
 * 
 * Breakdown:
 * - `M018` - Updating existing data
 */
type ValidationErrorCode = "M018";

type ErrorMessage = {
  // ...
  code: ValidationErrorCode;
  // ...
};

I think both have their pros and cons. If we go the Type direction, it doesn't impact our production bundle sizing (even though it's trivial). But if we go the config direction, it gives us the option to add potentially expand each object in the future (for example, if we needed a FE-dictated tooltip for each error code).

Or if this is an overkill, I can just add a comment as originally commented. Thoughts?

[Alejandro-Vega]

@amattu2 I think both have different benefits. For example, being able to map it to config adds flexibility and easily tells us what it is for. While the constrained string union type doesn't really tell us what any of them are for, but is just useful as a type like in your example.

I suggest we go with a mix of both, but mostly the first option. If we add the as const we can extract the error code values and use them like in your second suggestion. For example:

export const ValidationErrorCodes = {
  UPDATING_DATA: "M018",
  UPDATING_DATA_2: "M019",
  UPDATING_DATA_3: "M020",
} as const;

export type ValidationErrorCode = (typeof ValidationErrorCodes)[keyof typeof ValidationErrorCodes];
// "M018" | "M019" | "M020"

If you think this is overkill, I would still prefer option 1 to provide us with the most scalability.

[Alejandro-Vega]

Perhaps adding a required keyPrefix prop and using it as the key value would be safer here. Duplicate keys could lead to weird issues.

Ex.

{Array.from({ length: count }, (_, index) =>
  React.cloneElement(children, { key: `${keyPrefix}-${index}` })
)}

[Alejandro-Vega]

Super hard to tell while zoomed out, but the white background in this table is overflowing at the corners. Adding a borderRadius here should fix this.