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.

Sign up for GitHub

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

Jump to bottom

Include constraintID #1249

Merged
merged 3 commits into from
Dec 5, 2023
Merged

Include constraintID #1249

merged 3 commits into from
Dec 5, 2023

Conversation

goetzrrGit
Copy link
Contributor

Description

The UI needs constraintId to be able to know which constraint has violations, compile errors, or no violations within the Constraint View.

Verification

Updated the e2e test

@goetzrrGit goetzrrGit added refactor A code change that neither fixes a bug nor adds a feature constraints Anything related to the constraints domain labels Nov 27, 2023
@goetzrrGit goetzrrGit requested a review from a team as a code owner November 27, 2023 21:47
@dandelany dandelany mentioned this pull request Nov 28, 2023
Closed
@goetzrrGit goetzrrGit force-pushed the include_constraint_id branch from a66518c to 707d94f Compare November 29, 2023 21:24
@Mythicaeda Mythicaeda self-requested a review November 30, 2023 18:16
@Mythicaeda Mythicaeda added the breaking change A change that will require updating downstream code label Nov 30, 2023
Mythicaeda
Mythicaeda reviewed Nov 30, 2023
View reviewed changes
Copy link
Contributor

@Mythicaeda Mythicaeda left a comment

Choose a reason for hiding this comment

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

Looks good to me! A couple of non-blocking things. I'll approve once the type comment has been resolved.

e2e-tests/src/test/java/gov/nasa/jpl/aerie/e2e/ConstraintsTests.java Outdated Show resolved Hide resolved
e2e-tests/src/test/java/gov/nasa/jpl/aerie/e2e/types/ConstraintResult.java Outdated Show resolved Hide resolved
merlin-server/src/main/java/gov/nasa/jpl/aerie/merlin/server/http/ResponseSerializers.java Outdated Show resolved Hide resolved
deployment/hasura/metadata/actions.graphql Outdated Show resolved Hide resolved
@goetzrrGit
Add a constraintId and constraintName for each checked constraint.
3a74dd1 
This is valuable information for associating each response with its corresponding constraint, especially in cases where a compile error might not capture the constraint ID.
@goetzrrGit
Replace constraintID with the full Constraint object and pass it to t…
aa13ff1 
…he ResponseSerializer.
@goetzrrGit
Updated the e2e tests
b3c1940
@goetzrrGit goetzrrGit force-pushed the include_constraint_id branch from 707d94f to b3c1940 Compare December 5, 2023 21:12
@goetzrrGit
Copy link
Contributor Author

@Mythicaeda Thank you for reviewing. I made the requested changes and updated the e2e test to also check for type

Mythicaeda
Mythicaeda approved these changes Dec 5, 2023
View reviewed changes
Copy link
Contributor

@Mythicaeda Mythicaeda left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks!

@goetzrrGit goetzrrGit mentioned this pull request Dec 5, 2023
Merged
@goetzrrGit goetzrrGit merged commit 2dff316 into develop Dec 5, 2023
6 checks passed
@goetzrrGit goetzrrGit deleted the include_constraint_id branch December 5, 2023 21:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Mythicaeda Mythicaeda Mythicaeda approved these changes

@cohansen cohansen Awaiting requested review from cohansen cohansen is a code owner automatically assigned from NASA-AMMOS/aerie-backend

@JoelCourtney JoelCourtney Awaiting requested review from JoelCourtney JoelCourtney is a code owner automatically assigned from NASA-AMMOS/aerie-backend

Assignees
No one assigned
Labels
breaking change A change that will require updating downstream code constraints Anything related to the constraints domain refactor A code change that neither fixes a bug nor adds a feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@goetzrrGit @Mythicaeda