Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

AO3 5008: Tweak subscription validation #2974

Merged
merged 5 commits into from Jul 25, 2017

Conversation

Projects
None yet
4 participants
Owner

elzj commented Jul 25, 2017

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

AO3 5008: Tweak subscription validation to prevent subscriptions to nonsubscribable models.

First, explicitly validate the types of objects you're allowed to subscribe to. Then add a condition to the subscribable validation so that you don't get a 500 error if subscribable_type is not a real model name, allowing failures to have a consistent format.

elzj added some commits Jul 25, 2017

app/models/subscription.rb
@@ -1,10 +1,18 @@
class Subscription < ActiveRecord::Base
include ActiveModel::ForbiddenAttributesProtection
+ VALID_SUBSCRIBABLES = %w(Work User Series)
@houndci-bot

houndci-bot Jul 25, 2017

Freeze mutable objects assigned to constants.

@zz9pzza

zz9pzza Jul 25, 2017

Contributor

I think this is fair

app/models/subscription.rb
+ # Without the condition, you get a 500 error instead of a validation error
+ # if there's an invalid subscribable type
+ validates :subscribable, presence: true,
+ if: Proc.new { |s| VALID_SUBSCRIBABLES.include?(s.subscribable_type) }
@houndci-bot

houndci-bot Jul 25, 2017

Align the elements of a hash literal if they span more than one line.
Use proc instead of Proc.new.

@zz9pzza

zz9pzza Jul 25, 2017

Contributor

And this too.

app/models/subscription.rb
@@ -1,10 +1,18 @@
class Subscription < ActiveRecord::Base
include ActiveModel::ForbiddenAttributesProtection
+ VALID_SUBSCRIBABLES = %w(Work User Series)
@zz9pzza

zz9pzza Jul 25, 2017

Contributor

I think this is fair

app/models/subscription.rb
+ # Without the condition, you get a 500 error instead of a validation error
+ # if there's an invalid subscribable type
+ validates :subscribable, presence: true,
+ if: Proc.new { |s| VALID_SUBSCRIBABLES.include?(s.subscribable_type) }
@zz9pzza

zz9pzza Jul 25, 2017

Contributor

And this too.

elzj added some commits Jul 25, 2017

spec/models/subscription_spec.rb
@@ -63,18 +63,29 @@
end
it "should not save" do
- expect(subscription.save).to be_falsey
@zz9pzza

zz9pzza Jul 25, 2017

Contributor

It now doesn't match the title ?

And the same below.

@sarken sarken merged commit 45daa81 into otwcode:master Jul 25, 2017

3 checks passed

Scrutinizer No new issues
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
hound No violations found. Woof!
Owner

sarken commented Jul 25, 2017

Travis and james are both happy, so merged! Just a quick note -- could you make sure to use the issue template in the future? (It's mostly just the top part with the link that's important, since it makes it easier to go straight to JIRA and make sure the issue status and fix version are correct.)

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