Feature - added percentage and action type attributes #1507
Conversation
app/models/page.rb
Outdated
# So for page which has petition and followup as donation | ||
# the page is considered as petition page. | ||
def donation_page? | ||
plugin_names.include?('fundraiser') && !plugin_names.include?('petition') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about just using plugins.first.is_a?(Plugins::Fundraiser)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@osahyoun
Seems promising. Will update the code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@osahyoun
Is there a chance fundraiser is created first before petition plugin?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question. I don't think so.
spec/models/page_spec.rb
Outdated
describe '#donation_page?' do | ||
before do | ||
@page = create :page # , liquid_layout: donation | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally, we use the helper method, let
.
let(:page) { create(:page) }
spec/models/page_spec.rb
Outdated
end | ||
|
||
it 'should return false' do | ||
[create(:plugins_petition, page: @page), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why are these statements in an array?
spec/models/page_spec.rb
Outdated
|
||
it 'should return false' do | ||
[create(:call_tool, page: @page)] | ||
expect(@page.donation_page?).to be_falsey |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
be_falsey
is the wrong matcher. I think you'll want to use to be false
, as the method returns a boolean.
app/models/page.rb
Outdated
@@ -135,6 +135,17 @@ def image_to_display | |||
primary_image || images.first | |||
end | |||
|
|||
def plugin_thermometers | |||
plugins.collect do |x| | |||
x if ['Plugins::ActionsThermometer', 'Plugins::DonationsThermometer'].include?(x.class.to_s) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better to use kind_of?
or is_a?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@osahyoun
does using member?
is fine?
[Plugins::ActionsThermometer, Plugins::DonationsThermometer].member?(x.class)
if we use kind_of? and Is_a? it ends
Plugins::ActionsThermometer.is_a?(x.class) || Plugins::DonationsThermometer.is_a?(x.class)
Shall I use the member? or using the || operator?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
app/models/page.rb
Outdated
|
||
def plugin_thermometer_data | ||
thermometer = donation_page? ? 'DonationsThermometer' : 'ActionsThermometer' | ||
plugin_thermometers.select { |x| x.type == thermometer }.first.try(:liquid_data) || {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above (is_a?
or kind_of?
)
💯 looks good. just a few minor comments on style. |
Ticket:
https://app.asana.com/0/1119677445805032/1140485727078297