Project configuration page with credentials of gems by Shuotong · Pull Request #51 · AgileVentures/projectscope

Shuotong · 2016-10-21T23:14:23Z

User story is here. https://www.pivotaltracker.com/story/show/132780273

junyu-w · 2016-10-21T23:33:48Z

@Shuotong there are two failed tests, can you investigate on them? the 2nd one is failing on my branch too but the first one should be passing with your changes I think

Shuotong · 2016-10-22T00:29:30Z

@DrakeW My features depends on a class method named credentials for every metric gem. The test passed successfully when I change the gemfile to get the gem from my forked and modified version. I already made pull requests for those gems. Once the pr goes through, the test will pass.

tansaku · 2016-10-23T07:08:14Z

@Shuotong I see the other PR - see my comment there, if we're adding functionality to the gem it should be test driven:

AgileVentures/project_metric_slack#16

also in order to make this pass, we could monkey patch the gem in this PR to check that things actually pass in CI, e.g.

class ProjectMetricSlack
  def self.credentials
    ['token','channel']
  end

and then remove that in a subsequent PR - a little risky perhaps (although one can open a ticket as reminder)

tansaku · 2016-10-23T07:19:10Z

@Shuotong could we have a little more description in the PR above, e.g. link to relevant pivotal tracker story etc? many thanks

mtc2013 · 2016-10-24T12:26:39Z

app/views/projects/_form.html.haml

+      %fieldset{:id => name}
+        %legend= name.titleize
+        = hidden_field_tag "project[configs_attributes][#{index}][metric_name]", name
+        - existed = []


this seems to be getting alot of logic in the view.

I agree - the correct elements for display should be calculated in the controller, or even in the model or a separate service - not in the view

The reason I am doing it in the view is that both edit and index render this form. Should I separate them and create a new view for edit?

edit and index? you mean edit and new right? even if they are you can still pull all this into instance variables that you set in the appropriate controller actions and then minimize the code logic in the view itself

yea. Edit and new. Sorry about that. I will move the logic to controller and make a new pull request.

tansaku · 2016-10-24T16:09:54Z

app/views/projects/_form.html.haml

-          = text_field_tag "project[configs_attributes][#{index}][new][]", '', :id => nil, :class => 'newf'
-          \ :
-          = text_field_tag "project[configs_attributes][#{index}][new][]", '', :id => nil, :class => 'newf'
+    - classes = {'github' => ProjectMetricGithub, 'code_climate' => ProjectMetricCodeClimate, 'slack' => ProjectMetricSlack,'pivotal_tracker' => ProjectMetricPivotalTracker,'slack_trends' => ProjectMetricSlackTrends}


this shouldn't be hard coded here - the set of metric gems is defined in https://github.com/AgileVentures/projectscope/blob/develop/config/initializers/project_metrics.rb

there's a method to return metric names, ideally you should call this in the controller and set its value to an instance variable and then refer to that in the view:

https://github.com/AgileVentures/ProjectMetrics/blob/master/lib/project_metrics.rb#L13

Thanks. I will then change the controller to let edit and new to render its own view rather than sharing _form.

I don't think you need separate views, I think you need to move the logic into the controller and set instance variables appropriately and then have this form adapt based on those instance variables, but I am wrong occasionally :-)

That makes sense. Thanks. I will think through this problem again and make a new pull request after everything works well.

tansaku · 2016-10-24T16:13:27Z

app/views/projects/_form.html.haml

+          %br
+        - unless @readonly
+          - classes[name].credentials.each do |cred|
+            -if !existed.include?(cred)


prefer unless to if !

junyu-w · 2016-10-26T07:49:54Z

app/controllers/projects_controller.rb

+      name = config.metric_name
+      if @project_metrics[name].respond_to?(:credentials)
+        config.options.each_pair do |key,val|
+            @existed_configs[name] << key.intern


I think key.to_sym is more straightforward than key.intern, but they do the same thing anyways

Good point. I will change to key.to_sym.

tansaku · 2016-10-26T20:57:12Z

@Shuotong huge improvement - thanks! - now we need to merge this to the upstream develop after I pulled in @DrakeW 's other PR

tansaku · 2016-10-26T20:57:47Z

we've also got a failing test:

    And I enter new "Code Climate" config values:                     # features/step_definitions/project_steps.rb:6
      | key     | value |
      | url     | a.com |
      | channel | 12345 |
      | token   | 555   |
      Unable to find field "code_climate_url" (Capybara::ElementNotFound)
      ./features/step_definitions/project_steps.rb:10:in `block (2 levels) in <top (required)>'
      ./features/step_definitions/project_steps.rb:8:in `each'
      ./features/step_definitions/project_steps.rb:8:in `/^I enter new "(.*)" config values/'
      features/add_project_with_config.feature:13:in `And I enter new "Code Climate" config values:'

Shuotong · 2016-10-26T21:03:35Z

@tansaku It actually passed test locally. Let me look into it. I think it's probably because at the time of testing, the pull requests for gems were not merged so that we didn't have credential method. Everything should be fine now. Could you please run tests again?

tansaku · 2016-10-27T17:38:31Z

@Shuotong re-running - we also have some merge conflicts - perhaps you can fix those - or give me permission to push to your repo to fix them for you

tansaku · 2016-10-27T17:42:58Z

@Shuotong still failing - try fixing the merge conflicts and running bundle update

tansaku · 2016-11-02T12:40:43Z

@Shuotong @DrakeW I merged, but still failing with the same error :-(

    And I enter new "Code Climate" config values:                     # features/step_definitions/project_steps.rb:6
      | key     | value |
      | url     | a.com |
      | channel | 12345 |
      | token   | 555   |
      Unable to find field "code_climate_url" (Capybara::ElementNotFound)
      ./features/step_definitions/project_steps.rb:10:in `block (2 levels) in <top (required)>'
      ./features/step_definitions/project_steps.rb:8:in `each'
      ./features/step_definitions/project_steps.rb:8:in `/^I enter new "(.*)" config values/'
      features/add_project_with_config.feature:13:in `And I enter new "Code Climate" config values:'```

junyu-w · 2016-11-02T17:09:18Z

@tansaku updated Gemfile.lock fixed the test

tansaku · 2016-11-03T18:09:45Z

app/controllers/projects_controller.rb


  # GET /projects/1/edit
  def edit
+    setup_metric_configs(@project)


I notice if I remove this all the tests still pass, and everything still works - surplus to requirement perhaps

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.

tansaku · 2016-11-03T18:12:12Z

app/controllers/projects_controller.rb

  # GET /projects/1/edit
  def edit
+    setup_metric_configs(@project)
+    @project.configs.each do |config|


actually I can remove all this code as well and everything still works ...

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.

tansaku · 2016-11-03T18:22:17Z

app/views/projects/_form.html.haml

-          \ :
-          = text_field_tag "project[configs_attributes][#{index}][new][]", '', :id => nil, :class => 'newf'
-          %br
+        - if @project_metrics[name].respond_to?(:credentials)


it looks like you were moving this logic into the controller, but then it wasn't removed from here

I see. I will pass a hash mapping from the project metrics name to whether it responds to credentials method.

tansaku · 2016-11-03T18:23:18Z

app/controllers/projects_controller.rb

+      @project_metrics[name] = ProjectMetrics.class_for name
+    end
+  end
+


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 end

see http://stackoverflow.com/a/9434319/316729 for more alternatives

Good point. Thanks for your suggestion. I've modified my code accordingly.

tansaku · 2016-11-03T18:24:25Z

@DrakeW @Shuotong well done getting this working - I reviewed in detail and found some code that wasn't doing what it was supposed to - I think there's a little more logic to clean out of the view - I guess we could be making refactoring tickets if there was a rush to merge this in ...

junyu-w · 2016-11-04T05:08:24Z

@tansaku I talked with @Shuotong and we think it's probably better to merge this code in first and then he'll assign himself some refactoring tickets during next iteration.. since our next customer meeting is the coming monday and we'd like to show this feature :) Thanks!

tansaku · 2016-11-04T13:15:30Z

@DrakeW reasonable point - although can you not show the feature on the cs169 server?

junyu-w · 2016-11-04T18:00:27Z

@tansaku we can show this on our staging server, and that would not require this to be merged right now if that's what you were saying?

tansaku · 2016-11-04T22:15:19Z

@DrakeW sorry I think I was mistaken - I thought you guys were deploying code to http://projectscope-cs169.herokuapp.com/projects, but after speaking to An Ju I guess you are not.

Don't you have some of your own heroku instances you can deploy to? I mean I understand you wanting to merge it in and all, and it's great functionality, but with bits of code that have no function? :-) Armando is your customer - you think he wants to see the features more than he wants to see the code doing meaningful things? :-)

Is there some sense in which you need it merged and deployed (to cs169, or the develop server I just created) in order to demo it to the client?

Apologies if I'm not following - I'll check in on this again tomorrow

armandofox · 2016-11-04T22:19:40Z

the understanding is that the team can deploy to their own 'staging' server, and once i sign off on feature impelmentations, the latest features can be deployed to the cs169 production server

tansaku · 2016-11-04T22:20:40Z

thanks @armandofox - so then there's no rush to merge this ...?

junyu-w · 2016-11-05T04:32:24Z

README.md


 <a href="https://codeclimate.com/github/DrakeW/projectscope"><img src="https://codeclimate.com/github/DrakeW/projectscope/badges/gpa.svg" /></a>
 <a href="https://travis-ci.org/DrakeW/projectscope"><img src="https://travis-ci.org/DrakeW/projectscope.svg?branch=develop"></a>
-<a href="https://codeclimate.com/github/DrakeW/projectscope/coverage"><img src="https://codeclimate.com/github/DrakeW/projectscope/badges/coverage.svg" /></a>


this file shouldn't be changed?

not sure - which file are you referring to? sorry ... you mean the README? a codeclimate file?

I mean the README, this line is our repo's code climate coverage badge

ah, being changes from build to coverage - we should display both right?

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?

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

tansaku · 2016-11-15T19:24:02Z

@Shuotong looking forward to updates on this

…ethods

tansaku · 2016-11-22T19:58:20Z

Thanks @Shuotong - big step forward. I've made a few cosmetic changes to standardize indentation and spacing after commas, as well as renaming existed_configs to existing_configs which I think makes more sense grammatically.

I guess it's time to pull this in now, but I am worried that the logic in the view and controller is still more convoluted than it needs to be.

For example you use this method:

  def set_project_metrics
    @project_metrics = ProjectMetrics.metric_names.inject({}) do |hash, name|
      hash[name] = ProjectMetrics.class_for name; hash
    end
  end

to set up a hash for use in the view like so:

- name, index  = cf.object.metric_name, cf.index
   - @project_metrics[name].credentials.each do |cred|

I think it would be a lot less effort to adjust the config model to allow it to report its class directly, e.g.

class Config < ActiveRecord::Base
  def klass
     ProjectMetrics.class_for name
  end
end

and then you could have the following in the view, and no additional code in the controller:

- name, klass, index  = cf.object.metric_name, cf.object.klass, cf.index
   - klass.credentials.each do |cred|

what do you think?

tansaku · 2016-11-23T18:35:06Z

@DrakeW any thoughts on the above? I'd love to at least get a refactoring ticket out of this - I think moving code into models is something that @armandofox would love you all to grasp

junyu-w · 2016-11-29T03:50:44Z

@tansaku I agree with what you proposed above. And @Shuotong is not back yet from his thanksgiving trip so I'm not sure when he can put hands on it. But we can definitely open a refactor ticket

Shuotong · 2016-12-01T07:36:03Z

@tansaku That makes sense. I think it will be helpful for another feature I am working on, which let same services share one config so that for example, we don't need to config slack and slack_trends twice.

tansaku · 2016-12-01T11:24:23Z

@Shuotong @DrakeW that's great - can you paste in links to features and refactoring tickets for future reference? thanks :-)

tansaku · 2016-12-01T18:27:30Z

was just running this branch on the develop server and I notice that the code climate element is asking for channel and token:

some local fail - or possibly the gem reporting the wrong set of requirements?

tansaku · 2016-12-01T18:28:54Z

ah yes, looks like that is the issue: AgileVentures/project_metric_code_climate#12

tansaku · 2016-12-02T15:41:27Z

app/models/config.rb

  attr_accessible :options, :metric_name

+  def klass
+  	ProjectMetrics.class_for self.metric_name


2 space indenting please - let's be consistent :-)

tansaku · 2016-12-02T15:42:19Z

@Shuotong great work - can you see any other logic you can pull into the model? Feel free to create refactoring tickets :-) I think we're closing to getting this merged ...

junyu-w and others added 11 commits October 8, 2016 03:34

updated travis ci badges & iter0 doc

dbc148b

updated code climate badge & iter0 doc

79445f8

added pivotal tracker project url

089e1eb

iter 1: cucumber

703b0be

Merge remote-tracking branch 'upstream/develop' into develop

cc649b2

added status of both main repo and forked repo to readme

ac82d6f

Merge branch 'cucumber' into develop

982f2a3

added heroku staging url

a3ada8b

revise cukes

b3e3256

update _form to use gem's credentials method

d406b7c

remove irrelevant features

4a32792

tansaku mentioned this pull request Oct 23, 2016

Feature/coach login with GitHub & admin login with username/password #50

Merged

mtc2013 reviewed Oct 24, 2016

View reviewed changes

tansaku reviewed Oct 24, 2016

View reviewed changes

Shuotong added 4 commits October 25, 2016 00:00

change credentials strings to symbols

d348357

change unless to if not

5700756

change if not back to unless

fcb1f53

move logic from view to controller

13b2654

junyu-w reviewed Oct 26, 2016

View reviewed changes

change intern to to_sym

739fd3a

updated gems

d8a4ffc

tansaku reviewed Nov 3, 2016

View reviewed changes

junyu-w reviewed Nov 5, 2016

View reviewed changes

Shuotong added 4 commits November 20, 2016 00:19

change hash to ruby style and remove redundant lines

306eef6

move logic from view to controller: check repondency to credentials m…

a0c599c

…ethods

Merge branch 'config' of github.com:DrakeW/projectscope into config

8d99a3c

change styling according to codeclimate report

fc6b85b

add klass method in config model and change logic accordingly

a6c1797

tansaku reviewed Dec 2, 2016

View reviewed changes

Comments

Conversation

Shuotong commented Oct 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

junyu-w commented Oct 21, 2016

Uh oh!

Shuotong commented Oct 22, 2016

Uh oh!

tansaku commented Oct 23, 2016

Uh oh!

tansaku commented Oct 23, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tansaku commented Oct 26, 2016

Uh oh!

tansaku commented Oct 26, 2016

Uh oh!

Shuotong commented Oct 26, 2016

Uh oh!

tansaku commented Oct 27, 2016

Uh oh!

tansaku commented Oct 27, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tansaku commented Nov 2, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

junyu-w commented Nov 2, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tansaku commented Nov 3, 2016

Uh oh!

junyu-w commented Nov 4, 2016

Uh oh!

tansaku commented Nov 4, 2016

Uh oh!

junyu-w commented Nov 4, 2016

Uh oh!

tansaku commented Nov 4, 2016

Uh oh!

armandofox commented Nov 4, 2016 • edited by tansaku Loading Uh oh! There was an error while loading. Please reload this page.

Shuotong commented Oct 21, 2016 •

edited

Loading

tansaku commented Oct 27, 2016 •

edited

Loading

tansaku commented Nov 2, 2016 •

edited

Loading

armandofox commented Nov 4, 2016 •

edited by tansaku

Loading