Divide comments_controller_spec.rb into several smaller files

Description

The comments_controller_spec.rb file is about 1000 lines long and growing. It needs to be broken down so it’s more manageable.

Testing

Nothing to test.

Notes

The original approach proposed in this issue was:

  • Specs related to marking a comment as spam or ham in one file

  • Specs related to reviewing moderated comments in another file

  • Probably something else

It lead to the following conversation, which may be useful in determining an approach:

sarken  22:50
I'm not sure what other sort of logical division exists with these remaining sections:
GET #add_comment_reply"
POST #new"
POST #create"
GET #show_comments"
GET #hide_comments"
GET #add_comment"
GET #cancel_comment
GET #cancel_comment_reply
GET #cancel_comment_delete
GET #cancel_comment_edit
GET #destroy
GET #show
GET #index
shared_examples "no one can add or edit comments"
context "on a work hidden by an admin"
context "on an unrevealed work"
new, create, destroy, show, and index can go in one called default_rails_actions_spec.rb, like we have for works, but the others...

22:54
misc_actions_spec.rb, aka "where every single thing will end up in the future, like a junk drawer" or "where no one will think to look for the thing they want"

redsummernight  23:00
we could break the comment specs apart by commentable types at the file level, with each file covering some/all actions

sarken  23:01
Why do you hate me, red? :P

redsummernight  23:02
I've been trying to convert the comment requests spec (covering restricted works) to controller specs, and struggling with organization as well, yeah

sarken  23:04
It's a good idea; it just happens that "being able to see what goes where in here" is the thing standing between me and a pull request
23:05
But if you don't mind shuffling my new stuff around too, I'm game with just... making a best guess
23:05
And letting you have some reorganizing fun at a later date
23:06
(Or, if you're almost done, waiting for you to finish)

redsummernight  23:09
I scrapped my previous attempts; but looking at the specs we do have, a tentative breakdown could be like:
standard actions, logged in users' comments on basic/non-special works, super common stuff (show/add/delete)
same commentable type as the first, but covering moderation actions
commentable = hidden/unrevealed works (similar shared examples)
commentable = restricted works (guest vs non-guest)
commentable = other comments (here's where we test more complex threading, replying)
commentable = other comments (but for freezing this time)

sarken 23:19
That might work, although other comments also needs to be broken down by ultimate parent
23:20
I've got tests for the freeze action on admin posts, tags, and works, because it behaves differently depending on the ultimate parent
23:22
Then I got lazy didn't test the complex scenarios, e.g. "freeze something in the middle of a thread," on each ultimate parent type
23:23
It occurs to me I also need to test deleting, editing, and marking them as spam (edited)
23:23
And probably freezing comments that are unreviewed
23:23
So, okay, way less done than I thought

redsummernight 23:24
you can start on a new file without refactoring the existing stuff - that's where we're headed anyway

redsummernight 23:27
does freezing work the same for all ultimate parent types / can you get away with some set of shared examples?
23:30
(one set, not some)

sarken 23:30
Different permissions, e.g. only PAC admins and work creators can freeze threads on works, only Tag Wrangling on tags, plus the complicating factor of regular users not being able to access tag comments, so you get a different behavior when you try to freeze a comment on a tag than when you try to freeze a comment on a work

sarken 23:35
For example, if you're a user:
you get bounced to your dash with a "you don't have permission to access the page you were trying to reach" error for tags because the :check_tag_wrangler_access filter catches you before you get to the :check_permission_to_freeze filter ever has a chance to run
you get bounced to the ultimate parent with a "sorry, you don't have permission to freeze that comment thread" error for works, because you do make it to the :check_permission_to_freeze filter

redsummernight 23:43
hmm yeah this is why it's so annoying to group/reuse these tests, there are 3 ways they can differ: which commentable, which current user, what expected behavior (like different error messages for guest vs non-guest)
23:43
and since we divide the tests by action instead of those ways, we end up having to repeat combinations of 3 everywhere...

sarken 23:50
Not just commentable, but parent
23:50
So 4 ways they can differ

Activity

Show:

Sarken 
August 19, 2020 at 8:52 AM

@redsummernight Take it away!

DeployedToBeta

Details

Assignee

Reporter

Roadmap

Misc

Priority

Affects versions

Fix versions

Components

AutomatedTests

Difficulty

Milestone

Sentry

Created August 19, 2020 at 2:45 AM
Updated April 29, 2025 at 4:58 PM
Resolved April 29, 2025 at 4:58 PM