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

Easi 4470/dataloaders linked #2674

Merged
merged 15 commits into from
Jul 9, 2024
Merged

Easi 4470/dataloaders linked #2674

merged 15 commits into from
Jul 9, 2024

Conversation

samoddball
Copy link
Contributor

EASI-4470

Description

  • Hook up linked TRB Requests and linked System Intakes to dataloaders

How to test this change

  • pull branch and create new system intake locally
  • link it to existing systems, however many you want
  • open postman, and navigate to Cedar/Core/System Summary/Get System
  • use the id of the cedar system you linked the system intake to
  • confirm the response contains the system intake id in the linkedSystemIntakes field
  • you can create any number of TRB requests and system intakes and add the cedar system as the system
  • resend the postman request and confirm the ids all show up where they should under linkedSystemIntakes and linkedTrbRequests

PR Author Checklist

  • I have provided a detailed description of the changes in this PR.
  • I have provided clear instructions on how to test the changes in this PR.
  • I have updated tests or written new tests as appropriate in this PR.

PR Reviewer Guidelines

  • It's best to pull the branch locally and test it, rather than just looking at the code online!
  • When approving a PR, provide a reason why you're approving it
    • e.g. "Approving because I tested it locally and all functionality works as expected"
    • e.g. "Approving because the change is simple and matches the Figma design"
  • Don't be afraid to leave comments or ask questions, especially if you don't understand why something was done! (This is often a great time to suggest code comments or documentation updates)
  • Check that all code is adequately covered by tests - if it isn't feel free to suggest the addition of tests.

@samoddball samoddball requested a review from a team as a code owner July 1, 2024 23:09
@samoddball samoddball requested review from ClayBenson94 and removed request for a team July 1, 2024 23:09
@ClayBenson94 ClayBenson94 requested review from mynar7 and removed request for ClayBenson94 July 8, 2024 17:36
Comment on lines +150 to +163
err = sqlutils.WithTransaction(ctx, s.testConfigs.Store, func(tx *sqlx.Tx) error {
return s.testConfigs.Store.SetSystemIntakeSystems(ctx, tx, open1, systemNumbers)
})
s.NoError(err)

err = sqlutils.WithTransaction(ctx, s.testConfigs.Store, func(tx *sqlx.Tx) error {
return s.testConfigs.Store.SetSystemIntakeSystems(ctx, tx, open2, systemNumbers)
})
s.NoError(err)

err = sqlutils.WithTransaction(ctx, s.testConfigs.Store, func(tx *sqlx.Tx) error {
return s.testConfigs.Store.SetSystemIntakeSystems(ctx, tx, closed, systemNumbers)
})
s.NoError(err)
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick, but these could all be in the same WithTransaction call, right?

Suggested change
err = sqlutils.WithTransaction(ctx, s.testConfigs.Store, func(tx *sqlx.Tx) error {
return s.testConfigs.Store.SetSystemIntakeSystems(ctx, tx, open1, systemNumbers)
})
s.NoError(err)
err = sqlutils.WithTransaction(ctx, s.testConfigs.Store, func(tx *sqlx.Tx) error {
return s.testConfigs.Store.SetSystemIntakeSystems(ctx, tx, open2, systemNumbers)
})
s.NoError(err)
err = sqlutils.WithTransaction(ctx, s.testConfigs.Store, func(tx *sqlx.Tx) error {
return s.testConfigs.Store.SetSystemIntakeSystems(ctx, tx, closed, systemNumbers)
})
s.NoError(err)
err = sqlutils.WithTransaction(ctx, s.testConfigs.Store, func(tx *sqlx.Tx) error {
if err = s.testConfigs.Store.SetSystemIntakeSystems(ctx, tx, open1, systemNumbers); err != nil {
return err
}
if err = s.testConfigs.Store.SetSystemIntakeSystems(ctx, tx, open2, systemNumbers); err != nil {
return err
}
if err = s.testConfigs.Store.SetSystemIntakeSystems(ctx, tx, closed, systemNumbers); err != nil {
return err
}
return nil
})
s.NoError(err)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

they could be sure, but i wanted to keep it separate for tests (also easier to know which err occurred, for example)

Comment on lines +106 to +110
intake1 := models.SystemIntake{
EUAUserID: testhelpers.RandomEUAIDNull(),
RequestType: models.SystemIntakeRequestTypeNEW,
State: models.SystemIntakeStateOpen,
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This code is my fault, but there's a helper you can use to shorten this to intake1 := s.createNewIntake()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i need to create a closed one though

Comment on lines +98 to +108
trb1 := models.TRBRequest{
Type: models.TRBTBrainstorm,
State: models.TRBRequestStateOpen,
}
trb1.CreatedBy = testhelpers.RandomEUAIDNull().String

create1, err := s.testConfigs.Store.CreateTRBRequest(ctx, s.testConfigs.Store, &trb1)
s.NoError(err)
s.NotNil(create1)

open1 = create1.ID
Copy link
Contributor

Choose a reason for hiding this comment

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

I added a helper for these as well here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

same - i need a closed one

mynar7
mynar7 previously approved these changes Jul 9, 2024
Copy link
Contributor

@mynar7 mynar7 left a comment

Choose a reason for hiding this comment

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

Tested in the Graph Playground and it works!

Copy link
Contributor

@mynar7 mynar7 left a comment

Choose a reason for hiding this comment

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

Update looks good!

@samoddball samoddball merged commit cf19e05 into main Jul 9, 2024
12 checks passed
@samoddball samoddball deleted the EASI-4470/dataloaders_linked branch July 9, 2024 17:56
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.

3 participants