Skip to content

Conversation

dabrorius
Copy link
Contributor

What type of PR is this? (check all applicable)

  • Refactor
  • Feature
  • Bug Fix
  • Documentation Update

Description

Related Tickets & Documents

Currently several separate pages are all crammed under show action of DashboardsController.
This is the first step in an attempt of refactoring that action and separating it into several smaller controller actions.

Added to documentation?

  • docs.dev.to
  • readme
  • no documentation needed

[optional] What gif best describes this PR or how it makes you feel?

blek

@dabrorius dabrorius force-pushed the extract-following-action-from-dashboard branch from a881693 to 96f496b Compare March 26, 2019 06:42
@dabrorius dabrorius changed the title [WIP] Extract "following" action from "show" in DashboardController Extract "following" action from "show" in DashboardsController Mar 26, 2019
@pr-triage pr-triage bot added the PR: unreviewed bot applied label for PR's with no review label Mar 26, 2019
@lightalloy
Copy link
Contributor

Great 👍
I suggest adding request and feature specs for the /dashboard/following along with this refactoring.

@dabrorius
Copy link
Contributor Author

@lightalloy there are request specs covering this pretty well (https://github.com/thepracticaldev/dev.to/blob/master/spec/requests/dashboard_spec.rb#L59). Do you feel there's a need for feature specs on top of this?

@lightalloy
Copy link
Contributor

@dabrorius Oops, missed these specs somehow, sorry.
In my opinion, it would be nice to have feature specs on this feature.
Maybe it's ok to have them (later) in another pr 🤔

@dabrorius
Copy link
Contributor Author

@lightalloy I can add feature specs as part of this PR, but I'm not sure what to test in it. I'd like to avoid adding redundant feature tests as they would slow the test suite down and would not bring any value.

What do you feel should be tested in the feature spec that is not already tested in the request spec?

@lightalloy
Copy link
Contributor

@dabrorius I reviewed the existing specs again and now I agree with you that probably it's enough to have only requests specs for this functionality. However, I would add checks for the presence of the Followed users (1) and so on to the request specs.

@dabrorius
Copy link
Contributor Author

@lightalloy Makes sense, I'll expand the request specs.

Currently several separate pages are all crammed under `show` action
of DashboardsController.
This is the first step in an attempt of refactoring that action
and separating it into several smaller controller actions.
Add specs for:
- asserting tags count exist on 'following' dashboard
- asserting users count exist on 'following' dashboard
- asserting organizaitons count exist on 'following' dashboard
@dabrorius dabrorius force-pushed the extract-following-action-from-dashboard branch from 96f496b to db8e26f Compare March 26, 2019 11:33
@dabrorius
Copy link
Contributor Author

@lightalloy I've added new specs as discussed. I had to re-organize the specs as rubocop started complaining that I'm nesting specs too deep.

@lightalloy
Copy link
Contributor

@dabrorius Great! That was not obviously the problem of this pr, but I'm trying to make sure that we increase the test coverage when it's possible.
Thanks for your refactoring 🌞

@pr-triage pr-triage bot added PR: reviewed-approved bot applied label for PR's where reviewer approves changes and removed PR: unreviewed bot applied label for PR's with no review labels Mar 26, 2019
Copy link
Contributor

@maestromac maestromac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Thanks for this!

@maestromac maestromac merged commit 7a3b16c into forem:master Mar 27, 2019
@pr-triage pr-triage bot added PR: merged bot applied label for PR's that are merged and removed PR: reviewed-approved bot applied label for PR's where reviewer approves changes labels Mar 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: merged bot applied label for PR's that are merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants