Project configuration page with credentials of gems

[Shuotong]

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

[junyu-w]

@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]

@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]

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

https://github.com/AgileVentures/project_metric_slack/pull/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]

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

[tansaku]

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

[tansaku]

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]

@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]

@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]

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

[tansaku]

@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]

@tansaku updated Gemfile.lock fixed the test

[tansaku]

@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]

@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]

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

[junyu-w]

@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]

@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]

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]

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

[tansaku]

@Shuotong looking forward to updates on this

[tansaku]

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]

@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]

@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]

@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]

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

[tansaku]

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]

ah yes, looks like that is the issue: https://github.com/AgileVentures/project_metric_code_climate/issues/12

[tansaku]

@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 ...