AO3-4913 Increase coverage of ChallengeSignupsController #2798
Conversation
| end |
There was a problem hiding this comment.
Extra empty line detected at block body end.
There was a problem hiding this comment.
Just a reminder: with specs, you don't want to describe the big picture ("checks that signups are open") but rather what happens if sign-ups are open and what happens if sign-ups are closed. I haven't reviewed the code, but I've suggested better descriptions.
| let(:open_signup_owner) { Pseud.find(open_signup.pseud_id).user } | ||
|
|
||
| describe "new" do | ||
| it "ensures signups are open" do |
There was a problem hiding this comment.
it redirects and errors if sign-up is not open
| end | ||
|
|
||
| describe "show" do | ||
| xit "checks that there is a challenge" do |
There was a problem hiding this comment.
it redirects and errors if there is no challenge associated with the collection
| "What sign-up did you want to work on?") | ||
| end | ||
|
|
||
| it "checks ownership" do |
There was a problem hiding this comment.
it redirects and errors if the user does not own the sign-up
| end | ||
|
|
||
| describe "index" do | ||
| it "checks for the right owner" do |
There was a problem hiding this comment.
it redirects and errors if the current user is not allowed to see the specified user's sign-ups
|
|
||
| describe "destroy" do | ||
| context "signups are open" do | ||
| it "checks that signups are open" do |
There was a problem hiding this comment.
it deletes the sign-up and redirects with notice
| end | ||
| end | ||
| context "signups are closed" do | ||
| it "checks that signups are open" do |
| expect(response).to render_template('edit') | ||
| end | ||
|
|
||
| it "checks ownership of the signup" do |
There was a problem hiding this comment.
it redirects and errors if the current user can't edit the sign-up
| end | ||
|
|
||
| context "signups are closed" do | ||
| it "does not allow edits when signups are closed" do |
There was a problem hiding this comment.
You already have "signups are closed" in the context, so this only needs it redirects and errors without updating the sign-up
sarken
dismissed
their stale review
The descriptions are mostly good now, just needs code review
| let(:open_signup_owner) { Pseud.find(open_signup.pseud_id).user } | ||
|
|
||
| describe "new" do | ||
| it "directs and errors if sign-up is not open" do |
There was a problem hiding this comment.
Nothing that needs taking care of until this gets proper review, but typo: should be "redirects"
| @@ -299,7 +299,7 @@ def gift_exchange_to_csv | |||
| csv_array | |||
| end | |||
|
|
|||
|
|
|||
This reverts commit 84ae3a6.
|
Did you mean to add a code change to this? If so, please update the description and the JIRA issue so we don't accidentally merge this. |
|
I did change on the wrong branch, then reverted it but I missed the push that I did. |
|
Okie dokie -- thanks for clearing it up! |
factories/gift_exchange.rb
Outdated
| require 'faker' | ||
|
|
||
| FactoryGirl.define do | ||
| factory :gift_exchange do |
There was a problem hiding this comment.
You should use the factory for gift_exchange added in #2756.
|
This also has merge conflicts, jsyk. |
|
Do you think you can fix the merge conflicts and switch to the factories redsummernight mentioned soon? It would be great to get this merged before doing more Rails things. |
sarken
added
the
Scope: Tests Only
app/models/scheduled_tag_job.rb
Outdated
| @@ -0,0 +1,12 @@ | |||
| class ScheduledTagJob | |||
There was a problem hiding this comment.
Style/FrozenStringLiteralComment: Missing magic comment # frozen_string_literal: true.
| ) | ||
|
|
||
| unless parent_id(object.id, object).match("deleted") | ||
| json_object.merge!( |
There was a problem hiding this comment.
Performance/RedundantMerge: Use json_object[:bookmarkable_join] = {
| ) | ||
|
|
||
| unless parent_id(object.id, object).match("deleted") |
There was a problem hiding this comment.
Performance/RedundantMatch: Use =~ in places where the MatchData returned by #match will not be used.
| :id, :created_at, :bookmarkable_type, :bookmarkable_id, :user_id, | ||
| :notes, :private, :updated_at, :hidden_by_admin, :pseud_id, :rec | ||
| ], | ||
| methods: [:bookmarker, :collection_ids, :with_notes, :bookmarkable_date] |
There was a problem hiding this comment.
Style/SymbolArray: Use %i or %I for an array of symbols.
| root: false, | ||
| except: [:notes_sanitizer_version, :delta], | ||
| methods: [:bookmarker, :collection_ids, :with_notes] | ||
| only: [ |
There was a problem hiding this comment.
Style/SymbolArray: Use %i or %I for an array of symbols.
app/models/pseud.rb
Outdated
| scope :by_byline, lambda {|byline| | ||
| { | ||
| conditions: ['users.login = ? AND pseuds.name = ?', | ||
| scope :by_byline, -> (byline) { |
There was a problem hiding this comment.
Layout/SpaceInLambdaLiteral: Do not use spaces between -> and opening brace in lambda literals
Style/Lambda: Use the lambda method for multiline lambdas.
app/models/pseud.rb
Outdated
| .where( | ||
| pseuds: { id: pseud_ids }, bookmarks: { private: false, hidden_by_admin: false, rec: true } | ||
| ) | ||
| .group('pseuds.id') |
There was a problem hiding this comment.
Layout/DotPosition: Place the . on the previous line, together with the method call receiver.
Layout/MultilineMethodCallIndentation: Use 2 (not 0) spaces for indenting an expression spanning multiple lines.
app/models/pseud.rb
Outdated
| scope :public_rec_count_for, -> (pseud_ids) { | ||
| select('pseuds.id, count(pseuds.id) AS rec_count') | ||
| .joins(:bookmarks) | ||
| .where( |
There was a problem hiding this comment.
Layout/DotPosition: Place the . on the previous line, together with the method call receiver.
Layout/MultilineMethodCallIndentation: Use 2 (not 0) spaces for indenting an expression spanning multiple lines.
app/models/pseud.rb
Outdated
| } | ||
| scope :public_rec_count_for, -> (pseud_ids) { | ||
| select('pseuds.id, count(pseuds.id) AS rec_count') | ||
| .joins(:bookmarks) |
There was a problem hiding this comment.
Layout/DotPosition: Place the . on the previous line, together with the method call receiver.
Layout/MultilineMethodCallIndentation: Use 2 (not 0) spaces for indenting an expression spanning multiple lines.
app/models/pseud.rb
Outdated
| conditions: {bookmarks: {private: false, hidden_by_admin: false, rec: true}, pseuds: {id: pseud_ids}}, | ||
| group: 'pseuds.id' | ||
| } | ||
| scope :public_rec_count_for, -> (pseud_ids) { |
There was a problem hiding this comment.
Layout/SpaceInLambdaLiteral: Do not use spaces between -> and opening brace in lambda literals
Style/Lambda: Use the lambda method for multiline lambdas.
The challenge_signup factory is in fact a gift exchange sign up factory
* Move to exisiting file (and delete the newer one) * Make exisiting tests pass
Context was when sign-ups are open but test was for closed sign-up. Now there's tests for both! Also, less repetition of parameters.
| end | ||
| end | ||
|
|
||
| describe "update" do |
There was a problem hiding this comment.
Metrics/BlockLength: Block has too many lines. [43/25]
| end | ||
|
|
||
| describe "show" do | ||
| xit "redirects and errors if there is no challenge associated with the collection" do |
There was a problem hiding this comment.
This test is ignored, does it pass or do we need a follow-up issue for it?
There was a problem hiding this comment.
It fails with an ActuveRecord not found error and the actual behavior on production seems to be a 404, which I think is what we prefer, so we might want an issue to remove the code for the flash message this test is looking for. However, I'd like someone to double check the test and I are checking the right behavior because I'm a little confused why it's 404ing instead of giving the flash message.
There was a problem hiding this comment.
The name of the test doesn't match the contents of the test -- the invalid ID below is for the challenge signup, not the challenge itself. (Hence the error message, "What sign-up did you want to work on?")
The sign-up is loaded with find instead of find_by, which makes it throw an exception instead of returning nil when there's no record:
otwarchive/app/controllers/challenge_signups_controller.rb
Lines 60 to 63 in 540f520
There was a problem hiding this comment.
Thank you, it was the description that was throwing me off! I've updated that and made an issue.
| fake_login | ||
| get :new, params: { collection_id: plain_collection } | ||
| it_redirects_to_with_error(collection_path(plain_collection), | ||
| "What challenge did you want to sign up for?") |
There was a problem hiding this comment.
Layout/AlignParameters: Align the parameters of a method call if they span more than one line.
| fake_login_known_user(open_signup_owner) | ||
| get :new, params: { collection_id: open_collection.name, pseud: user.pseuds.first } | ||
| it_redirects_to_with_notice(edit_collection_signup_path(open_collection, open_signup), | ||
| "You are already signed up for this challenge. You can edit your sign-up below.") |
There was a problem hiding this comment.
Layout/AlignParameters: Align the parameters of a method call if they span more than one line.
| fake_login | ||
| get :new, params: { collection_id: closed_collection.name } | ||
| it_redirects_to_with_error(collection_path(closed_collection), | ||
| "Sign-up is currently closed: please contact a moderator for help.") |
There was a problem hiding this comment.
Layout/AlignParameters: Align the parameters of a method call if they span more than one line.
redsummernight
added
Reviewed: Ready to Merge
and removed
Coder Has Actioned Review
labels
Issue
https://otwarchive.atlassian.net/browse/AO3-4913
Purpose
Increase coverage of ChallengeSignupsController
Testing
No manual testing required.
challenge_claims_controller currently has 70.11% test coverage on Coveralls:
https://coveralls.io/builds/10550066/source?filename=app%2Fcontrollers%2Fchallenge_signups_controller.rb
We'd like to increase that, with an ultimate goal of 90% or greater.
How to test:
Use Coveralls to confirm that the coverage percentage has increased: