Refactor user model (publiclab#4894)

* Remove inherited method

* Break query to multilines, easier for git blame

* Remove puts from method

* Make use of the contants set

* Better constants store/access

* Fix rubocop offences

* Use Arel for input

* Refactor

* Rename variable

* Use right constants in right places

* Use present?

* Revert to > 1

app/controllers/application_controller.rb

@@ -136,7 +136,7 @@ def alert_and_redirect_moderated
       flash.now[:warning] = "First-time poster <a href='/profile/#{@node.author.name}'>#{@node.author.name}</a> submitted this #{time_ago_in_words(@node.created_at)} ago and it has not yet been approved by a moderator. <a class='btn btn-default btn-sm' href='/moderate/publish/#{@node.id}'>Approve</a> <a class='btn btn-default btn-sm' href='/moderate/spam/#{@node.id}'>Spam</a>"
     elsif @node.status == 4 && (current_user && current_user.id == @node.author.id) && !flash[:first_time_post]
       flash.now[:warning] = "Thank you for contributing open research, and thanks for your patience while your post is approved by <a href='/wiki/moderation'>community moderators</a> and we'll email you when it is published. In the meantime, if you have more to contribute, feel free to do so."
-    elsif @node.status == 3 && (current_user && (current_user.is_coauthor(@node) || current_user.can_moderate?)) && !flash[:first_time_post]
+    elsif @node.status == 3 && (current_user && (current_user.is_coauthor?(@node) || current_user.can_moderate?)) && !flash[:first_time_post]
       flash.now[:warning] = "This is a draft note. Once you're ready, click <a class='btn btn-success btn-xs' href='/notes/publish_draft/#{@node.id}'>Publish Draft</a> to make it public. You can share it with collaborators using this private link <a href='#{@node.draft_url}'>#{@node.draft_url}</a>"
     elsif @node.status != 1 && @node.status != 3 && !(current_user && (current_user.role == 'admin' || current_user.role == 'moderator'))
       # if it's spam or a draft

app/models/comment.rb

@@ -101,11 +101,7 @@ def next_thread
   end
 
   def parent
-    if aid == 0
-      node
-    else
-      return answer.node unless answer.nil?
-    end
+    aid.zero? ? node : answer&.node
   end
 
   def mentioned_users
@@ -370,15 +366,14 @@ def trimmed_content?
   end
 
   def parse_quoted_text
-    match = body.match(/(.+)(On .+<.+@.+> wrote:)(.+)/m)
-    if match.nil?
-      false
-    else
+    if regex_match = body.match(/(.+)(On .+<.+@.+> wrote:)(.+)/m)
       {
-        body: match[1], # the new message text
-        boundary: match[2], # quote delimeter, i.e. "On Tuesday, 3 July 2018, 11:20:57 PM IST, RP <rp@email.com> wrote:"
-        quote: match[3] # quoted text from prior email chain
+        body: regex_match[1],     # The new message text
+        boundary: regex_match[2], # Quote delimeter, i.e. "On Tuesday, 3 July 2018, 11:20:57 PM IST, RP <rp@email.com> wrote:"
+        quote: regex_match[3]     # Quoted text from prior email chain
       }
+    else
+      {}
     end
   end
 
@@ -394,7 +389,7 @@ def render_body
     # if it has quoted email text that wasn't caught by the yahoo and gmail filters,
     # manually insert the comment filter delimeter:
     parsed = parse_quoted_text
-    if !trimmed_content? && parsed != false
+    if !trimmed_content? && parsed.present?
       body = parsed[:body] + COMMENT_FILTER + parsed[:boundary] + parsed[:quote]
     end
     body

app/models/revision.rb

@@ -20,9 +20,9 @@ class Revision < ApplicationRecord
   before_create :setup
 
   scope :published, -> { where(status: 1) }
-  scope :past_week, -> { where("timestamp > ?", (Time.now - 7.days).to_i) }
-  scope :past_month, -> { where("timestamp > ?", (Time.now - 1.months).to_i) }
-  scope :past_year, -> { where("timestamp > ?", (Time.now - 1.years).to_i) }
+  scope :past_week, -> { where("timestamp > ?", 7.days.ago) }
+  scope :past_month, -> { where("timestamp > ?", 1.month.ago) }
+  scope :past_year, -> { where("timestamp > ?", 1.year.ago) }
 
   def setup
     self.teaser = ''

app/models/user.rb

@@ -12,9 +12,13 @@ class User < ActiveRecord::Base
   self.table_name = 'rusers'
   alias_attribute :name, :username
 
-  NORMAL = 1 # Usage: User::NORMAL
-  BANNED = 0 # Usage: User::BANNED
-  MODERATED = 5 # Usage: User::MODERATED
+  module Status
+    VALUES = [
+      NORMAL = 1,   # Usage: Status::NORMAL
+      BANNED = 0,   # Usage: Status::BANNED
+      MODERATED = 5 # Usage: Status::MODERATED
+    ].freeze
+  end
 
   attr_readonly :username
 
@@ -60,12 +64,12 @@ def self.search_by_username(query)
     User.where('MATCH(username) AGAINST(? IN BOOLEAN MODE)', query + '*')
   end
 
-  def is_new_contributor
-    Node.where(uid: id).length === 1 && Node.where(uid: id).first.created_at > Date.today - 1.month
+  def is_new_contributor?
+    Node.where(uid: id).length === 1 && Node.where(uid: id).first.created_at > 1.month.ago
   end
 
   def new_contributor
-    return "<a href='/tag/first-time-poster' class='label label-success'><i>new contributor</i></a>".html_safe if is_new_contributor
+    return "<a href='/tag/first-time-poster' class='label label-success'><i>new contributor</i></a>".html_safe if is_new_contributor?
   end
 
   def set_token
@@ -135,7 +139,7 @@ def can_moderate?
     admin? || moderator?
   end
 
-  def is_coauthor(node)
+  def is_coauthor?(node)
     id == node.author.id || node.has_tag("with:#{username}")
   end
 
@@ -206,11 +210,11 @@ def photo_path(size = :medium)
   end
 
   def first_time_poster
-    notes.where(status: 1).count == 0
+    notes.where(status: 1).count.zero?
   end
 
   def first_time_commenter
-    Comment.where(status: 1, uid: uid).count == 0
+    Comment.where(status: 1, uid: uid).count.zero?
   end
 
   def follow(other_user)
@@ -227,7 +231,6 @@ def following?(other_user)
 
   def profile_image
     if photo_file_name
-      puts photo_path(:thumb)
       photo_path(:thumb)
     else
       "https://www.gravatar.com/avatar/#{OpenSSL::Digest::MD5.hexdigest(email)}"
@@ -264,35 +267,35 @@ def social_link(site)
   end
 
   def moderate
-    self.status = 5
-    self.save
+    self.status = Status::MODERATED
+    save
     # user is logged out next time they access current_user in a controller; see application controller
     self
   end
 
   def unmoderate
-    self.status = 1
-    self.save
+    self.status = Status::NORMAL
+    save
     self
   end
 
   def ban
     decrease_likes_banned
-    self.status = 0
-    self.save
+    self.status = Status::BANNED
+    save
     # user is logged out next time they access current_user in a controller; see application controller
     self
   end
 
   def unban
     increase_likes_unbanned
-    self.status = 1
-    self.save
+    self.status = Status::NORMAL
+    save
     self
   end
 
   def banned?
-    status.zero?
+    status == Status::BANNED
   end
 
   def note_count
@@ -306,7 +309,10 @@ def node_count
   def liked_notes
     Node.includes(:node_selections)
       .references(:node_selections)
-      .where("type = 'note' AND node_selections.liking = ? AND node_selections.user_id = ? AND node.status = 1", true, id)
+      .where("type = 'note' AND \
+              node_selections.liking = ? \
+              AND node_selections.user_id = ? \
+              AND node.status = 1", true, id)
       .order('node_selections.nid DESC')
   end
 
@@ -319,8 +325,8 @@ def liked_pages
   end
 
   def send_digest_email
-    top_picks = content_followed_in_period(Time.now - 1.week, Time.now)
-    if top_picks.count > 0
+    top_picks = content_followed_in_period(1.week.ago, Time.now)
+    if top_picks.count.positive?
       SubscriptionMailer.send_digest(id, top_picks).deliver_now
     end
   end
@@ -348,7 +354,6 @@ def self.validate_token(token)
     begin
       decrypted_data = User.decrypt(token)
     rescue ActiveSupport::MessageVerifier::InvalidSignature => e
-      puts e.message
       return 0
     end
     if (Time.now - decrypted_data[:timestamp]) / 1.hour > 24.0
@@ -362,14 +367,14 @@ def self.validate_token(token)
 
   def decrease_likes_banned
     node_selections.each do |selection|
-      selection.node.cached_likes = selection.node.cached_likes - 1
+      selection.node.cached_likes -= 1
       selection.node.save!
     end
   end
 
   def increase_likes_unbanned
     node_selections.each do |selection|
-      selection.node.cached_likes = selection.node.cached_likes + 1
+      selection.node.cached_likes += 1
       selection.node.save!
     end
   end
@@ -382,7 +387,11 @@ def map_openid_registration(registration)
   def self.watching_location(nwlat, selat, nwlng, selng)
     raise("Must be a float") unless (nwlat.is_a? Float) && (nwlng.is_a? Float) && (selat.is_a? Float) && (selng.is_a? Float)
 
-    tids = Tag.where("SUBSTRING_INDEX(term_data.name,':',1) = ? AND SUBSTRING_INDEX(SUBSTRING_INDEX(term_data.name, ':', 2),':',-1)+0 <= ? AND SUBSTRING_INDEX(SUBSTRING_INDEX(term_data.name, ':', 3),':',-1)+0 <= ? AND SUBSTRING_INDEX(SUBSTRING_INDEX(term_data.name, ':', 4),':',-1)+0 <= ? AND SUBSTRING_INDEX(term_data.name, ':', -1) <= ?", 'subscribed', nwlat, nwlng, selat, selng).collect(&:tid).uniq || []
+    tids = Tag.where("SUBSTRING_INDEX(term_data.name,':',1) = ? \
+                      AND SUBSTRING_INDEX(SUBSTRING_INDEX(term_data.name, ':', 2),':',-1)+0 <= ? \
+                      AND SUBSTRING_INDEX(SUBSTRING_INDEX(term_data.name, ':', 3),':',-1)+0 <= ? \
+                      AND SUBSTRING_INDEX(SUBSTRING_INDEX(term_data.name, ':', 4),':',-1)+0 <= ? \
+                      AND SUBSTRING_INDEX(term_data.name, ':', -1) <= ?", 'subscribed', nwlat, nwlng, selat, selng).collect(&:tid).uniq || []
     uids = TagSelection.where('tag_selections.tid IN (?)', tids).collect(&:user_id).uniq || []
 
     User.where("id IN (?)", uids).order(:id)
@@ -415,7 +424,7 @@ def self.create_with_omniauth(auth)
       user.username = email_prefix
       user.email = auth["info"]["email"]
       user.password = s
-      user.status = 1
+      user.status = Status::NORMAL
       user.password_confirmation = s
       user.password_checker = hash[auth["provider"]]
       user.save!
@@ -430,13 +439,4 @@ def self.count_all_time_contributor
     revisions = Revision.where(status: 1).pluck(:uid)
     contributors = (notes + answers + questions + comments + revisions).compact.uniq.length
   end
-
-  def self.to_csv(options = {})
-    CSV.generate(options) do |csv|
-      csv << column_names
-      all.each do |object|
-        csv << object.attributes.values_at(*column_names)
-      end
-    end
-  end
 end

app/services/search_service.rb

@@ -133,7 +133,7 @@ def tagNearbyNodes(coordinates, tag, period = { "from" => nil, "to" => nil }, so
               items.order("changed #{order_direction}")
                    .limit(limit)
             else
-              items.order("created #{order_direction}")
+              items.order(Arel.sql("created #{order_direction}"))
                    .limit(limit)
             end
   end

test/unit/node_test.rb

@@ -193,7 +193,8 @@ class NodeTest < ActiveSupport::TestCase
 
   test 'latest revision based on timestamp' do
     node = nodes(:spam_targeted_page)
-    assert node.revisions.count > 1
+
+    assert node.revisions.size > 1
     assert_equal node.revisions.first, node.latest
     assert node.revisions.first.timestamp.to_i > node.revisions.last.timestamp.to_i
     assert_not_equal node.revisions.last, node.latest
@@ -202,9 +203,12 @@ class NodeTest < ActiveSupport::TestCase
 
   test 'latest revision not a moderated revision' do
     node = nodes(:spam_targeted_page)
-    assert node.revisions.count > 1
+
+    assert node.revisions.size > 1
     assert_equal node.revisions.first, node.latest
+
     node.latest.spam
+
     assert_not_equal node.revisions.first, node.latest
     assert_equal 1, node.latest.status
   end