Autocomplete for starter code repo #508

cyhsutw · 2016-03-20T03:33:47Z

Hello guys,

This should fix #480.

Here's a short demo:

By default, it shows the repositories that the user can access using the list your repos API.
Or, if the user put type some string like education/t, it will search the the repositories under namespace education (in this case, it will show education/teachers_pet and education/teachers).
It utilizes the repository search API directly for other queries like rails.

Issues with Testing

Previously, I have the AutocompleteController generate JSON responses and use CoffeeScript to create suggestion elements.
After digging into the code base, I found using the render_partial might be more extensible for future additions like searching users.

After this change, it becomes hard to write tests since we will deal with the HTML contents which is relatively harder to validate.

One possible solution is making the controller support both HTML and JSON and the tests will check the JSON responses instead of HTML responses.

Is there any good way to make the AutocompleteController more testable?

I would love to participate GSoC 2016 and this is my first PR for the Classroom. I would be very grateful if you can provide any advice/feedback 😄

Thank you!

- Add a new controller `AutocompleteController`. - Add a new route to `routes.rb`. - Add a new attribute `github_search_client` to `user.rb`. - Add a new view file for autocomplete suggestions.

tarebyte · 2016-03-20T19:27:57Z

config/routes.rb

@@ -12,6 +12,9 @@
  match '/auth/:provider/callback', to: 'sessions#create',  via: [:get, :post]
  match '/auth/failure',            to: 'sessions#failure', via: [:get, :post]

+  # autocomplete


This comment isn't really needed, would you mind taking it out?

tarebyte · 2016-03-20T19:51:20Z

@cyhsutw would you mind using this spinner instead?

tarebyte · 2016-03-20T19:52:55Z

app/assets/javascripts/autocomplete.coffee

+  # TODO: test with IE 8
+  # onpropertychange -> oninput for IE 8
+  # https://msdn.microsoft.com/en-us/library/ms536956(v=vs.85).aspx
+  $('#autocomplete-textfield').on('focus input propertychange', ->


GitHub doesn't support IE 8 https://help.github.com/articles/supported-browsers/, so we don't have to worry about testing for that.

I'm also not a huge fan of id based javascript bindings.

Generally what I do is add another class to the DOM object and add a js- prefix to it.

Example.

<dd><input class="textfield js-autocomplete-textfield" ...>...</dd>

So then your coffee would look like

$('.js-autocomplete-textfield').on('focus input propertychange', ->

tarebyte · 2016-03-20T20:39:58Z

@cyhsutw this is looking really good so far, I left some comments.

Regarding the testing we don't really do any markup testing right now.

What I would do is make controller tests that verify that you get search results from the controller action.

- Remove redundant comments in `.scss`, `.coffee` and `.html.erb`. - Change `id`-based bindings to `class`-based ones. - Add `autocomplete="off"` to `textfield` of `group_assignment_form_options`.

…lassroom into starter-code-repo-autocomplete

cyhsutw · 2016-03-21T02:26:27Z

Hi @tarebyte thanks for reviewing my code.

Your comments are really helpful for me to improve my coding styles.

I've updated my code according to your suggestions, please review them when you are available.

Thank you! 😄

cyhsutw · 2016-03-21T13:36:02Z

Hi @johndbritton @tarebyte

I've updated the code based on your suggestions, please review them when you are available.

Thank you! 😄

cyhsutw · 2016-03-21T14:46:10Z

What I would do is make controller tests that verify that you get search results from the controller action.

@tarebyte does this mean that we only test if the endpoint works?

I tried to write some tests for the AutocompleteController, here's what I got so far:

RSpec.describe AutocompleteController, type: :controller do
  let(:organization) { GitHubFactory.create_owner_classroom_org }
  let(:user)         { organization.users.first                 }

  before do
    sign_in(user)
  end

  describe 'GET #github_repos', :vcr do
    it 'returns success and returns repos of the user' do
      get :github_repos
      expect(response).to have_http_status(:success)
      expect(response).to render_template(partial: 'autocomplete/_repository_suggestions')
    end

    it 'returns success and returns search results of a query' do
      get :github_repos, query: 'rails'
      expect(response).to have_http_status(:success)
      expect(response).to render_template(partial: 'autocomplete/_repository_suggestions')
    end
  end
end

I don't know if it's a proper way to test this controller.

Could you please give me some advice?

Thanks! 😄

tarebyte · 2016-03-23T14:23:39Z

@cyhsutw sorry for not being more clear, I meant test that a known search gives you back the results you expect.

Like rails/rails returns the ruby on rails project.

johndbritton · 2016-03-23T22:31:16Z

app/controllers/autocomplete_controller.rb

+
+  private
+
+  def required_scopes


How does this behave if we don't have the right scopes? Since we're requesting this endpoint via ajax, there's no opportunity to ask for more scopes.

@johndbritton

Currently, there isn't any error handling in this controller (which is bad), I will try to add some of them to make it more robust.

the #required_scopes method is used to request more scopes for the user if necessary. In this case it doens't really make sense, since it's happening in the context of an ajax request.

Sorry for misunderstanding the meaning of the method. I'll pushed a fix later tonight.

Thank you for pointing this out.

- Modify AssignmentController & GroupAssignmentController to accept `repo_id` as a param. - Modify markup & coffeescript to send `repo_id` as a param. - Add specs to test `repo_id` param functionalities.

…lassroom into starter-code-repo-autocomplete

cyhsutw · 2016-03-24T12:57:36Z

Hey @johndbritton @tarebyte

Thank you very much for taking time to review my code 😄

I've update my code based on your suggestions.

Short summary:

Add some specs to test the functionality of AutocompleteController.
Ability to pass/handle repo_id when using autocomplete (as suggested here).
Remove misuse of #required_scopes.

cyhsutw · 2016-03-25T13:02:47Z

johndbritton had a problem deploying to production 23 hours ago

@johndbritton I saw this message in this PR, I'd like to know if there's anything I can help with?
(I've check the log but it shows Not Found)

tarebyte · 2016-03-25T13:04:18Z

@cyhsutw that's because the log it GitHub employee only 😄

We were trying to see if we could use our internal tools to branch deploy a fork, it doesn't look like we can though

cyhsutw · 2016-03-25T13:13:06Z

@tarebyte Thanks for the explanation! 😄

tarebyte · 2016-03-25T13:28:43Z

@cyhsutw can you merge master back in one more time? I think this is good to 🚢

johndbritton · 2016-03-25T13:36:18Z

spec/controllers/assignments_controller_spec.rb

@@ -66,6 +66,23 @@
        expect(flash[:error]).to eql('Invalid repository name, use the format owner/name')
      end
    end
+
+    context 'repo_id for starter_code is passed' do


You need one more context here invalid repo_id for starter_code is passed, this could be the case of a repository the user doesn't have access to or an invalid input for repo_id like the string abc or 0.

johndbritton · 2016-03-25T13:45:10Z

@cyhsutw Left a few more inline notes, but this is looking very good. Once those last few things are addressed I think this is ready to deploy.

cyhsutw · 2016-03-25T15:57:05Z

@johndbritton @tarebyte Thanks for spending time reviewing my code.

I've updated the code based on your suggestions.

You need one more context here invalid repo_id for starter_code is passed, this could be the case of a repository the user doesn't have access to or an invalid input for repo_id like the string abc or 0.

Actually, the controller DOES create assignments with those invalid repo_ids!
To address this, I added a method in concerns/starter_code.rb to check if the user has proper access to that repo.

johndbritton · 2016-03-25T16:24:39Z

@cyhsutw Thank you for your contribution, this has been deployed to production.

cyhsutw · 2016-03-25T16:26:52Z

@johndbritton @tarebyte Thank you for taking time reviewing my code, helped me a lot!

cyhsutw and others added 4 commits March 20, 2016 10:04

Add endpoint API for autcomplete.

717cded

- Add a new controller `AutocompleteController`. - Add a new route to `routes.rb`. - Add a new attribute `github_search_client` to `user.rb`. - Add a new view file for autocomplete suggestions.

Add client side support for autocomplete.

0f3af36

Remove redundant sub-array statement.

e10ffba

Merge branch 'master' into starter-code-repo-autocomplete

9152944

cyhsutw mentioned this pull request Mar 20, 2016

Autocomplete for starter code repo #480

Closed

tarebyte reviewed Mar 20, 2016
View reviewed changes

tarebyte added the ready for review label Mar 20, 2016

cyhsutw added 6 commits March 21, 2016 09:16

Remove redundant comments in scss files.

c4e81be

Update javascript bindings.

49ed235

- Remove redundant comments in `.scss`, `.coffee` and `.html.erb`. - Change `id`-based bindings to `class`-based ones. - Add `autocomplete="off"` to `textfield` of `group_assignment_form_options`.

Neater AutocompleteController.

88d08d8

Update autocomplete endpoint.

8780f67

Update Octocat spinner image.

7c6c59d

Merge branch 'starter-code-repo-autocomplete' of github.com:cyhsutw/c…

6664d1e

…lassroom into starter-code-repo-autocomplete

cyhsutw added 2 commits March 23, 2016 23:36

Add specs for AutocompleteController.

ec4a76b

Cleaner markup and coffeescript.

57b696a

johndbritton reviewed Mar 23, 2016
View reviewed changes

cyhsutw and others added 4 commits March 24, 2016 07:35

Merge branch 'master' into starter-code-repo-autocomplete

36adbcf

Remove #required_scopes from AutocompleteController.

085296d

Use repo_id param if it's present.

6732be1

- Modify AssignmentController & GroupAssignmentController to accept `repo_id` as a param. - Modify markup & coffeescript to send `repo_id` as a param. - Add specs to test `repo_id` param functionalities.

Merge branch 'starter-code-repo-autocomplete' of github.com:cyhsutw/c…

db0ea63

…lassroom into starter-code-repo-autocomplete

johndbritton had a problem deploying to production March 24, 2016 13:53 Failure

Merge branch 'master' into starter-code-repo-autocomplete

a32c197

johndbritton reviewed Mar 25, 2016
View reviewed changes

cyhsutw added 4 commits March 25, 2016 21:46

Merge branch 'master' into starter-code-repo-autocomplete

b64e58e

Refine specs for AutocompleteController.

9ed8eb8

Validate the repo_id passed by the client.

215d0bb

Refine specs to cover cases with invalid repo_id.

6ebf90a

tarebyte merged commit e9cdd20 into education:master Mar 25, 2016

BenEmdon added the ready for review label Oct 13, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Autocomplete for starter code repo #508

Autocomplete for starter code repo #508

cyhsutw commented Mar 20, 2016

tarebyte Mar 20, 2016

tarebyte commented Mar 20, 2016

tarebyte Mar 20, 2016

tarebyte Mar 20, 2016

tarebyte commented Mar 20, 2016

cyhsutw commented Mar 21, 2016

cyhsutw commented Mar 21, 2016

cyhsutw commented Mar 21, 2016

tarebyte commented Mar 23, 2016

johndbritton Mar 23, 2016

cyhsutw Mar 23, 2016

johndbritton Mar 23, 2016

cyhsutw Mar 24, 2016

cyhsutw commented Mar 24, 2016

cyhsutw commented Mar 25, 2016

tarebyte commented Mar 25, 2016

cyhsutw commented Mar 25, 2016

tarebyte commented Mar 25, 2016

johndbritton Mar 25, 2016

johndbritton commented Mar 25, 2016

cyhsutw commented Mar 25, 2016

johndbritton commented Mar 25, 2016

cyhsutw commented Mar 25, 2016

Autocomplete for starter code repo #508

Autocomplete for starter code repo #508

Conversation

cyhsutw commented Mar 20, 2016

Issues with Testing

Choose a reason for hiding this comment

tarebyte commented Mar 20, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tarebyte commented Mar 20, 2016

cyhsutw commented Mar 21, 2016

cyhsutw commented Mar 21, 2016

cyhsutw commented Mar 21, 2016

tarebyte commented Mar 23, 2016

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

cyhsutw commented Mar 24, 2016

cyhsutw commented Mar 25, 2016

tarebyte commented Mar 25, 2016

cyhsutw commented Mar 25, 2016

tarebyte commented Mar 25, 2016

Choose a reason for hiding this comment

johndbritton commented Mar 25, 2016

cyhsutw commented Mar 25, 2016

johndbritton commented Mar 25, 2016

cyhsutw commented Mar 25, 2016