Skip to content

AO3-4862 Extending work controller test coverage - #3165

Closed
hatal175 wants to merge 5 commits into
otwcode:masterfrom
hatal175:AO3-4862
Closed

AO3-4862 Extending work controller test coverage#3165
hatal175 wants to merge 5 commits into
otwcode:masterfrom
hatal175:AO3-4862

Conversation

@hatal175

Copy link
Copy Markdown
Contributor

Issue

https://otwarchive.atlassian.net/browse/AO3-4862

Purpose

This PR extends the test coverage for works controller.

I've also made a few changes for this purpose and I would really appreciate a review on them:

  1. I've added a route for show_multiple under pseuds so you can see and edit only the works under that pseud.
  2. I've deleted a code path in preview_mode that could not really be reached in current code base.
  3. Deleted unused function tag_list
  4. various bug fixes in previously uncovered code.

Testing

Run tests and see coverage improvement.

Credit

Tal Hayon

Please use he.

Also implemented show multiple for pseuds
- Added edit multiple works for pseud
- Removed several dead code paths
let(:work_params) {
{
work: {
summary: "a" * (ArchiveConfig.SUMMARY_MAX + 1),

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Use 2 spaces for indentation in a hash, relative to the start of the line where the left curly brace is.

context 'when work parameters are invalid' do
let(:work_params) {
{
work: {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Use 2 spaces for indentation in a hash, relative to the start of the line where the left curly brace is.

end

context 'when work parameters are invalid' do
let(:work_params) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Avoid using {...} for multi-line blocks.

it "should throw error when there are invalid tags and trying to preview" do
allow_any_instance_of(Work).to receive(:invalid_tags).and_return([create(:unsorted_tag)])

expect {patch :update_tags, params: { id: work, preview_button: true } }.to raise_error UncaughtThrowError

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Space missing inside {.

end

it "should show results" do
get :collected, params: { user_id: collected_user.login, work_search: { query: "fandom_ids:#{collected_fandom.id}" }}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Space inside } missing.


describe "edit" do
let(:user) { create(:user) }
let(:work) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Avoid using {...} for multi-line blocks.

Comment thread app/controllers/works_controller.rb Outdated
Comment thread app/controllers/works_controller.rb Outdated
Comment thread app/controllers/works_controller.rb Outdated
Comment thread app/controllers/works_controller.rb Outdated
@sarken

sarken commented Jun 29, 2018

Copy link
Copy Markdown
Collaborator

Thanks for the pull request! However, this appears to have gone a bit beyond the scope of the issue, which just calls for the test coverage to be extended. We really do appreciate the bug fixes, but for numerous reasons, we prefer small pull requests that match the issue as closely as possible.

Before we can review this, we'd appreciate it if you could reduce its scope a bit.

I've added a route for show_multiple under pseuds so you can see and edit only the works under that pseud.

This isn't something we've had any requests for and aren't currently interested in implementing.

I've deleted a code path in preview_mode that could not really be reached in current code base.

Hound had an additional comment on this regarding a lingering unused argument. Ultimately, this seems like more of a refactor of the method than a simple deletion of unused code, so we'll make an issue for that once we know what to test to make sure the method still works.

Deleted unused function tag_list

We've made a separate issue for this: AO3-5457

various bug fixes in previously uncovered code.

We'll need a detailed list of the bugs and descriptions of how to manually reproduce them (whenever possible) to make issues as appropriate.

You can either give us the info here or email otw-coders@transformativeworks.org with the details -- whichever is easier.

Thanks again!

let(:work_params) {
{
work: {
summary: "a" * (ArchiveConfig.SUMMARY_MAX + 1),

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Layout/IndentHash: Use 2 spaces for indentation in a hash, relative to the start of the line where the left curly brace is.
Style/TrailingCommaInHashLiteral: Avoid comma after the last item of a hash.

context "when work parameters are invalid" do
let(:work_params) {
{
work: {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Layout/IndentHash: Use 2 spaces for indentation in a hash, relative to the start of the line where the left curly brace is.

end

context "when work parameters are invalid" do
let(:work_params) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Style/BlockDelimiters: Avoid using {...} for multi-line blocks.

it "should throw error when there are invalid tags and trying to preview" do
allow_any_instance_of(Work).to receive(:invalid_tags).and_return([create(:unsorted_tag)])

expect {patch :update_tags, params: { id: work, preview_button: true } }.to raise_error UncaughtThrowError

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Layout/SpaceInsideBlockBraces: Space missing inside {.

end

xit "should show results" do
get :collected, params: { user_id: collected_user.login, work_search: { query: "fandom_ids:#{collected_fandom.id}" }}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Layout/SpaceInsideHashLiteralBraces: Space inside } missing.

let(:update_work) {
let!(:update_user) { create(:user) }
let!(:update_chapter) { create(:chapter) }
let!(:update_work) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Style/BlockDelimiters: Avoid using {...} for multi-line blocks.


context "destroy" do
let(:user) { create(:user) }
let!(:work) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Style/BlockDelimiters: Avoid using {...} for multi-line blocks.


describe "edit" do
let(:user) { create(:user) }
let(:work) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Style/BlockDelimiters: Avoid using {...} for multi-line blocks.

@hatal175

hatal175 commented Jul 7, 2018

Copy link
Copy Markdown
Contributor Author

Well, I removed the changes I made beyond adding tests and pending tests. I've sent a mail about the things I found.
I'd still like to reexamine the preview mode lines and another line dealing with collections before I consider this ready for review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants