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(star): could accept email in /users/{id}/starred #81

Merged
merged 2 commits into from
Feb 23, 2022

Conversation

mabdh
Copy link
Member

@mabdh mabdh commented Feb 18, 2022

No description provided.

@mabdh mabdh requested a review from StewartJingga February 18, 2022 07:53
@mabdh mabdh linked an issue Feb 18, 2022 that may be closed by this pull request
@mabdh mabdh added the enhancement New feature or request label Feb 18, 2022
@mabdh mabdh force-pushed the users-api-accept-email branch from bd11e45 to af67536 Compare February 18, 2022 08:25
@mabdh mabdh requested review from ravisuhag and rahmatrhd February 18, 2022 09:30
star/star.go Outdated
Comment on lines 23 to 24
GetAllAssetsByUserID(ctx context.Context, cfg Config, userID string) ([]asset.Asset, error)
GetAllAssetsByUserEmail(ctx context.Context, cfg Config, userID string) ([]asset.Asset, error)
Copy link
Member

Choose a reason for hiding this comment

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

type filter struct {
    id    string
    email string
}

type Repository interface {
    ...
	GetAllAssets(ctx context.Context, cfg Config, filter filter) ([]asset.Asset, error)
    ...
}

have we looked into this^ approach?

Copy link
Member Author

Choose a reason for hiding this comment

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

These 2 have different sql query,
GetAllAssetsByUserEmail joined 3 tables
GetAllAssetsByUserID joined 2 tables

I am afraid it will just make it more complex

@mabdh mabdh requested a review from rahmatrhd February 23, 2022 09:39
@ravisuhag ravisuhag merged commit 1dc679c into main Feb 23, 2022
@ravisuhag ravisuhag deleted the users-api-accept-email branch February 23, 2022 09:56
@mabdh mabdh self-assigned this Mar 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Accept email in URL param in API GET /users/{email}/stargazers [Temporary]
3 participants