-
Notifications
You must be signed in to change notification settings - Fork 0
Implement Contribution Service #13
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
base: main
Are you sure you want to change the base?
Conversation
…y service (in future this will be a cron job that will be scheduled to run daily)
|
|
||
| repoFetched, err := s.repositoryService.GetRepoByRepoId(ctx, contribution.RepoID) | ||
| repositoryId := repoFetched.Id | ||
| if err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error must be checked before the assignment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay, I can put it in else then
| } | ||
| } | ||
|
|
||
| func (s *service) ProcessFetchedContributions(ctx context.Context) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method is very big please check if we can we refactor this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay
| "github.com/joshsoftware/code-curiosity-2025/internal/pkg/apperrors" | ||
| ) | ||
|
|
||
| type repositoryRepository struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we have a different name for the struct
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We discussed about it, this one works for now as the standard naming kept for db layer is repository and we have a repository table in our system as well. If needed then I can change the repository folder name to dbstore or something else. This will initiate changes in all other parts as well. Let me know if I should.
internal/app/repository/service.go
Outdated
|
|
||
| req.Header.Add("Authorization", s.appCfg.GithubPersonalAccessToken) | ||
|
|
||
| client := &http.Client{} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggesstion -> Pass this in the service struct so that we can reuse it rather than creating a new client everytime
…andling while fetching repositoryDetails
…ndled in frontend
internal/app/contribution/service.go
Outdated
| if err := contributions.Next(&contribution); err == iterator.Done { | ||
| break | ||
| } else if err != nil { | ||
| slog.Error("error iterating contribution rows", "error", err) | ||
| break | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
error is being not returned here, try to refactor this as:
if err != nil {
if err == iterator.Done {
break
}
slog.Error("error iterating contribution rows", "error", err)
return err
}
|
|
||
| var repositoryId int | ||
| repoFetched, err := s.repositoryService.GetRepoByRepoId(ctx, contribution.RepoID) //err no rows | ||
| if err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Explicitly check for apperrors.ErrRepoNotFound
internal/app/contribution/service.go
Outdated
| repo, err := s.repositoryService.FetchRepositoryDetails(ctx, contribution.RepoUrl) | ||
| if err != nil { | ||
| slog.Error("error fetching repository details") | ||
| return err | ||
| } | ||
|
|
||
| repositoryCreated, err := s.repositoryService.CreateRepository(ctx, contribution.RepoID, repo) | ||
| if err != nil { | ||
| slog.Error("error creating repository", "error", err) | ||
| return err | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
move the remove creation logic to another function, to reduce cognitive complexity
| var action string | ||
| if actionVal, ok := contributionPayload["action"]; ok { | ||
| action = actionVal.(string) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it ok if action is empty, do we need to handle it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
action value cannot be empty, according to the events used all those have action as a key in their payload.
internal/app/contribution/service.go
Outdated
| var contributionType string | ||
| switch contribution.Type { | ||
| case "PullRequestEvent": | ||
| if action == "closed" && isMerged { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
create constants for strings
internal/app/repository/domain.go
Outdated
|
|
||
| import "time" | ||
|
|
||
| type RepoOWner struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit- typo
| resp, err := s.httpClient.Do(req) | ||
| if err != nil { | ||
| slog.Error("error fetching user repositories details", "error", err) | ||
| return FetchRepositoryDetailsResponse{}, err | ||
| } | ||
|
|
||
| body, err := io.ReadAll(resp.Body) | ||
| if err != nil { | ||
| slog.Error("error freading body", "error", err) | ||
| return FetchRepositoryDetailsResponse{}, err | ||
| } | ||
|
|
||
| var repoDetails FetchRepositoryDetailsResponse | ||
| err = json.Unmarshal(body, &repoDetails) | ||
| if err != nil { | ||
| slog.Error("error unmarshalling fetch repository details body", "error", err) | ||
| return FetchRepositoryDetailsResponse{}, err | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Create a utils function to do network calls, eg: DoPost, DoGet and so on
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay. Also I want to move the FetchRepositoryDetails and other functions which are calls to github API's in a separate service named github. So there I can call the utility. Is that fine?
internal/app/bigquery/service.go
Outdated
| YesterdayDate := time.Now().AddDate(0, 0, -1) | ||
| YesterdayYearMonthDay := YesterdayDate.Format("20060102") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
define variables close to where they are used
internal/app/bigquery/service.go
Outdated
| fetchDailyContributionsQuery := fmt.Sprintf(` | ||
| SELECT | ||
| id, | ||
| type, | ||
| public, | ||
| actor.id AS actor_id, | ||
| actor.login AS actor_login, | ||
| actor.gravatar_id AS actor_gravatar_id, | ||
| actor.url AS actor_url, | ||
| actor.avatar_url AS actor_avatar_url, | ||
| repo.id AS repo_id, | ||
| repo.name AS repo_name, | ||
| repo.url AS repo_url, | ||
| payload, | ||
| created_at, | ||
| other | ||
| FROM | ||
| githubarchive.day.%s | ||
| WHERE | ||
| type IN ( | ||
| 'IssuesEvent', | ||
| 'PullRequestEvent', | ||
| 'PullRequestReviewEvent', | ||
| 'IssueCommentEvent', | ||
| 'PullRequestReviewCommentEvent' | ||
| ) | ||
| AND ( | ||
| actor.login IN (%s) | ||
| ) | ||
| `, YesterdayYearMonthDay, githubUsernames) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
define the query in a constant or another file
| executer := cr.BaseRepository.initiateQueryExecuter(tx) | ||
|
|
||
| var contribution Contribution | ||
| err := executer.QueryRowContext(ctx, createContributionQuery, | ||
| contributionInfo.UserId, | ||
| contributionInfo.RepositoryId, | ||
| contributionInfo.ContributionScoreId, | ||
| contributionInfo.ContributionType, | ||
| contributionInfo.BalanceChange, | ||
| contributionInfo.ContributedAt, | ||
| ).Scan( | ||
| &contribution.Id, | ||
| &contribution.UserId, | ||
| &contribution.RepositoryId, | ||
| &contribution.ContributionScoreId, | ||
| &contribution.ContributionType, | ||
| &contribution.BalanceChange, | ||
| &contribution.ContributedAt, | ||
| &contribution.CreatedAt, | ||
| &contribution.UpdatedAt, | ||
| ) | ||
| if err != nil { | ||
| slog.Error("error occured while inserting contributions", "error", err) | ||
| return Contribution{}, apperrors.ErrContributionCreationFailed | ||
| } | ||
|
|
||
| return contribution, err | ||
| } | ||
|
|
||
| func (cr *contributionRepository) GetContributionScoreDetailsByContributionType(ctx context.Context, tx *sqlx.Tx, contributionType string) (ContributionScore, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to handle no row error explicitly in this function
Issue resolved #3
If it does not exist:
Create a new entry in the repositories table
Retrieve the newly created repository ID