Skip to content

Add adoptions - #1018

Open
LeviCameron1 wants to merge 21 commits into
release26.3-SNAPSHOTfrom
26.3_fb_adoptions
Open

Add adoptions#1018
LeviCameron1 wants to merge 21 commits into
release26.3-SNAPSHOTfrom
26.3_fb_adoptions

Conversation

@LeviCameron1

Copy link
Copy Markdown
Collaborator

Rationale

This table and form will be used later by the new housing to determine condition codes.

Related Pull Requests

Changes

  • Added data entry for adoptions
  • Permissions for adoptions

@aschmidt34 aschmidt34 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two breaking issues, fix before merging:

  1. adoptions.query.xml‎ - Dam/Sire mismatch.
  2. AdoptionForm.tsx‎ - Dam/Sire/Result non-optional declaration.

Listed a few other minor comments, otherwise this is fine to deploy when you're ready.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if you need it but dateColumnType is never exported. dateTimeColumnType has 'export const' but this is missing the 'export'. Only an issue if other files need to use this.

Also, for this line: export const dateTimeColumnType: GridColTypeDef<Date, string> = {
Native JS type 'Date' has no .toDate() call. I see you're using the Dayjs, though, so this comes down to how strict TS checks Dayjs against GridColTypeDef, just make sure this won't throw an error.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the line: catch (QueryUpdateServiceException | BatchValidationException | DuplicateKeyException | RuntimeException | SQLException e)

Java requires none of the listed exceptions be subclasses of eachother in the same catch block. This should be fine as long as you verify none of these contain eachother.

Comment thread CageUI/package.json

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Risk here: "@mui/x-data-grid": "^8.28.6",

Apparently this only supports @mui/material 5, 6, or 7 but this project pins @ 9.

Fine to leave this, just know this is unsupported and we'll have to keep an eye on this to verify it doesn't break on any future updates.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Real issue: You accidentally used ${dam} instead of ${sire} for your 'sire' column. Please fix this before merging (unless this was intentional due to the dam/sire mixup issues we've seen lately).

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In handleAddAnimal here, you declare dam, sire, and result as null; however in the adoptionFormTypes.ts file in this PR, you listed dam, sire, and result as non-optional. Please update adoptionFormTypes.ts to support this.

Also, your catch block here handles errors correctly if the server responds normally with an error (it returns err.errors) but if the network request itself fails from a server being down or a timeout, this rejects with a plain error object (which does not have an .errors list) and will crash. Not a big deal; won't happen often. Feel free to add another clause to the catch if you'd like to handle that.

Comment thread CageUI/package-lock.json

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assuming this file works correctly as i'm not able to test all these nodes.

Ran it through Claude just to double check and the only catch is that it looks like this also deals with the @mui/material issue i mentioned in another comment. This file shows @mui/material resolved to 9.3.1 while @mui/x-data-grid resolved to 8.29.2. x-data-grid's peerDependencies only allow @mui/material 5, 6, and 7. Same as the other comment, feel free to leave this but just keep an eye on any future breakage due to this package combo being unsupported.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants