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

feat: add connection type to goal, query connections when getting goal #1456

Merged
merged 8 commits into from
Jan 17, 2025

Conversation

hvn2k1
Copy link
Contributor

@hvn2k1 hvn2k1 commented Jan 15, 2025

Part of #1417

@hvn2k1 hvn2k1 marked this pull request as ready for review January 15, 2025 10:31
@cre8ivejp
Copy link
Member

@hvn2k1, please fix the conflict.

@hvn2k1
Copy link
Contributor Author

hvn2k1 commented Jan 16, 2025

@hvn2k1, please fix the conflict.

@cre8ivejp I have resolved the conflict

@@ -229,7 +290,7 @@ func (s *experimentService) CreateGoal(
if err := validateCreateGoalRequest(req, localizer); err != nil {
return nil, err
}
goal, err := domain.NewGoal(req.Command.Id, req.Command.Name, req.Command.Description)
goal, err := domain.NewGoal(req.Command.Id, req.Command.Name, req.Command.Description, proto.Goal_UNKNOWN)
Copy link
Member

Choose a reason for hiding this comment

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

How about implementing this proto.Goal_UNKNOWN in the command?

Copy link
Member

@cre8ivejp cre8ivejp Jan 17, 2025

Choose a reason for hiding this comment

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

Instead of using UNKNOWN, we could implement this on the old console when creating the experiment, too.
It doesn't require too many changes.
WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice idea, that would make server side more compatible!

@hvn2k1 hvn2k1 marked this pull request as draft January 17, 2025 03:27
@cre8ivejp
Copy link
Member

@hvn2k1, please add the connection_type to the order by.

case proto.ListGoalsRequest_DEFAULT,
proto.ListGoalsRequest_NAME:
column = "name"
case proto.ListGoalsRequest_CREATED_AT:
column = "created_at"
case proto.ListGoalsRequest_UPDATED_AT:
column = "updated_at"

@hvn2k1
Copy link
Contributor Author

hvn2k1 commented Jan 17, 2025

@hvn2k1, please add the connection_type to the order by.

case proto.ListGoalsRequest_DEFAULT,
proto.ListGoalsRequest_NAME:
column = "name"
case proto.ListGoalsRequest_CREATED_AT:
column = "created_at"
case proto.ListGoalsRequest_UPDATED_AT:
column = "updated_at"

I have supported order by connection type

@hvn2k1 hvn2k1 marked this pull request as ready for review January 17, 2025 07:33
Copy link
Member

@cre8ivejp cre8ivejp left a comment

Choose a reason for hiding this comment

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

Nice work!

@cre8ivejp cre8ivejp merged commit 5d01e7f into main Jan 17, 2025
17 checks passed
@cre8ivejp cre8ivejp deleted the rest-goal branch January 17, 2025 07:42
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