-
Notifications
You must be signed in to change notification settings - Fork 14
Project configuration page with credentials of gems #51
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: develop
Are you sure you want to change the base?
Changes from 15 commits
dbc148b
79445f8
089e1eb
703b0be
cc649b2
ac82d6f
982f2a3
a3ada8b
b3e3256
d406b7c
4a32792
d348357
5700756
fcb1f53
13b2654
739fd3a
b8ca10c
d8a4ffc
306eef6
a0c599c
8d99a3c
fc6b85b
a6c1797
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,10 +1,12 @@ | ||
| class ProjectsController < ApplicationController | ||
| include ProjectsHelper | ||
| before_action :set_project, only: [:show, :edit, :update, :destroy] | ||
|
|
||
| before_action :set_project_metrics, only: [:show, :edit, :new] | ||
| before_action :init_existed_configs, only: [:show, :edit, :new] | ||
| http_basic_authenticate_with name: "cs169", password: ENV['PROJECTSCOPE_PASSWORD'] | ||
|
|
||
| # GET /projects | ||
| # GET /projects.json | ||
|
|
||
| def index | ||
| @projects = Project.all | ||
| @metric_names = ProjectMetrics.metric_names | ||
|
|
@@ -24,6 +26,15 @@ def new | |
|
|
||
| # GET /projects/1/edit | ||
| def edit | ||
| setup_metric_configs(@project) | ||
| @project.configs.each do |config| | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. actually I can remove all this code as well and everything still works ...
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The loop here checks whether certain credentials has already been given value. In _form.html, it first displays all the existed credentials and adds fields for credentials that haven't been configured. The cucumber test would pass, and everything still works if we remove this part. But on the edit page, for example, if we already have an url for codeclimate, it still provides an empty field to config url. I hope that makes sense. I should probably modify the cucumber for edit page to test this scenario. |
||
| name = config.metric_name | ||
| if @project_metrics[name].respond_to?(:credentials) | ||
| config.options.each_pair do |key,val| | ||
| @existed_configs[name] << key.intern | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point. I will change to key.to_sym. |
||
| end | ||
| end | ||
| end | ||
| end | ||
|
|
||
| # POST /projects | ||
|
|
@@ -73,6 +84,20 @@ def set_project | |
| @project = Project.includes(:configs).find(params[:id]) | ||
| end | ||
|
|
||
| def set_project_metrics | ||
| @project_metrics = {} | ||
| ProjectMetrics.metric_names.each do |name| | ||
| @project_metrics[name] = ProjectMetrics.class_for name | ||
| end | ||
| end | ||
|
|
||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. slightly more rubyish version: def set_project_metrics
@project_metrics = ProjectMetrics.metric_names.inject({}) do |hash, name|
hash[name] = ProjectMetrics.class_for name; hash
end
endsee http://stackoverflow.com/a/9434319/316729 for more alternatives
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point. Thanks for your suggestion. I've modified my code accordingly. |
||
| def init_existed_configs | ||
| @existed_configs = {} | ||
| ProjectMetrics.metric_names.each do |name| | ||
| @existed_configs[name] = [] | ||
| end | ||
| end | ||
|
|
||
| # Never trust parameters from the scary internet, only allow the white list through. | ||
| def project_params | ||
| # Grab new option keys/vals from params, and incorporate them into | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -24,15 +24,24 @@ | |
| = text_field_tag field_name, val, :disabled => @readonly | ||
| %br | ||
| - unless @readonly | ||
| = link_to 'Add new', '', :class => 'add' | ||
| .field.new | ||
| = text_field_tag "project[configs_attributes][#{index}][new][]", '', :id => nil, :class => 'newf' | ||
| \ : | ||
| = text_field_tag "project[configs_attributes][#{index}][new][]", '', :id => nil, :class => 'newf' | ||
| %br | ||
| - if @project_metrics[name].respond_to?(:credentials) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it looks like you were moving this logic into the controller, but then it wasn't removed from here
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see. I will pass a hash mapping from the project metrics name to whether it responds to credentials method. |
||
| - @project_metrics[name].credentials.each do |cred| | ||
| -unless @existed_configs[name].include?(cred) | ||
| .field.new | ||
| = label_tag "project[configs_attributes][#{index}][new][]", "#{cred}", :id => nil, :class => 'newf' | ||
| \ : | ||
| = hidden_field_tag "project[configs_attributes][#{index}][new][]", "#{cred}", :id => nil, :class => 'newf' | ||
| = text_field_tag "project[configs_attributes][#{index}][new][]", '', :id => "#{name}_#{cred}", :class => 'newf' | ||
| - else | ||
| = link_to 'Add new', '', :class => 'add' | ||
| .field.new | ||
| = text_field_tag "project[configs_attributes][#{index}][new][]", '', :id => nil, :class => 'newf' | ||
| \ : | ||
| = text_field_tag "project[configs_attributes][#{index}][new][]", '', :id => nil, :class => 'newf' | ||
| %br | ||
|
|
||
| :javascript | ||
| $('.add').click(function() { $(this).closest('fieldset').append($('.new')[0].outerHTML); return(false); } ) | ||
| :javascript | ||
| $('.add').click(function() { $(this).closest('fieldset').append($('.new')[0].outerHTML); return(false); } ) | ||
|
|
||
| - unless @readonly | ||
| .actions | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| ## Iteration 0 | ||
|
|
||
| Entry video | ||
| --- | ||
| URL: https://youtu.be/0qMtzclG9Is | ||
|
|
||
| Development Environment Setup Screencast | ||
| --- | ||
| Junyu Wang: https://youtu.be/pPfSiQsZm5I | ||
|
|
||
| Jiacheng Wu: https://youtu.be/x8AOtKAzmaQ | ||
|
|
||
| Shuotong Wu: http://youtu.be/-65Oz8VtzDw?hd=1 | ||
|
|
||
| Yibing Chen: https://youtu.be/U1Z-9KzoL7c | ||
|
|
||
| Jiawei Jiang: https://youtu.be/ClqYbe2Ipnc | ||
|
|
||
| Pivotal Tracker Project | ||
| --- | ||
| URL: https://www.pivotaltracker.com/n/projects/1886749 | ||
|
|
||
| Heroku | ||
| --- | ||
| URL: https://projectscope-cs169-junyu.herokuapp.com/ | ||
|
|
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.
I notice if I remove this all the tests still pass, and everything still works - surplus to requirement perhaps
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.
Yea. I've done some tests as well. And I think this line is redundant. This function is already called when rendering _form.html. And also we no longer need to include ProjectsHelper then.