Project configuration page with credentials of gems #51
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,9 +1,14 @@ | ||
| 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] | ||
| before_action :authenticate_user! | ||
|
|
||
| # GET /projects | ||
| # GET /projects.json | ||
|
|
||
| def index | ||
| @projects = Project.all | ||
| @metric_names = ProjectMetrics.metric_names | ||
|
|
@@ -23,6 +28,15 @@ def new | |
|
|
||
| # GET /projects/1/edit | ||
| def edit | ||
| setup_metric_configs(@project) | ||
|
Contributor
There was a problem hiding this comment. I notice if I remove this all the tests still pass, and everything still works - surplus to requirement perhaps
Author
There was a problem hiding this comment. 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. |
||
| @project.configs.each do |config| | ||
|
Contributor
There was a problem hiding this comment. actually I can remove all this code as well and everything still works ...
Author
There was a problem hiding this comment. 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.to_sym | ||
| end | ||
| end | ||
| end | ||
| end | ||
|
|
||
| # POST /projects | ||
|
|
@@ -72,6 +86,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. 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. 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 | ||
|
|
||
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.
this file shouldn't be changed?
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.
not sure - which file are you referring to? sorry ... you mean the README? a codeclimate file?
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 mean the README, this line is our repo's code climate coverage badge
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.
ah, being changes from build to coverage - we should display both right?
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.
did I change this? sorry - I thought I was updating to be correct - feel free to set to what you think it should be - for this whole branch though the main thing we need is the refactoring of the view to use the new controller code right?
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.
Sorry for the late comment, been busy with other stuff recently..
and @Shuotong can you update this readme to not remove the coverage badge and update this branch according to @tansaku 's comments days ago? Thanks