Dashboard for Teachers (time of the last push & build status) #708

cyhsutw · 2016-08-18T02:50:21Z

Part of #613.

This pull request adds functionality for teachers to check the time of the last push and the build status of the latest commit.

Preview:

Edit: This feature is now placed under Flipper teacher_dashboard flag.

Would love to hear you feedback! Thanks 😄

cyhsutw · 2016-08-18T13:49:26Z

Currently, this function will cost a lot of time for the pages to load, I'll change the implementations using AJAX.

nwoodthorpe · 2017-05-23T23:09:57Z

Hey @cyhsutw how close is this to being finished? I'd love to see this get merged, it looks great from what I can see.

nwoodthorpe · 2017-05-24T21:39:17Z

Closing for now, feel free to re-open if there's progress.

cyhsutw · 2017-05-25T11:46:27Z

@nwoodthorpe Thanks for the heads-up. I've merged master into this and made necessary modifications. Please review it at your convenience. 😄

cyhsutw · 2017-05-25T11:51:17Z

spec/factories.rb

@@ -52,7 +52,9 @@
    end

    after(:build) do |organization, evaluator|
-      create_list(:user, evaluator.users_count, organizations: [organization])
+      if evaluator.users.count < evaluator.users_count
+        create_list(:user, evaluator.users_count, organizations: [organization])


I added this check because this is adding random extra users to the organization.

This will break the functionality of Organization#github_client since the access_token of the randomly created user is not valid, which will make the API calls return 401.

Sorry, could you explain this a bit more? What's creating random users in the organization?

Having to edit this random unrelated line of code that's been fine for 2 months worries me a little bit

I'll try my best to explain it.

So this line of code will generate a list of users with length equals to evaluator.users_count, and the users are generated using this factory.

This is totally okay if we don't access any of the users generated by this line of code (and yes, there isn't a line in the codebase that does this). However, if we try to use any of the access tokens that are associated with these users to call GitHub APIs, it simply fails as these are just some random hex string.

This line of code will create a new user even if we already specified users to be associated with the organization (e.g. GitHubFactory#classroom_org).

This is the issue I faced when I run test examples:

let(:organization) { classroom_org } # => organization.users.size <--- this is 2!

In this case, you can only get a working organization.github_client on a 50% chance.

IMHO the reason we have this line of code is to ensure that there's at least one user in an organization, we don't need to add another user if we already got one. That's the reason I made this change.

Please let me know if there's anything I can help. Thanks! 😄

Right, because a token is chosen from a random user. In that case, this makes sense to me.

nwoodthorpe · 2017-05-25T15:38:20Z

Thanks for getting this sorted! It's a pretty hefty diff, I'll try to find some time today to step through it.

nwoodthorpe · 2017-05-25T19:45:24Z

app/controllers/concerns/github_repo_status.rb

@@ -0,0 +1,41 @@
+# frozen_string_literal: true
+
+module GitHubRepoStatus


+1 for using a concern here. Assignment and group-assignment are super similar, we should be using these more often.

nwoodthorpe · 2017-05-25T19:48:47Z

lib/github/event.rb

+
+    def latest_push_event(repo_id, options = { per_page: 100, page: 1 })
+      events = event_client.repository_events(repo_id, options).select do |event|
+        event.type == 'PushEvent' ||


These can be on the same line

nwoodthorpe · 2017-05-25T19:49:04Z

lib/github/event.rb

+          (event.type == 'CreateEvent' && event.payload.ref.present?)
+      end
+
+      events.length.positive? ? events.first : nil


Any reason for this vs #empty?

Nice catch! Should have used #empty for this.

I'm going to simplify this further using #first only as the result will be the same according to the docs.

nwoodthorpe · 2017-05-25T19:51:34Z

spec/factories.rb

@@ -52,7 +52,9 @@
    end

    after(:build) do |organization, evaluator|
-      create_list(:user, evaluator.users_count, organizations: [organization])
+      if evaluator.users.count < evaluator.users_count
+        create_list(:user, evaluator.users_count, organizations: [organization])


Right, because a token is chosen from a random user. In that case, this makes sense to me.

nwoodthorpe · 2017-05-25T19:53:43Z

This looks really good to me! Thanks for finishing this up

nwoodthorpe · 2017-05-25T19:55:04Z

Also I'd like to squash before we merge. You don't have to worry about it yet though.

cyhsutw · 2017-05-26T01:23:57Z

@nwoodthorpe Thanks for reviewing! I've updated the code to resolve the issues you spotted, please have a 👀 .

Also I'd like to squash before we merge.

+1 on this. Would you like me to do that or this can be done using the squash on merge feature on GitHub?

nwoodthorpe · 2017-05-26T16:40:52Z

Code LGTM

Yeah, squash on merge should be fine. @johndbritton for review

stale · 2017-07-25T21:27:51Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

cyhsutw added ready for review WIP 💭 PR's that are a 'Work In Progress' and removed ready for review labels Aug 18, 2016

cyhsutw added ready for review and removed WIP 💭 PR's that are a 'Work In Progress' labels Aug 19, 2016

cyhsutw closed this Aug 19, 2016

cyhsutw reopened this Aug 19, 2016

cyhsutw added 22 commits November 18, 2016 16:16

Add GitHub event client

66b19e9

Change page size

f7f42e6

Copy paste error

ce4ae47

Add method to get latest push event

9167ffe

Shows last push time

3b45d0d

Use ref instead of head

f7c5c01

Also considers CreateEvent

4b727cf

Move rendering code to helper

e8535f6

Add shared GitHub repository status templates

556815b

Add repo status to assignment repos

4a9753f

Error handling

24ad834

Add GitHub repo status to group assignment repos

5a4c89a

Don't cache status

2d0a577

Remove unused styles

84b2558

Adjust spacing

9d273ef

Refactor a bit

b2e90ba

Add a test for GitHub::Event

8752ce5

Add tests for new instance methods of GitHubRepository

408c691

Handle deleted state

ba42374

Add helper method: html_url for specific ref

1b5061d

Add shared methods for checking status

7a14f0b

Add two endpoints for checking repo status

5769b91

asyraf290885 approved these changes May 8, 2017

View reviewed changes

nwoodthorpe closed this May 24, 2017

cyhsutw added 4 commits May 25, 2017 18:23

Merge branch 'master' into dashboard-for-teacher

8ef3012

Remove unused code

790d129

Reduce diff

c89223b

Added eager load for assignment

c0c8026

cyhsutw reopened this May 25, 2017

cyhsutw commented May 25, 2017

View reviewed changes

nwoodthorpe reviewed May 25, 2017

View reviewed changes

Simplify empty check & combine logics in one line

5d6c888

nwoodthorpe mentioned this pull request Jun 13, 2017

Dashboard for Teachers #613

Open

obcode mentioned this pull request Jun 13, 2017

Dashboard: Overview over all assignments grouped by student #918

Closed

nwoodthorpe mentioned this pull request Jun 14, 2017

Update repo/show UI to fit more information #920

Merged

4 tasks

stale bot added the stale label Jul 25, 2017

stale bot closed this Aug 2, 2017

tarebyte deleted the dashboard-for-teacher branch October 20, 2017 00:21

BenEmdon added ready for review stale and removed wontfix labels Oct 13, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dashboard for Teachers (time of the last push & build status) #708

Dashboard for Teachers (time of the last push & build status) #708

cyhsutw commented Aug 18, 2016 •

edited

cyhsutw commented Aug 18, 2016

nwoodthorpe commented May 23, 2017

nwoodthorpe commented May 24, 2017

cyhsutw commented May 25, 2017

cyhsutw May 25, 2017

nwoodthorpe May 25, 2017

cyhsutw May 25, 2017

nwoodthorpe May 25, 2017

nwoodthorpe commented May 25, 2017

nwoodthorpe May 25, 2017

nwoodthorpe May 25, 2017

cyhsutw May 26, 2017

nwoodthorpe May 25, 2017

cyhsutw May 26, 2017

nwoodthorpe May 25, 2017

nwoodthorpe commented May 25, 2017

nwoodthorpe commented May 25, 2017

cyhsutw commented May 26, 2017

nwoodthorpe commented May 26, 2017

stale bot commented Jul 25, 2017

		@@ -0,0 +1,41 @@
		# frozen_string_literal: true

		module GitHubRepoStatus

Dashboard for Teachers (time of the last push & build status) #708

Dashboard for Teachers (time of the last push & build status) #708

Conversation

cyhsutw commented Aug 18, 2016 • edited

cyhsutw commented Aug 18, 2016

nwoodthorpe commented May 23, 2017

nwoodthorpe commented May 24, 2017

cyhsutw commented May 25, 2017

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

nwoodthorpe commented May 25, 2017

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

nwoodthorpe commented May 25, 2017

nwoodthorpe commented May 25, 2017

cyhsutw commented May 26, 2017

nwoodthorpe commented May 26, 2017

stale bot commented Jul 25, 2017

cyhsutw commented Aug 18, 2016 •

edited