[MINT-3222] Milestone Solution Link #1541

OddTomBrooks · 2024-11-27T18:19:02Z

MINT-3222

Description

Added support for automatic upsertion of solutions when creating a new common milestone
Added support for milestone-solution links

How to test this change

Validate scripts/dev test:go runs successfully
Through Postman create a new common solution including common solution keys and validate that milestone-solution links are created in the mto_mileston_solution_links table corresponding to your inputs

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.
Updated the Postman Collection if necessary.

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.

pkg/sqlqueries/SQL/mto/solution/get_by_model_plan_id_and_common_solution_key_LOADER.sql

pkg/sqlqueries/SQL/mto/milestone/create_milestone_solution_link.sql

pkg/graph/resolvers/mto_milestone.go

…ns_when_adding_common_milestone # Conflicts: # pkg/graph/resolvers/mto_milestone.go

…ns_when_adding_common_milestone

ClayBenson94

Overall this looks good! I think the new UPSERT-style resolver reads cleanly!

There's a few pieces of code I think we can nix, but the main thing to look at is probably adding the JOIN to the create-solution (allowing conflicts) query

pkg/graph/resolvers/mto_milestone.go

pkg/sqlqueries/SQL/mto/solution/create_allow_conflicts.sql

pkg/sqlqueries/mto_milestone.go

pkg/storage/loaders/mto_solution.go

pkg/storage/dataloader_keys.go

StevenWadeOddball

Thanks @OddTomBrooks , I think that this looks good overall! I left a few comments, mostly around todos for future enhancement and minor changes.

I think we can get this to be a bit more efficient, but I think a TODO could be appropriate for the sake of this PR.

pkg/sqlqueries/mto_milestone.go

pkg/sqlqueries/SQL/mto/milestone/create_milestone_solution_link.sql

StevenWadeOddball · 2024-12-03T21:02:09Z

pkg/storage/mto_milestone.go

+
+	arg := map[string]interface{}{"milestone_id": milestoneID}
+
+	returned, procErr := sqlutils.SelectProcedure[models.MTOMilestoneSolutionLink](np, sqlqueries.MTOMilestone.GetMilestoneSolutionLinksByMilestoneID, arg)


I'm not sure that you need to fetch links explicitly here, I think you can validate by fetching linked solutions instead. We don't really need to fetch links, just the solutions / milestones themselves.

You're right. I thought about this quite a bit yesterday. In an ideal world our store would be mocked and we could do flow assertions, but the only way to validate the linking table is functioning as expected was to create a simple fetch. I feel like for unit testing / validation purposes having a fetch method is worthwhile.

sounds good, that works for me! We have a separate ticket for fetching linked solutions, we could use that later if we want, I'm also fine just keeping it.

pkg/storage/dataloader_keys.go

StevenWadeOddball · 2024-12-03T21:04:06Z

pkg/sqlqueries/SQL/mto/milestone/get_milestone_solution_links_by_milestone_id.sql

Related to earlier conversation, I think this might not be needed, instead we can just fetch the actual milestone / solution that is linked.

See above comment for continued discussion, I will apply the resolution here as well

pkg/graph/resolvers/mto_milestone.go

pkg/graph/resolvers/mto_milestone_test.go

StevenWadeOddball · 2024-12-03T21:18:23Z

pkg/graph/resolvers/resolver_test.go

@@ -390,6 +390,24 @@ func (suite *ResolverSuite) createDefaultTestAnalyzedAudit(mp *models.ModelPlan,

 }

+func (suite *ResolverSuite) createMilestoneCommon(


@OddTomBrooks, this is fine here, wanted to see what you thought about moving to the mto_milestone test file instead though? I think it might feel a bit better organizationally, though we aren't doing this now.

I think this is worth a higher level discussion on how we want to organize the logic in our test suite, let's have a chat after standup today

pkg/storage/mto_solution.go

…ns_when_adding_common_milestone # Conflicts: # MINT.postman_collection.json # pkg/graph/generated/generated.go # pkg/graph/schema/types/mto_milestone.graphql

StevenWadeOddball

Thanks for making these changes @OddTomBrooks, it looks good.

I think that git did something weird with some files (eg, it's showing you adding the new categories resolver in GQL). I don't think it matters functionally though

OddTomBrooks · 2024-12-04T16:35:10Z

Thanks for making these changes @OddTomBrooks, it looks good.

I think that git did something weird with some files (eg, it's showing you adding the new categories resolver in GQL). I don't think it matters functionally though

This happened when I merged in from the base branch. I took the incoming logic, not sure why it would be an addition. Maybe the formatting is off or something.

feat: rough draft of mto milestone solution link

70e5469

OddTomBrooks changed the base branch from main to feature/MINT-3175_mto November 27, 2024 18:19

OddTomBrooks commented Nov 27, 2024

View reviewed changes

pkg/sqlqueries/SQL/mto/solution/get_by_model_plan_id_and_common_solution_key_LOADER.sql Outdated Show resolved Hide resolved

OddTomBrooks commented Nov 27, 2024

View reviewed changes

pkg/sqlqueries/SQL/mto/milestone/create_milestone_solution_link.sql Outdated Show resolved Hide resolved

OddTomBrooks commented Nov 27, 2024

View reviewed changes

pkg/graph/resolvers/mto_milestone.go Outdated Show resolved Hide resolved

OddTomBrooks added 6 commits November 27, 2024 17:44

Merge branch 'feature/MINT-3175_mto' into task/MINT-3222_link_solutio…

6cd9d51

…ns_when_adding_common_milestone # Conflicts: # pkg/graph/resolvers/mto_milestone.go

feat: reoriented to use an upsert paradigm

45bd047

wip: unit test for milestone solution link

9edd27b

chore: cleanup and linting

0aec984

Merge branch 'feature/MINT-3175_mto' into task/MINT-3222_link_solutio…

e5858ed

…ns_when_adding_common_milestone

chore: code cleanup and postman export

e00a8aa

OddTomBrooks marked this pull request as ready for review December 3, 2024 16:58

OddTomBrooks requested review from a team as code owners December 3, 2024 16:58

OddTomBrooks requested review from StevenWadeOddball, garyjzhao and patrickseguraoddball and removed request for a team December 3, 2024 16:58

ClayBenson94 requested changes Dec 3, 2024

View reviewed changes

OddTomBrooks added 2 commits December 3, 2024 14:53

chore: cleaned up ghost code and added coalesce to sql

0e56c7f

fix: corrected sql

9d72941

OddTomBrooks requested a review from ClayBenson94 December 3, 2024 20:26

StevenWadeOddball requested changes Dec 3, 2024

View reviewed changes

chore: various PR refinements

20bcc9d

OddTomBrooks requested a review from StevenWadeOddball December 4, 2024 14:40

Merge branch 'feature/MINT-3175_mto' into task/MINT-3222_link_solutio…

b5d7d34

…ns_when_adding_common_milestone # Conflicts: # MINT.postman_collection.json # pkg/graph/generated/generated.go # pkg/graph/schema/types/mto_milestone.graphql

StevenWadeOddball approved these changes Dec 4, 2024

View reviewed changes

ClayBenson94 mentioned this pull request Dec 4, 2024

[NOREF] Initial MTO Milestone resolver tests #1548

Merged

4 tasks

ClayBenson94 approved these changes Dec 4, 2024

View reviewed changes

OddTomBrooks merged commit 9d8e048 into feature/MINT-3175_mto Dec 4, 2024
10 checks passed

OddTomBrooks deleted the task/MINT-3222_link_solutions_when_adding_common_milestone branch December 4, 2024 16:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MINT-3222] Milestone Solution Link #1541

[MINT-3222] Milestone Solution Link #1541

OddTomBrooks commented Nov 27, 2024 •

edited

Loading

ClayBenson94 left a comment

StevenWadeOddball left a comment

StevenWadeOddball Dec 3, 2024

OddTomBrooks Dec 3, 2024

StevenWadeOddball Dec 4, 2024

StevenWadeOddball Dec 3, 2024

OddTomBrooks Dec 3, 2024

StevenWadeOddball Dec 3, 2024

OddTomBrooks Dec 4, 2024

StevenWadeOddball left a comment

OddTomBrooks commented Dec 4, 2024


		arg := map[string]interface{}{"milestone_id": milestoneID}

		returned, procErr := sqlutils.SelectProcedure[models.MTOMilestoneSolutionLink](np, sqlqueries.MTOMilestone.GetMilestoneSolutionLinksByMilestoneID, arg)

		@@ -390,6 +390,24 @@ func (suite ResolverSuite) createDefaultTestAnalyzedAudit(mp models.ModelPlan,

		}

		func (suite *ResolverSuite) createMilestoneCommon(

[MINT-3222] Milestone Solution Link #1541

[MINT-3222] Milestone Solution Link #1541

Conversation

OddTomBrooks commented Nov 27, 2024 • edited Loading

Description

How to test this change

PR Author Checklist

PR Reviewer Guidelines

ClayBenson94 left a comment

Choose a reason for hiding this comment

StevenWadeOddball left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StevenWadeOddball left a comment

Choose a reason for hiding this comment

OddTomBrooks commented Dec 4, 2024

OddTomBrooks commented Nov 27, 2024 •

edited

Loading