AgileVentures / projectscope

MVP dashboard for ProjectScope, using new gems architecture developed by AV folks
2 stars 14 forks source link

Sort #54

Closed Chenyb closed 7 years ago

Chenyb commented 8 years ago

PV story: https://www.pivotaltracker.com/story/show/132356283 1.Sort feature(add sort feature to each metric) 2.Add seeds 3.Resolve conflict.

tansaku commented 8 years ago

thanks for this @Chenyb - could you link to the relevant pivotal feature in the PR description?

Also, I notice that you have really quite a lot of logic in your controller. We usually don't like controller methods to get over 5 or 6 lines. Any chance you could work on reducing the complexity there? Perhaps moving logic into private methods?

tansaku commented 8 years ago

@Chenyb thanks for this - we have some test failures:

  Scenario: drag upwards                   # features/dashboard_drag.feature:15
    Given I am on the dashboard page       # features/step_definitions/web_steps.rb:45
      Can't find mapping from "the dashboard page" to a path.
      Now, go and add a mapping in /home/travis/build/AgileVentures/projectscope/features/support/paths.rb (RuntimeError)
      ./features/support/paths.rb:36:in `rescue in path_to'
      ./features/support/paths.rb:31:in `path_to'
      ./features/step_definitions/web_steps.rb:46:in `/^(?:|I )am on (.+)$/'
      features/dashboard_drag.feature:16:in `Given I am on the dashboard page'
    And I drag oram before city dog        # features/dashboard_drag.feature:17
      Undefined step: "I drag oram before city dog" (Cucumber::Undefined)
      features/dashboard_drag.feature:17:in `And I drag oram before city dog'
    Then I should see oram before city dog # features/dashboard_drag.feature:18
      Undefined step: "I should see oram before city dog" (Cucumber::Undefined)
      features/dashboard_drag.feature:18:in `Then I should see oram before city dog'
tansaku commented 7 years ago

@Chenyb thanks for updating it - do see @DrakeW 's comments - there are a few other changes you could make

junyu-w commented 7 years ago

@tansaku I'm temporarily taking over the refactoring task from @Chenyb

I found it hard to completely replace the order_by_project_name and order_by_metric_name methods with scopes, because I still need to write logic to determine the sort order based on session[:pre_click]. So in the end I just replaced some of the duplicated part with scopes...

any ideas about how to go around with that? or this is actually what you were expecting?

Thanks

tansaku commented 7 years ago

definite improvement here @DrakeW - if the scopes is getting tricky we can assign further work to a refactoring ticket - although I notice other PRs starting to play with this too :-)

Do we even need a default Project.all - why not have the default ordering start with by project name?

junyu-w commented 7 years ago

@tansaku that's actually a good point.. we should just order projects according to their names..

junyu-w commented 7 years ago

@tansaku refactored logic so that it uses scope directly instead of doing reorder, and sort by project name by default