Campaign show page #62
Conversation
coderberry
commented
Nov 27, 2018
coderberry
commented
coderberry
added
in progress
| before_action :set_campaign | ||
|
|
||
| def show | ||
| end |
There was a problem hiding this comment.
I've changed my thinking on having empty actions in controllers. While it does communicate some intent, the routes file is the true canonical source of what actions exist. I see these empty method defs as unnecessary noise... at least for experienced Rails developers. This practice could lead to cruft that we forget to clean up if/when the routes change.
No need to remove right now. I just wanted to communicate my thoughts on this pattern.
There was a problem hiding this comment.
Can you help me understand this better? How would you handle this?
There was a problem hiding this comment.
I'd just omit the empty method. Rails will simply render view template if the route is defined.
There was a problem hiding this comment.
In light of the fact that this is open source, do you think that it may be confusing to less experienced Rails developers?
|
|
||
| def diff | ||
| render layout: false | ||
| end |
There was a problem hiding this comment.
Consider changing to:
# config/routes.rb
resource :diffs, only: [:show]This controller/action now becomes polymorphic in nature and can show diffs on any model with a paper_trail.
| campaign = @version.reify | ||
| campaign.save | ||
| redirect_to campaign_campaign_changelogs_path(campaign) | ||
| end |
There was a problem hiding this comment.
Consider changing to:
# config/routes.rb
resource :rollbacks, only: [:create]This controller/action now becomes polymorphic in nature and can support rollbacks for any model with a paper_trail.
| def set_campaign | ||
| @campaign = Campaign.find(params[:campaign_id]) | ||
| end | ||
| end |
There was a problem hiding this comment.
I think we could likely have a polymorphic comments controller and a single set of views that work for any model we give acts_as_commentable_with_threading behavior to.
# config/routes.rb
resources :campaigns do
resources :comments
end
app/models/campaign.rb
Outdated
| end | ||
|
|
||
| def remaining_days | ||
| return 0 unless start_date.present? && end_date.present? |
There was a problem hiding this comment.
Could reuse total_days impl here.
return 0 if total_days.zero?| return 0 if start_date > Date.current | ||
| return 100 if end_date < Date.current | ||
|
|
||
| ((remaining_days.to_f / total_days.to_f) * 100).to_i |
There was a problem hiding this comment.
See comment above about divide by zero. Note the first guard could/should be replaced with the total_days impl.
| @@ -37,6 +37,7 @@ class Impression < ApplicationRecord | |||
|
|
|||
| # callbacks ................................................................. | |||
| # scopes .................................................................... | |||
| scope :clicked, -> { where.not(clicked_at: nil) } | |||
There was a problem hiding this comment.
We don't current have an index to optimize this query. How often will we ask for only clicks vs all impressions then determining clicks from that set? Is this something we should optimize for?
| <a data-controller="remote-modal" | ||
| data-action="click->remote-modal#displayModal" | ||
| data-url="<%= diff_campaign_campaign_changelog_path(@campaign, version) %>" | ||
| href="#"> |
config/initializers/paper_trail.rb
Outdated
| @@ -0,0 +1,2 @@ | |||
| PaperTrail.config.enabled = true | |||
| PaperTrail.config.version_limit = 3 | |||
There was a problem hiding this comment.
Is 3 enough for version_limit? I thought we wanted to target 25.
config/routes.rb
Outdated
| member do | ||
| get :diff, to: "campaign_changelogs#diff" | ||
| patch :rollback, to: "campaign_changelogs#rollback" | ||
| end |
There was a problem hiding this comment.
| resource :campaign_targeting, only: [:show], path: "/targeting" | ||
| resource :campaign_budget, only: [:show], path: "/budget" | ||
| resources :campaign_properties, only: [:index], path: "/properties" | ||
| end |
There was a problem hiding this comment.
These routes are beautiful.
| resources :versions, only: [:index], as: :property_versions | ||
| resources :comments, only: [:index], as: :property_comments | ||
| end | ||
|
|
| negative_keywords: negative_keywords, | ||
| status: ["active"], | ||
| }).apply(Property.order(name: :asc)) | ||
| end |
There was a problem hiding this comment.
I might rename this method to matching_properties. What do you think?
| def average_ctr | ||
| 0.35 | ||
| end | ||
|
|
There was a problem hiding this comment.
These methods can be replaced with my branch when it lands.
app/models/comment.rb
Outdated
| # commentable class name and commentable id. | ||
| scope :find_comments_for_commentable, lambda { |commentable_str, commentable_id| | ||
| where(commentable_type: commentable_str.to_s, commentable_id: commentable_id).order("created_at DESC") | ||
| } |
There was a problem hiding this comment.
Love these scopes. I'd probably remove the prefix of find_comments_ as its somewhat redundant and I think the semantics will read better.
Consider this usage:
Comment.by_user(user)
# ~vs~
Comment.find_comments_by_user(user)Also, switch to stabby lambda -> for scopes.
There was a problem hiding this comment.
These are gem-generated files
| keywords | ||
| prohibited_advertisers | ||
| prohibit_fallback_campaigns | ||
| ] |
There was a problem hiding this comment.
I might consider sorting these types of lists. Sorting makes the semantics worse, but it also helps speed up scanning for a value and spotting dupes in the list.
| Popper: 'popper.js', | ||
| Chartist: 'Chartist', | ||
| Typed: 'Typed', | ||
| SVGInjector: 'SVGInjector', |
There was a problem hiding this comment.
Are we using SVGInjector or was that experimental?
There was a problem hiding this comment.
We do use this. It's part of the theme.
# Conflicts: # app/models/impression.rb # db/structure.sql
