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

Remove GitHub teams #1083

Merged
merged 22 commits into from May 24, 2024
Merged

Remove GitHub teams #1083

merged 22 commits into from May 24, 2024

Conversation

JosteinLindhom
Copy link
Contributor

@JosteinLindhom JosteinLindhom commented May 3, 2024

Closes #1084.

Copy link

codecov bot commented May 7, 2024

Codecov Report

Attention: Patch coverage is 45.00000% with 33 lines in your changes are missing coverage. Please review.

Project coverage is 40.33%. Comparing base (ed8ca81) to head (25e93f5).
Report is 3 commits behind head on master.

Files Patch % Lines
scm/github.go 0.00% 27 Missing ⚠️
scm/mock.go 80.00% 2 Missing ⚠️
web/courses.go 33.33% 2 Missing ⚠️
web/groups.go 83.33% 0 Missing and 1 partial ⚠️
web/scm_helpers.go 91.66% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1083      +/-   ##
==========================================
- Coverage   40.39%   40.33%   -0.06%     
==========================================
  Files         110      110              
  Lines        9019     8896     -123     
==========================================
- Hits         3643     3588      -55     
+ Misses       5041     4975      -66     
+ Partials      335      333       -2     
Flag Coverage Δ
unittests 40.33% <45.00%> (-0.06%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

OrganizationID is not used other than in mock tests and is not necessary otherwise.
Old GroupOptions is no longer needed. Instead renamed TeamOptions to GroupOptions.
@JosteinLindhom JosteinLindhom marked this pull request as ready for review May 13, 2024 11:05
meling and others added 7 commits May 13, 2024 14:51
For assignments with autoapprove it may happen that students
submit a solution that passes to both their student repo and their
joint group repo, which caused the approval to be counted twice.

This commit ignores submissions from a group repo on a non-group
assignment to be approved, and thus won't be counted. Similarly,
submissions from a student repo to a group assignment will also
be ignored.

This is mainly an issue for autoapprove assignments.
This avoids counting duplicate approvals for the same assignment.
This should hopefully not be necessary for future courses when
the database records avoids this due to the previous commit.
Copy link
Contributor

@meling meling left a comment

Choose a reason for hiding this comment

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

Mostly minor stuff

kit/score/score.pb.go Outdated Show resolved Hide resolved
doc/teacher.md Show resolved Hide resolved
@@ -55,7 +55,6 @@ message Group {
uint64 ID = 1;
string name = 2 [(go.field) = { tags: 'gorm:"uniqueIndex:group"' }];
uint64 courseID = 3 [(go.field) = { tags: 'gorm:"uniqueIndex:group"' }];
uint64 ScmTeamID = 4;
Copy link
Contributor

Choose a reason for hiding this comment

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

Will this be a problem for the database? Or can we just update the database table to ignore this attribute?

@@ -119,31 +117,19 @@ func (s *QuickFeedService) updateGroup(ctx context.Context, sc scm.SCM, request
return fmt.Errorf("failed to get %s repository for group %q: %w", course.Code, group.Name, err)
}
if repo == nil {
// no group repository exists; create new repository for group
if request.Name != "" && newGroup.ScmTeamID < 1 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we still check for request.Name != "" and update newGroup.Name as before?

We will get here only if repo == nil meaning that there is no repo recorded in the database, and thus we haven't created it yet. Thus, we can still change the name according to the request... Or perhaps I'm missing something?

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 don't think it's necessary as a similar check is being done in s.newGroup.

web/groups.go Outdated Show resolved Hide resolved
@meling meling merged commit 9867a6a into master May 24, 2024
15 checks passed
@meling meling deleted the remove-teams branch May 24, 2024 15:35
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.

Remove team creation on github to simplify management of groups
2 participants