If you just want the solution, skip down to Attempt Three.
Attempt One - Overwrite the share_user's approved=(value) method
New approved=(value) method:
def approved=(value)
share = self.share
if value == true
if share.share_users.where("approved = true").count < share.user_limit
write_attribute(:approved, value)
else
raise Exceptions::ShareUserLimitReached
end
end
if value == false
write_attribute(:approved, value)
end
end
Supporting share_user spec:
describe "approved=(value) > " do
it {should respond_to :approved=}
let(:approved_share) {FactoryGirl.create(:basic_share, user_limit: 1)}
let!(:approved_share_user) {FactoryGirl.create(:share_user, share_id: approved_share.id, approved: true)}
let!(:disapproved_share_user) {FactoryGirl.create(:share_user, share_id: approved_share.id, approved: false)}
it "should not throw a ShareUserLimitReached error when the share's user_limit is not being exceeded" do
expect{share_user.approved=true}.not_to raise_error
end
it "should throw a ShareUserLimitReached error when the share's user_limit attempts to be exceeded" do
approved_share.user_limit = 2
approved_share.save
expect{disapproved_share_user.approved=true}.to raise_error Exceptions::ShareUserLimitReached
end
it "should allow share_users to be disapproved without issue" do
expect{approved_share_user.approved=false}.not_to raise_error
end
end
Now, if you look at the code, you'll see a couple of things.
- share_user#approved=(value) is not encapsulated. It's reaching through to the share, retrieving attributes from it and then executing upon those attributes.
- share_user#approved=(value) raises a nasty error that then has to be handled specially everywhere it is likely to be invoked.
- share_user#approved=(value) heavily complicates creating new share_users as it requires the share_user to already have a share at point of creation. This requires telling the relationship to retrieve it from memory as opposed to the database. More on this can be found here.
All in all, not a great solution, it's messy, doesn't quite do the job and requires modifying a lot of other code.
Attempt Two - validates_associated + validates_with
- Create a new validation for the Share called ApprovedShareUserCount and invoke it with validates_with.
- Invoke this new validation whenever a share_user is saved by adding 'validates_associated :share' to it.
class ApprovedShareUserCountValidator < ActiveModel::Validator
def validate(record)
#Required to ensure that the presence validation spec for user_limit doesn't trigger this validation to throw a nil error.
return if record.user_limit.nil?
count = record.share_users.where("approved = true").count
if count > record.user_limit
record.errors.add(:share_users, 'The share has reached its user limit, please contact your CEO or Tech Admin about this issue.')
end
end
end
class Share < ActiveRecord::Base
has_many :share_users, foreign_key: "share_id", dependent: :destroy
validates :user_limit, presence: true
validates_with ApprovedShareUserCountValidator
end
class ShareUser < ActiveRecord::Base
belongs_to :share
validates :share_id, presence: true
validates_associated :share
end
describe "should ensure the user_limit is never exceeded" do
let!(:user_limit_share) {FactoryGirl.create(:share, user_limit: 1)}
let!(:approved_share_user) {FactoryGirl.create(:share_user, approved: true, share_id: user_limit_share.id)}
let!(:disapproved_share_user) {FactoryGirl.create(:share_user, approved: false, share_id: user_limit_share.id)}
subject{user_limit_share}
it "should be valid if the number of approved share_users is <= the share's user_limit" do
should be_valid
end
end
Supporting Share_User spec
describe "validate_association :share" do
let!(:user_limit_share) {FactoryGirl.create(:share, user_limit: 1)}
let!(:approved_share_user) {FactoryGirl.create(:share_user, approved: true, share_id: user_limit_share.id)}
let!(:disapproved_share_user) {FactoryGirl.create(:share_user, approved: false, share_id: user_limit_share.id)}
it "should not be valid if approving it would exceed the share's user_limit" do
disapproved_share_user.approved = true
expect(disapproved_share_user.valid?).to_not be_true
end
end
The allure of this is that it ties into the existing validation framework. Unfortunately, there is one critical problem; it doesn't work. The validations all trigger and work perfectly fine! But the share's validation is triggered against the database, and does NOT use the share_user in memory that triggered it.
So to use this solution, the share_user would have to be saved to the database, thus defeating the entire purpose.
Attempt Three - Share#respect_share?(share_user) + ShareUser#validates_with custom validator
Attempt two was definitely far more elegant then attempt one and was certainly moving in the right direction. So how do we build on that with something that respects encapsulation, can be easily expanded on and that uses the existing validation framework?
- Create a public method on Share that determines if a given share_user would violate the share's rules.
- Create a custom validator for share_user that invokes the owning share's respect_share? method.
def respect_share?(share_user)
#Ensure the share's user_limit is respected.
if share_user.approved == true
count = self.share_users.where("approved = true").count
#This assumes that in the event this returns true, the share_user will be added to the share.
#Thus it must count up, so that a share with 10 users and a 10 user limit will return false.
if count+1<=self.user_limit
return true
else
return false
end
end
#This method can be expanded as the share grows.
end
class RespectShareValidator < ActiveModel::Validator
def validate(record)
#Required to ensure that the presence validation spec for share_id doesn't trigger this validation to throw a nil error.
return if record.share_id.nil?
#This simply calls the record's owning share and then passes the record to it.
if record.share.respect_share?(record) == false
record.errors.add(:share_users, '<Error Message Here>')
end
end
end
Supporting Share Spec
describe "Public Methods > " do
describe "respect_share?(share_user)" do
let!(:user_limit_share) {FactoryGirl.create(:share, user_limit: 1)}
let!(:approved_share_user) {FactoryGirl.create(:share_user, approved: true, share_id: user_limit_share.id)}
let!(:disapproved_share_user) {FactoryGirl.create(:share_user, approved: false, share_id: user_limit_share.id)}
it "should return false if the share's user_limit would be exceeded by saving the passed in share_user" do
disapproved_share_user.approved = true
expect(user_limit_share.respect_share?(disapproved_share_user)).to be false
end
end
end
Supporting Share_User Spec
describe "RespectShareValidator" do
let!(:user_limit_share) {FactoryGirl.create(:share, user_limit: 1)}
let!(:approved_share_user) {FactoryGirl.create(:share_user, approved: true, share_id: user_limit_share.id)}
let!(:disapproved_share_user) {FactoryGirl.create(:share_user, approved: false, share_id: user_limit_share.id)}
it "should not be valid if approving it would exceed the share's user_limit" do
disapproved_share_user.approved = true
expect(disapproved_share_user.valid?).to_not be_true
end
end
Unlike Attempt Two this one actually works. This is because the current in-memory representation of the share_user is passed to Share#respect_share? and the method knows that it is dealing with an in memory model instead of a database record. Now while this knowledge isn't ideal, it is much cleaner and functional then either of the other two attempts.
The existing validation mechanisms are used, and there are no custom errors to be caught. This means that the majority of existing code will function without issue. On top of this, because all of the code necessary to determine if a Share would be broken is contained within the Share class itself, this solution doesn't violate encapsulation.
No comments:
Post a Comment