Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Types Cleanup #11719

Merged
merged 5 commits into from
Jun 5, 2024
Merged

Types Cleanup #11719

merged 5 commits into from
Jun 5, 2024

Conversation

karla-vm
Copy link
Collaborator

@karla-vm karla-vm commented Jun 3, 2024

Description

Cleaning up the types across ui-src and app-api for better understanding in the future.

Related ticket(s)

N/A


How to test

There should be no functional changes in this PR.

Notes

N/A


Pre-review checklist

  • I have added thorough tests, if necessary
  • I have updated relevant documentation, if necessary
  • I have performed a self-review of my code
  • I have manually tested this PR in the deployed cloud environment

Pre-merge checklist

Review

  • Design: This work has been reviewed and approved by design, if necessary
  • Product: This work has been reviewed and approved by product owner, if necessary

Security

If either of the following are true, notify the team's ISSO (Information System Security Officer).

  • These changes are significant enough to require an update to the SIA.
  • These changes are significant enough to require a penetration test.

convert to a different template: test → val | val → prod

services/ui-src/src/types/other.ts Outdated Show resolved Hide resolved
services/app-api/utils/types/reportContext.ts Show resolved Hide resolved
services/ui-src/src/types/other.ts Outdated Show resolved Hide resolved
services/ui-src/src/types/reports.ts Show resolved Hide resolved
services/app-api/utils/types/reportContext.ts Show resolved Hide resolved
services/app-api/utils/types/reports.ts Show resolved Hide resolved
services/ui-src/src/types/formFields.ts Show resolved Hide resolved
services/app-api/utils/types/formFields.ts Outdated Show resolved Hide resolved
services/app-api/utils/types/reportContext.ts Show resolved Hide resolved
services/app-api/utils/types/other.ts Show resolved Hide resolved
Copy link
Contributor

@gmrabian gmrabian left a comment

Choose a reason for hiding this comment

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

I know some of these are unused and may be used in the future, but just want to be careful about what we assume is important to both sides. ex: I think ReportMetadata should absolutely be in both and should match. That is an essential part of the fe/be contract. S3EventRecordGlacierRestoreEventData though? If the FE ever needs that one we are doing something wrong.

Do we have a plan for the next phase? Is there a way to tighten up the ones we need in both and the ones we don't?

services/app-api/utils/types/formFields.ts Outdated Show resolved Hide resolved
services/app-api/utils/types/formFields.ts Outdated Show resolved Hide resolved
services/app-api/utils/types/formFields.ts Outdated Show resolved Hide resolved
services/app-api/utils/types/formFields.ts Outdated Show resolved Hide resolved
services/app-api/utils/types/formFields.ts Outdated Show resolved Hide resolved
services/ui-src/src/types/other.ts Outdated Show resolved Hide resolved
services/ui-src/src/types/other.ts Outdated Show resolved Hide resolved
services/ui-src/src/types/other.ts Outdated Show resolved Hide resolved
Copy link

codeclimate bot commented Jun 4, 2024

Code Climate has analyzed commit b34e0cc and detected 31 issues on this pull request.

Here's the issue category breakdown:

Category Count
Duplication 31

The test coverage on the diff in this pull request is 100.0% (90% is the threshold).

This pull request will bring the total coverage in the repository to 95.9% (0.1% change).

View more on Code Climate.

@karla-vm karla-vm merged commit 9499ebc into main Jun 5, 2024
19 checks passed
@karla-vm karla-vm deleted the types-restructure branch June 5, 2024 14:46
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