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

[EASI-4457] TRB Advice letter dataloader #2702

Merged
merged 11 commits into from
Jul 16, 2024
Merged

[EASI-4457] TRB Advice letter dataloader #2702

merged 11 commits into from
Jul 16, 2024

Conversation

mynar7
Copy link
Contributor

@mynar7 mynar7 commented Jul 16, 2024

EASI-4457

Description

  • Adds a dataloader for advice letters for TRB Reqs.
  • Refactors status resolver to fully use dataloaders.
  • Modifies some seed data and test code to accommodate the new dataloaders.

How to test this change

  • Pull the adviceLetter and status fields on TRB Requests or run the automated test suite.

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.

@mynar7 mynar7 requested a review from a team as a code owner July 16, 2024 14:35
@mynar7 mynar7 requested review from samoddball and removed request for a team July 16, 2024 14:35
samoddball
samoddball approved these changes Jul 16, 2024
View reviewed changes
Copy link
Contributor

@samoddball samoddball left a comment

Choose a reason for hiding this comment

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

left a question, but otherwise looks great! love the removal of the *store arg everywhere!

pkg/graph/resolvers/trb_request_status.go Show resolved Hide resolved
pkg/graph/resolvers/trb_request_status.go
if err != nil {
return nil, err
}
if adviceLetterStatus == nil {
Copy link
Contributor

@samoddball samoddball Jul 16, 2024 •
edited

Choose a reason for hiding this comment

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

should we nil-check all of these? i see they are all dereferenced below

also a curiosity - why remove the goroutines?

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 was thinking that opening a zillion channels is probably worse than just writing it synchronously and letting the dataloaders return cached data, but I might be wrong there. @ClayBenson94 do you have any thoughts?

I missed the dereference! I will add checks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nil checks added here: ed83b18

Copy link
Collaborator

Choose a reason for hiding this comment

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

If each synchronous call is behind a dataloader it probably boils down to readability more than anything, since each of those calls will probably already be cached for this request

pkg/storage/trb_request_feedback.go Show resolved Hide resolved
@mynar7 mynar7 requested a review from samoddball July 16, 2024 18:21
@mynar7 mynar7 merged commit 08b3419 into feature/EASI-4457 Jul 16, 2024
12 checks passed
@mynar7 mynar7 deleted the EASI-4457/advice-letter-dataloader branch July 16, 2024 19:27
mynar7 added a commit that referenced this pull request Aug 1, 2024
@mynar7 @samoddball
[EASI-4457] Dataloaders Refactor (#2727)
bedaf2d 
* [EASI-4457] Funding sources dataloader and mapper refactor (#2692)

* add funding source dataloader and refactor helpers.OneToMany and Mapper interface

* fix store method, call dataloader version in resolver

* [EASI-4457] Add dataloader to TRB request form resolver (#2695)

add dataloader to trb request form resolver

* [EASI-4457] System intake notes dataloader (#2693)

update notes resolver to use dataloader, add tests for notes

* [EASI-4457] System Intake Actions dataloader (#2694)

add dataloader to system intake actions resolver

* [EASI 4457] - TRB Request Feedback dataloaders (#2700)

* add dataloaders to TRB Feedback

* convert slice to postgres array

* use dataloaders in status calls

* remove unnecessary no rows error check

* [EASI-4457] TRB Advice letter dataloader (#2702)

* add dataloaders to TRB Feedback

* convert slice to postgres array

* use dataloaders in status calls

* remove unnecessary no rows error check

* add dataloaders to advice letter and finish refactoring statuses

* sort mock users

* nil check statuses

* [EASI-4457] TRB attendees dataloaders (#2705)

* add dataloader to trb attendees

* add dataloader for attendees by EUA and TRB ID, update requester component resolver

* use ptr helper, add to resolver test

* [EASI-4457] Add closed requests for testing (#2709)

* refactor seed data to allow for closed request generation

* remove context value and just use a constant

* add regex to LCID test

* [EASI-4457] TRB documents dataloaders (#2706)

* add a dataloader for trb documents, use dataloaders in other resolvers

* semicolons and better error handling

* [EASI-4457] TRB Admin Notes dataloader (#2714)

add dataloaders for TRB admin notes

* [EASI-4457] TRB Request Funding Sources dataloader (#2715)

* add dataloader for trb funding sources

* fix test to allow for empty slice

* add TODO comment

* [EASI-4457] TRB Form System Intakes dataloader (#2722)

add dataloader for trb request form system intakes

* [EASI-4457] Add gov req feedback dataloader (#2731)

* add gov req feedback dataloader

* run gql gen

* [EASI-4457] intake documents dataloader (#2732)

* add dataloader for documents

* use resolver to create fake documents

* update seed script

* update seed script to not fetch users multiple times for closed requests

* add comments

* [EASI-4457] Business Case dataloaders (#2729)

* refactor business case legacy code to allow for biz case and cost line dataloaders with less friction

* cleanup comments

---------

Co-authored-by: samoddball <156127704+samoddball@users.noreply.github.com>

---------

Co-authored-by: samoddball <156127704+samoddball@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ClayBenson94 ClayBenson94 ClayBenson94 left review comments

@samoddball samoddball samoddball approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@mynar7 @ClayBenson94 @samoddball