Create Membership Event Job #1254

Kadaaran · 2018-02-02T17:22:07Z

What happens now: When a member is removed from a team, Classroom still displays this member and do not let users join the team(because it appears to be full in some cases).

What it changes: Now we display the right numbers of members ✨

Why I need help: I think Travis is failing because when I run RSPEC locally and I create RepoAccess, it tries to add the user I created from the payload to my organization and apparently the API is not happy with that and returns a 401 but for now I don't know why. I'm still trying to figure out why but if someone has an idea I'd be happy to hear more about it ✨

To do next:

Same thing with people being removed from organization 👍
Fix this massive dependencies in the rspec file
Bug fixed thanks to @d12 🍰

houndci-bot · 2018-02-02T22:02:45Z

spec/jobs/membership_event_job_spec.rb

+  let(:organization) { classroom_org }
+  let(:student)      { create(:user, uid: payload.dig("member", "id")) }
+  let(:repo_access)  { RepoAccess.create(user: student, organization: organization) }
+  let(:group)        { Group.create(title: 'Group 2', github_team_id: payload.dig("team", "id")) }


Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

koenyskout · 2018-02-21T08:46:33Z

app/jobs/membership_event_job.rb

+    team_id = payload_body.dig("team", "id")
+
+    user = User.find_by(uid: user_id)
+    return true if user.present?


I think the switches between if/unless and blank/present reversed the logic of this line (and the one for group below).

But it still works though 🤔

houndci-bot · 2018-02-23T17:23:20Z

spec/jobs/membership_event_job_spec.rb

+      group = Group.create(github_team_id: payload.dig("team", "id"), title: payload.dig("team", "name"), grouping: group_assignment.grouping)
+      repo_access = RepoAccess.find_or_create_by!(user: student, organization: organization)
+
+      #fails with the next line


Missing space after #.

houndci-bot · 2018-02-23T17:23:20Z

spec/jobs/membership_event_job_spec.rb

+  context "ACTION member_removed", :vcr do
+    it "removes user from team" do
+      group_assignment = create(:group_assignment, title: "Intro to Rails #2", organization: organization)
+      group = Group.create(github_team_id: payload.dig("team", "id"), title: payload.dig("team", "name"), grouping: group_assignment.grouping)


Line is too long. [142/120]

houndci-bot · 2018-03-05T15:49:42Z

spec/jobs/membership_event_job_spec.rb

+  context "ACTION member_removed", :vcr do
+    it "removes user from team" do
+      group_assignment = create(:group_assignment, title: "Intro to Rails #2", organization: organization)
+      group = Group.create(title: "GROUP", github_team_id: payload.dig("team", "id"), grouping: group_assignment.grouping)


Line is too long. [122/120]

Kadaaran · 2018-03-05T16:24:53Z

@fjorgemota Not yet, just a little change and we will be good to change for review 👍

fjorgemota · 2018-03-11T23:19:32Z

Guys, Sorry to bother again, but...is there any plans to review this?

I'm going to use Github Classroom on a class this semester and this PR is very important to fix problems where a student join a wrong group.

Thanks.

d12 · 2018-03-12T13:40:33Z

Hey @fjorgemota ,

We're ramping up some more engineers to work on this project, but for now we don't have a ton of bandwidth. We'll get this shipped as soon as we can.

I'm going to use Github Classroom on a class this semester and this PR is very important to fix problems where a student join a wrong group.

Are you saying there's a student who's joined the wrong group? Or you're trying to prevent that from happening in the future? If something's gone wrong with your Classroom, can you email education@github.com? We can try to get you sorted while you wait for this fix. Thanks :)

d12

One super small style thing, this looks great! cc @tarebyte.

d12 · 2018-03-12T13:43:34Z

app/jobs/membership_event_job.rb

+  queue_as :github_event
+
+  def perform(payload_body)
+    return true unless payload_body.dig("action") == "removed"


We probably don't need dig here, we can just do payload_body["action"]

fjorgemota · 2018-03-12T14:23:16Z

Hey @d12! Nice to know that Github Classroom will receive more attention!

About your question, I'm trying to prevent that from happening on the class this semester, because for now the use of Github Classroom will be just a test with a class where i'm a teaching assistant, to see if it helps with the verification of the activities (just by having a simple git diff I think it will), and I did not know that you're available to receive small problems manually on the database (I mean, I think that this problem is not too frequent, but it's good to know anyway).

About the test, I think that this is the most import important issue we detected right now that may affect us. We will use Github Classroom together with the Moodle of the university (not a direct integration, tho) so for us #603 will not be a big problem right now (because we will request the student to enter a limited group on Moodle (which have these restrictions) and on the same group on Github Classroom), so it's not a big problem right now.

Anyway, here's a "how-to document" that me and the teacher developed to allow students to use git and github classroom more easily: https://github.com/nanvix/teaching/tree/master/howtos/github-classroom (it's in Portuguese, but ... okay)

Thanks and keep the good work, guys. =D

d12 · 2018-03-12T14:35:26Z

@fjorgemota Ah I see. We're done with reviewing now, so this will get deployed likely sometime today or tomorrow. :)

Glad to hear you're trying out Classroom for your course, if you run into any problems or any features you think would help you, we take feature requests here!

fjorgemota · 2018-03-12T14:38:11Z

@d12 Nice! Thanks for the support, and like I said before, sorry for bothering. =)

tarebyte · 2018-03-12T15:17:13Z

app/jobs/membership_event_job.rb

+    return true unless group.present? && user.present?
+
+    repo_access = group.repo_accesses.find_by(user_id: user.id)
+    repo_access&.destroy


So I don't think we want to delete the repo_access here, just the fact has_and_belongs_to_many relationship exists.

So it would be something like

repo_access = group.repo_accesses.find_by(user_id: user.id) group.repo_accesses.delete(repo_access)

…com:education/classroom into synchronize_users_removed_from_organization

Kassandre added 9 commits January 25, 2018 18:06

Add membership as an accepted event

74eb7c3

Create a membership job to get a team member is removed

0938be2

Use safe navigation

b9d68d1

Update perform based on tarebyte's comments

29c0b71

Change present? to blank? based on Hound request

ad7606c

Add if present? instead of unless blank?

aa2e9c9

Init spec file

26b83b1

Add team_member_removed payload

6077cc6

First shot spec membership and add MembershipEventJob cassette

79e0f13

Kadaaran requested review from d12 and tarebyte February 2, 2018 17:22

Kadaaran changed the title ~~[WIP] Crete Membership Event Job~~ [WIP] Create Membership Event Job Feb 2, 2018

Kadaaran force-pushed the synchronize_users_removed_from_organization branch from e4750f9 to 0e5c2db Compare February 2, 2018 18:13

houndci-bot reviewed Feb 2, 2018

View reviewed changes

Kadaaran changed the title ~~[WIP] Create Membership Event Job~~ Create Membership Event Job Feb 2, 2018

Kadaaran added help wanted 🖐️ WIP 💭 PR's that are a 'Work In Progress' labels Feb 2, 2018

Remove cassette and remove context to see if travis likes it or not

d88effa

Kadaaran force-pushed the synchronize_users_removed_from_organization branch from 74923aa to d88effa Compare February 2, 2018 22:07

Kadaaran mentioned this pull request Feb 16, 2018

Inconsistency in the teams structure between GitHubClassroom and the organization #1273

Closed

koenyskout reviewed Feb 21, 2018

View reviewed changes

Merge branch 'master' into synchronize_users_removed_from_organization

c16c1b1

Kadaaran mentioned this pull request Feb 23, 2018

Deleted user still shows up in assignment group #1289

Closed

houndci-bot reviewed Feb 23, 2018

View reviewed changes

Kadaaran force-pushed the synchronize_users_removed_from_organization branch from ff5a318 to 24b9faa Compare February 23, 2018 17:25

New shot to fix 401

24b9faa

Kadaaran mentioned this pull request Mar 2, 2018

Rejoining group assignment #1139

Closed

Fix membership event job spec

e5774be

houndci-bot reviewed Mar 5, 2018

View reviewed changes

Hound fix

a89bdef

Kadaaran removed the help wanted 🖐️ label Mar 5, 2018

Kassandre added 2 commits March 5, 2018 17:29

Prevent groups with the same name to exists

e7a1105

Merge branch 'master' into synchronize_users_removed_from_organization

2f9d20a

Kadaaran added 🚀 ready for review and removed WIP 💭 PR's that are a 'Work In Progress' labels Mar 5, 2018

d12 approved these changes Mar 12, 2018

View reviewed changes

Remove .dig and add payload_body[action]

9ffd720

tarebyte approved these changes Mar 12, 2018

View reviewed changes

Merge branch 'master' into synchronize_users_removed_from_organization

34f7b94

tarebyte suggested changes Mar 12, 2018

View reviewed changes

Kassandre added 3 commits March 12, 2018 18:30

Update repo_access w/ delete instead of destroy

fff9a8e

Merge branch 'synchronize_users_removed_from_organization' of github.…

dc4855e

…com:education/classroom into synchronize_users_removed_from_organization

Add new cassettes for membership_job

0708118

tarebyte approved these changes Mar 12, 2018

View reviewed changes

Kadaaran merged commit a0f29d2 into master Mar 13, 2018

Kadaaran deleted the synchronize_users_removed_from_organization branch March 13, 2018 14:27

This was referenced Mar 13, 2018

Could not join the team again...?? #1229

Closed

Number of group members wrong #1216

Closed

Merging teams, adding new member or other operations on previously created team #1295

Closed

Automatically merge teams #1264

Closed

BenEmdon added 🚀 ready for review help wanted 🖐️ labels Oct 13, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create Membership Event Job #1254

Create Membership Event Job #1254

Kadaaran commented Feb 2, 2018 •

edited

Loading

houndci-bot Feb 2, 2018

koenyskout Feb 21, 2018

Kadaaran Feb 21, 2018

houndci-bot Feb 23, 2018

houndci-bot Feb 23, 2018

houndci-bot Mar 5, 2018

Kadaaran commented Mar 5, 2018

fjorgemota commented Mar 11, 2018

d12 commented Mar 12, 2018

d12 left a comment

d12 Mar 12, 2018

fjorgemota commented Mar 12, 2018

d12 commented Mar 12, 2018

fjorgemota commented Mar 12, 2018

tarebyte Mar 12, 2018

Create Membership Event Job #1254

Create Membership Event Job #1254

Conversation

Kadaaran commented Feb 2, 2018 • edited Loading

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

Kadaaran commented Mar 5, 2018

fjorgemota commented Mar 11, 2018

d12 commented Mar 12, 2018

d12 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fjorgemota commented Mar 12, 2018

d12 commented Mar 12, 2018

fjorgemota commented Mar 12, 2018

Choose a reason for hiding this comment

Kadaaran commented Feb 2, 2018 •

edited

Loading