diff options
| -rw-r--r-- | app/models/about_me_validator.rb | 2 | ||||
| -rw-r--r-- | app/models/change_email_validator.rb | 45 | ||||
| -rw-r--r-- | app/models/comment.rb | 66 | ||||
| -rw-r--r-- | app/models/contact_validator.rb | 4 | ||||
| -rw-r--r-- | app/models/holiday.rb | 24 | ||||
| -rw-r--r-- | app/models/mail_server_log_done.rb | 3 | ||||
| -rw-r--r-- | app/models/post_redirect.rb | 78 | ||||
| -rw-r--r-- | app/models/profile_photo.rb | 54 | ||||
| -rw-r--r-- | app/models/public_body.rb | 39 | ||||
| -rw-r--r-- | app/models/purge_request.rb | 19 | ||||
| -rw-r--r-- | app/models/raw_email.rb | 32 | ||||
| -rw-r--r-- | app/models/track_thing.rb | 410 | ||||
| -rw-r--r-- | app/models/user.rb | 356 | ||||
| -rw-r--r-- | app/models/user_info_request_sent_alert.rb | 24 | ||||
| -rw-r--r-- | spec/models/change_email_validator_spec.rb | 124 |
15 files changed, 716 insertions, 564 deletions
diff --git a/app/models/about_me_validator.rb b/app/models/about_me_validator.rb index 7df70fb61..8c24cfd67 100644 --- a/app/models/about_me_validator.rb +++ b/app/models/about_me_validator.rb @@ -21,7 +21,7 @@ class AboutMeValidator private def length_of_about_me - if !self.about_me.blank? && self.about_me.size > 500 + if !about_me.blank? && about_me.size > 500 errors.add(:about_me, _("Please keep it shorter than 500 characters")) end end diff --git a/app/models/change_email_validator.rb b/app/models/change_email_validator.rb index 5cc13d4c2..7ee6654bb 100644 --- a/app/models/change_email_validator.rb +++ b/app/models/change_email_validator.rb @@ -7,11 +7,22 @@ class ChangeEmailValidator include ActiveModel::Validations - attr_accessor :old_email, :new_email, :password, :user_circumstance, :logged_in_user + attr_accessor :old_email, + :new_email, + :password, + :user_circumstance, + :logged_in_user + + validates_presence_of :old_email, + :message => N_("Please enter your old email address") + + validates_presence_of :new_email, + :message => N_("Please enter your new email address") + + validates_presence_of :password, + :message => N_("Please enter your password"), + :unless => :changing_email - validates_presence_of :old_email, :message => N_("Please enter your old email address") - validates_presence_of :new_email, :message => N_("Please enter your new email address") - validates_presence_of :password, :message => N_("Please enter your password"), :unless => :changing_email validate :password_and_format_of_email def initialize(attributes = {}) @@ -20,7 +31,6 @@ class ChangeEmailValidator end end - def changing_email self.user_circumstance == 'change_email' end @@ -28,22 +38,33 @@ class ChangeEmailValidator private def password_and_format_of_email - if !self.old_email.blank? && !MySociety::Validate.is_valid_email(self.old_email) - errors.add(:old_email, _("Old email doesn't look like a valid address")) - end + check_email_is_present_and_valid(:old_email) if errors[:old_email].blank? - if self.old_email.downcase != self.logged_in_user.email.downcase + if !email_belongs_to_user?(old_email) errors.add(:old_email, _("Old email address isn't the same as the address of the account you are logged in with")) - elsif (!self.changing_email) && (!self.logged_in_user.has_this_password?(self.password)) + elsif !changing_email && !correct_password? if errors[:password].blank? errors.add(:password, _("Password is not correct")) end end end - if !self.new_email.blank? && !MySociety::Validate.is_valid_email(self.new_email) - errors.add(:new_email, _("New email doesn't look like a valid address")) + check_email_is_present_and_valid(:new_email) + end + + def check_email_is_present_and_valid(email) + if !send(email).blank? && !MySociety::Validate.is_valid_email(send(email)) + errors.add(email, _("#{ email.to_s.humanize } doesn't look like a valid address")) end end + + def email_belongs_to_user?(email) + email.downcase == logged_in_user.email.downcase + end + + def correct_password? + logged_in_user.has_this_password?(password) + end + end diff --git a/app/models/comment.rb b/app/models/comment.rb index a62c086d5..a286aa1f5 100644 --- a/app/models/comment.rb +++ b/app/models/comment.rb @@ -28,15 +28,32 @@ class Comment < ActiveRecord::Base #validates_presence_of :user # breaks during construction of new ones :( validates_inclusion_of :comment_type, :in => [ 'request' ] - validate :body_of_comment + validate :check_body_has_content, + :check_body_uses_mixed_capitals + + after_save :event_xapian_update + + # When posting a new comment, use this to check user hasn't double + # submitted. + def self.find_existing(info_request_id, body) + # TODO: can add other databases here which have regexp_replace + if ActiveRecord::Base.connection.adapter_name == "PostgreSQL" + # Exclude spaces from the body comparison using regexp_replace + regex_replace_sql = "regexp_replace(body, '[[:space:]]', '', 'g') = regexp_replace(?, '[[:space:]]', '', 'g')" + Comment.where(["info_request_id = ? AND #{ regex_replace_sql }", info_request_id, body ]).first + else + # For other databases (e.g. SQLite) not the end of the world being + # space-sensitive for this check + Comment.where(:info_request_id => info_request_id, :body => body).first + end + end def body ret = read_attribute(:body) - if ret.nil? - return ret - end + return ret if ret.nil? ret = ret.strip - ret = ret.gsub(/(?:\n\s*){2,}/, "\n\n") # remove excess linebreaks that unnecessarily space it out + # remove excess linebreaks that unnecessarily space it out + ret = ret.gsub(/(?:\n\s*){2,}/, "\n\n") ret end @@ -45,48 +62,39 @@ class Comment < ActiveRecord::Base end # So when takes changes it updates, or when made invisble it vanishes - after_save :event_xapian_update def event_xapian_update - for event in self.info_request_events - event.xapian_mark_needs_index - end + info_request_events.each { |event| event.xapian_mark_needs_index } end # Return body for display as HTML def get_body_for_html_display - text = self.body.strip + text = body.strip text = CGI.escapeHTML(text) text = MySociety::Format.make_clickable(text, :contract => 1) text = text.gsub(/\n/, '<br>') - return text.html_safe - end - - # When posting a new comment, use this to check user hasn't double submitted. - def Comment.find_existing(info_request_id, body) - # TODO: can add other databases here which have regexp_replace - if ActiveRecord::Base.connection.adapter_name == "PostgreSQL" - # Exclude spaces from the body comparison using regexp_replace - return Comment.find(:first, :conditions => [ "info_request_id = ? and regexp_replace(body, '[[:space:]]', '', 'g') = regexp_replace(?, '[[:space:]]', '', 'g')", info_request_id, body ]) - else - # For other databases (e.g. SQLite) not the end of the world being space-sensitive for this check - return Comment.find(:first, :conditions => [ "info_request_id = ? and body = ?", info_request_id, body ]) - end + text.html_safe end def for_admin_column self.class.content_columns.each do |column| - yield(column.human_name, self.send(column.name), column.type.to_s, column.name) + yield(column.human_name, send(column.name), column.type.to_s, column.name) end end - private + private - def body_of_comment - if self.body.empty? || self.body =~ /^\s+$/ + def check_body_has_content + if body.empty? || body =~ /^\s+$/ errors.add(:body, _("Please enter your annotation")) end - if !MySociety::Validate.uses_mixed_capitals(self.body) - errors.add(:body, _('Please write your annotation using a mixture of capital and lower case letters. This makes it easier for others to read.')) + end + + def check_body_uses_mixed_capitals + unless MySociety::Validate.uses_mixed_capitals(body) + msg = 'Please write your annotation using a mixture of capital and ' \ + 'lower case letters. This makes it easier for others to read.' + errors.add(:body, _(msg)) end end + end diff --git a/app/models/contact_validator.rb b/app/models/contact_validator.rb index e9a6e491c..8d7e4ff08 100644 --- a/app/models/contact_validator.rb +++ b/app/models/contact_validator.rb @@ -24,6 +24,8 @@ class ContactValidator private def email_format - errors.add(:email, _("Email doesn't look like a valid address")) unless MySociety::Validate.is_valid_email(self.email) + unless MySociety::Validate.is_valid_email(email) + errors.add(:email, _("Email doesn't look like a valid address")) + end end end diff --git a/app/models/holiday.rb b/app/models/holiday.rb index 3076cc0fd..4c4941589 100644 --- a/app/models/holiday.rb +++ b/app/models/holiday.rb @@ -22,15 +22,15 @@ class Holiday < ActiveRecord::Base - def Holiday.holidays - @@holidays ||= self.all.collect { |h| h.day }.to_set + def self.holidays + @@holidays ||= all.collect { |h| h.day }.to_set end - def Holiday.weekend_or_holiday?(date) + def self.weekend_or_holiday?(date) date.wday == 0 || date.wday == 6 || Holiday.holidays.include?(date) end - def Holiday.due_date_from(start_date, days, type_of_days) + def self.due_date_from(start_date, days, type_of_days) case type_of_days when "working" Holiday.due_date_from_working_days(start_date, days) @@ -44,14 +44,14 @@ class Holiday < ActiveRecord::Base # Calculate the date on which a request made on a given date falls due when # days are given in working days # i.e. it is due by the end of that day. - def Holiday.due_date_from_working_days(start_date, working_days) + def self.due_date_from_working_days(start_date, working_days) # convert date/times into dates start_date = start_date.to_date - # Count forward the number of working days. We start with today as "day zero". The - # first of the full working days is the next day. We return the - # date of the last of the number of working days. - + # Count forward the number of working days. We start with today as "day + # zero". The first of the full working days is the next day. We return + # the date of the last of the number of working days. + # # This response for example of a public authority complains that we had # it wrong. We didn't (even thought I changed the code for a while, # it's changed back now). A day is a day, our lawyer tells us. @@ -71,9 +71,9 @@ class Holiday < ActiveRecord::Base # Calculate the date on which a request made on a given date falls due when # the days are given in calendar days (rather than working days) - # If the due date falls on a weekend or a holiday then the due date is the next - # weekday that isn't a holiday. - def Holiday.due_date_from_calendar_days(start_date, days) + # If the due date falls on a weekend or a holiday then the due date is the + # next weekday that isn't a holiday. + def self.due_date_from_calendar_days(start_date, days) # convert date/times into dates start_date = start_date.to_date diff --git a/app/models/mail_server_log_done.rb b/app/models/mail_server_log_done.rb index 222b072c5..1bbb23ac4 100644 --- a/app/models/mail_server_log_done.rb +++ b/app/models/mail_server_log_done.rb @@ -17,6 +17,3 @@ class MailServerLogDone < ActiveRecord::Base has_many :mail_server_logs end - - - diff --git a/app/models/post_redirect.rb b/app/models/post_redirect.rb index 6f288b471..8049349d0 100644 --- a/app/models/post_redirect.rb +++ b/app/models/post_redirect.rb @@ -31,66 +31,66 @@ class PostRedirect < ActiveRecord::Base # Optional, does a login confirm before redirect for use in email links. belongs_to :user - after_initialize :generate_token + after_initialize :generate_token, + :generate_email_token + + # Makes a random token, suitable for using in URLs e.g confirmation + # messages. + def self.generate_random_token + MySociety::Util.generate_token + end + + # Used by (rspec) test code only + def self.get_last_post_redirect + # TODO: yeuch - no other easy way of getting the token so we can check + # the redirect URL, as it is by definition opaque to the controller + # apart from in the place that it redirects to. + post_redirects = PostRedirect.find_by_sql("select * from post_redirects order by id desc limit 1") + post_redirects.size.should == 1 + post_redirects[0] + end + + # Called from cron job delete-old-things + def self.delete_old_post_redirects + PostRedirect.delete_all("updated_at < (now() - interval '2 months')") + end # We store YAML version of POST parameters in the database def post_params=(params) self.post_params_yaml = params.to_yaml end + def post_params - if self.post_params_yaml.nil? - return {} - end - YAML.load(self.post_params_yaml) + return {} if post_params_yaml.nil? + YAML.load(post_params_yaml) end # We store YAML version of textual "reason for redirect" parameters def reason_params=(reason_params) self.reason_params_yaml = reason_params.to_yaml end + def reason_params - YAML.load(self.reason_params_yaml) + YAML.load(reason_params_yaml) end # Extract just local path part, without domain or # def local_part_uri - self.uri.match(/^http:\/\/.+?(\/[^#]+)/) - return $1 - end - - # Makes a random token, suitable for using in URLs e.g confirmation messages. - def self.generate_random_token - MySociety::Util.generate_token - end - - # Used by (rspec) test code only - def self.get_last_post_redirect - # TODO: yeuch - no other easy way of getting the token so we can check - # the redirect URL, as it is by definition opaque to the controller - # apart from in the place that it redirects to. - post_redirects = PostRedirect.find_by_sql("select * from post_redirects order by id desc limit 1") - post_redirects.size.should == 1 - return post_redirects[0] - end - - # Called from cron job delete-old-things - def self.delete_old_post_redirects - PostRedirect.delete_all "updated_at < (now() - interval '2 months')" + uri.match(/^http:\/\/.+?(\/[^#]+)/) + $1 end private + # The token is used to return you to what you are doing after the login + # form. def generate_token - # The token is used to return you to what you are doing after the login form. - if not self.token - self.token = PostRedirect.generate_random_token - end - # There is a separate token to use in the URL if we send a confirmation email. - if not self.email_token - self.email_token = PostRedirect.generate_random_token - end + self.token = PostRedirect.generate_random_token unless token end -end - - + # There is a separate token to use in the URL if we send a confirmation + # email. + def generate_email_token + self.email_token = PostRedirect.generate_random_token unless email_token + end +end diff --git a/app/models/profile_photo.rb b/app/models/profile_photo.rb index 3c0be222c..61f88faf3 100644 --- a/app/models/profile_photo.rb +++ b/app/models/profile_photo.rb @@ -15,87 +15,84 @@ # Email: hello@mysociety.org; WWW: http://www.mysociety.org/ class ProfilePhoto < ActiveRecord::Base + # deliberately don't strip_attributes, so keeps raw photo properly + WIDTH = 96 HEIGHT = 96 - MAX_DRAFT = 500 # keep even pre-cropped images reasonably small belongs_to :user validate :data_and_draft_checks - # deliberately don't strip_attributes, so keeps raw photo properly - attr_accessor :x, :y, :w, :h - attr_accessor :image after_initialize :convert_data_to_image # make image valid format and size def convert_image - if self.data.nil? - return - end - if self.image.nil? - return - end + return if data.nil? + return if image.nil? # convert to PNG if it isn't, and to right size altered = false - if self.image.format != 'PNG' + if image.format != 'PNG' self.image.format = 'PNG' altered = true end + # draft images are before the user has cropped them - if !self.draft && (image.columns != WIDTH || image.rows != HEIGHT) + if !draft && (image.columns != WIDTH || image.rows != HEIGHT) # do any exact cropping (taken from Jcrop interface) - if self.w && self.h - image.crop!(self.x.to_i, self.y.to_i, self.w.to_i, self.h.to_i) + if w && h + image.crop!(x.to_i, y.to_i, w.to_i, h.to_i) end # do any further cropping image.resize_to_fill!(WIDTH, HEIGHT) altered = true end - if self.draft && (image.columns > MAX_DRAFT || image.rows > MAX_DRAFT) + + if draft && (image.columns > MAX_DRAFT || image.rows > MAX_DRAFT) image.resize_to_fit!(MAX_DRAFT, MAX_DRAFT) altered = true end + if altered - write_attribute(:data, self.image.to_blob) + write_attribute(:data, image.to_blob) end end private def data_and_draft_checks - if self.data.nil? + if data.nil? errors.add(:data, _("Please choose a file containing your photo.")) return end - if self.image.nil? + if image.nil? errors.add(:data, _("Couldn't understand the image file that you uploaded. PNG, JPEG, GIF and many other common image file formats are supported.")) return end - if self.image.format != 'PNG' + if image.format != 'PNG' errors.add(:data, _("Failed to convert image to a PNG")) end - if !self.draft && (self.image.columns != WIDTH || self.image.rows != HEIGHT) + if !draft && (image.columns != WIDTH || image.rows != HEIGHT) errors.add(:data, _("Failed to convert image to the correct size: at {{cols}}x{{rows}}, need {{width}}x{{height}}", - :cols => self.image.columns, - :rows => self.image.rows, + :cols => image.columns, + :rows => image.rows, :width => WIDTH, :height => HEIGHT)) end - if self.draft && self.user_id + if draft && user_id raise "Internal error, draft pictures must not have a user" end - if !self.draft && !self.user_id + if !draft && !user_id raise "Internal error, real pictures must have a user" end end @@ -108,6 +105,7 @@ class ProfilePhoto < ActiveRecord::Base end image_list = Magick::ImageList.new + begin image_list.from_blob(data) rescue Magick::ImageMagickError @@ -115,9 +113,9 @@ class ProfilePhoto < ActiveRecord::Base return end - self.image = image_list[0] # TODO: perhaps take largest image or somesuch if there were multiple in the file? - self.convert_image + # TODO: perhaps take largest image or somesuch if there were multiple + # in the file? + self.image = image_list[0] + convert_image end end - - diff --git a/app/models/public_body.rb b/app/models/public_body.rb index f61a3f449..1929272ea 100644 --- a/app/models/public_body.rb +++ b/app/models/public_body.rb @@ -49,7 +49,12 @@ class PublicBody < ActiveRecord::Base attr_accessor :no_xapian_reindex has_tag_string - before_save :set_api_key, :set_default_publication_scheme + + before_save :set_api_key, + :set_default_publication_scheme, + :set_first_letter + after_save :purge_in_cache + after_update :reindex_requested_from # Every public body except for the internal admin one is visible scope :visible, lambda { @@ -75,6 +80,21 @@ class PublicBody < ActiveRecord::Base ] end + acts_as_xapian :texts => [ :name, :short_name, :notes ], + :values => [ + [ :created_at_numeric, 1, "created_at", :number ] # for sorting + ], + :terms => [ [ :variety, 'V', "variety" ], + [ :tag_array_for_search, 'U', "tag" ] + ] + + acts_as_versioned + self.non_versioned_columns << 'created_at' << 'updated_at' << 'first_letter' << 'api_key' + self.non_versioned_columns << 'info_requests_count' << 'info_requests_successful_count' + self.non_versioned_columns << 'info_requests_count' << 'info_requests_visible_classified_count' + self.non_versioned_columns << 'info_requests_not_held_count' << 'info_requests_overdue' + self.non_versioned_columns << 'info_requests_overdue_count' + # Public: Search for Public Bodies whose name, short_name, request_email or # tags contain the given query # @@ -189,7 +209,6 @@ class PublicBody < ActiveRecord::Base end # Set the first letter, which is used for faster queries - before_save(:set_first_letter) def set_first_letter PublicBody.set_first_letter(self) end @@ -236,13 +255,6 @@ class PublicBody < ActiveRecord::Base end end - acts_as_versioned - self.non_versioned_columns << 'created_at' << 'updated_at' << 'first_letter' << 'api_key' - self.non_versioned_columns << 'info_requests_count' << 'info_requests_successful_count' - self.non_versioned_columns << 'info_requests_count' << 'info_requests_visible_classified_count' - self.non_versioned_columns << 'info_requests_not_held_count' << 'info_requests_overdue' - self.non_versioned_columns << 'info_requests_overdue_count' - class Version def last_edit_comment_for_html_display @@ -273,13 +285,6 @@ class PublicBody < ActiveRecord::Base end end - acts_as_xapian :texts => [ :name, :short_name, :notes ], - :values => [ - [ :created_at_numeric, 1, "created_at", :number ] # for sorting - ], - :terms => [ [ :variety, 'V', "variety" ], - [ :tag_array_for_search, 'U', "tag" ] - ] def created_at_numeric # format it here as no datetime support in Xapian's value ranges return self.created_at.strftime("%Y%m%d%H%M%S") @@ -291,7 +296,6 @@ class PublicBody < ActiveRecord::Base # if the URL name has changed, then all requested_from: queries # will break unless we update index for every event for every # request linked to it - after_update :reindex_requested_from def reindex_requested_from if self.changes.include?('url_name') for info_request in self.info_requests @@ -680,7 +684,6 @@ class PublicBody < ActiveRecord::Base } end - after_save(:purge_in_cache) def purge_in_cache self.info_requests.each {|x| x.purge_in_cache} end diff --git a/app/models/purge_request.rb b/app/models/purge_request.rb index 4e6267bd2..81980188d 100644 --- a/app/models/purge_request.rb +++ b/app/models/purge_request.rb @@ -19,15 +19,17 @@ class PurgeRequest < ActiveRecord::Base def self.purge_all done_something = false - for item in PurgeRequest.all() + + PurgeRequest.all.each do |item| item.purge done_something = true end - return done_something + + done_something end + # Run purge_all in an endless loop, sleeping when there is nothing to do def self.purge_all_loop - # Run purge_all in an endless loop, sleeping when there is nothing to do while true sleep_seconds = 1 while !purge_all @@ -39,13 +41,8 @@ class PurgeRequest < ActiveRecord::Base end def purge - config = MySociety::Config.load_default() - varnish_url = config['VARNISH_HOST'] - result = quietly_try_to_purge(varnish_url, self.url) - self.delete() + config = MySociety::Config.load_default + result = quietly_try_to_purge(config['VARNISH_HOST'], url) + delete end end - - - - diff --git a/app/models/raw_email.rb b/app/models/raw_email.rb index 21a53f493..3b466cb81 100644 --- a/app/models/raw_email.rb +++ b/app/models/raw_email.rb @@ -17,44 +17,46 @@ class RawEmail < ActiveRecord::Base has_one :incoming_message def directory - request_id = self.incoming_message.info_request.id.to_s if request_id.empty? raise "Failed to find the id number of the associated request: has it been saved?" end if Rails.env.test? - return File.join(Rails.root, 'files/raw_email_test') + File.join(Rails.root, 'files/raw_email_test') else - return File.join(AlaveteliConfiguration::raw_emails_location, - request_id[0..2], request_id) + File.join(AlaveteliConfiguration::raw_emails_location, + request_id[0..2], request_id) end end def filepath - incoming_message_id = self.incoming_message.id.to_s if incoming_message_id.empty? raise "Failed to find the id number of the associated incoming message: has it been saved?" end - File.join(self.directory, incoming_message_id) + + File.join(directory, incoming_message_id) end def data=(d) - if !File.exists?(self.directory) - FileUtils.mkdir_p self.directory - end - File.atomic_write(self.filepath) { |file| - file.write d - } + FileUtils.mkdir_p(directory) unless File.exists?(directory) + File.atomic_write(filepath) { |file| file.write(d) } end def data - File.open(self.filepath, "r").read + File.open(filepath, "r").read end def destroy_file_representation! - File.delete(self.filepath) + File.delete(filepath) end -end + private + def request_id + incoming_message.info_request.id.to_s + end + def incoming_message_id + incoming_message.id.to_s + end +end diff --git a/app/models/track_thing.rb b/app/models/track_thing.rb index 10ba28f4a..5819876ff 100644 --- a/app/models/track_thing.rb +++ b/app/models/track_thing.rb @@ -25,120 +25,86 @@ require 'set' # TODO: TrackThing looks like a good candidate for single table inheritance class TrackThing < ActiveRecord::Base - belongs_to :tracking_user, :class_name => 'User' - validates_presence_of :track_query - validates_presence_of :track_type + # { TRACK_TYPE => DESCRIPTION } + TRACK_TYPES = { 'request_updates' => _('Individual requests'), + 'all_new_requests' => _('Many requests'), + 'all_successful_requests' => _('Many requests'), + 'public_body_updates' => _('Public authorities'), + 'user_updates' => _('People'), + 'search_query' => _('Search queries') } + + TRACK_MEDIUMS = %w(email_daily feed) belongs_to :info_request belongs_to :public_body + belongs_to :tracking_user, :class_name => 'User' belongs_to :tracked_user, :class_name => 'User' - has_many :track_things_sent_emails - validates_inclusion_of :track_type, :in => [ - 'request_updates', - 'all_new_requests', - 'all_successful_requests', - 'public_body_updates', - 'user_updates', - 'search_query' - ] - - validates_inclusion_of :track_medium, :in => [ - 'email_daily', - 'feed' - ] - - def TrackThing.track_type_description(track_type) - if track_type == 'request_updates' - _("Individual requests") - elsif track_type == 'all_new_requests' || track_type == "all_successful_requests" - _("Many requests") - elsif track_type == 'public_body_updates' - _("Public authorities") - elsif track_type == 'user_updates' - _("People") - elsif track_type == 'search_query' - _("Search queries") - else - raise "internal error " + track_type - end - end - def track_type_description - TrackThing.track_type_description(self.track_type) - end + validates_presence_of :track_query + validates_presence_of :track_type + validates_inclusion_of :track_type, :in => TRACK_TYPES.keys + validates_inclusion_of :track_medium, :in => TRACK_MEDIUMS - def track_query_description - filter_description = query_filter_description('(variety:sent OR variety:followup_sent OR variety:response OR variety:comment)', - :no_query => N_("all requests or comments"), - :query => N_("all requests or comments matching text '{{query}}'")) - return filter_description if filter_description - filter_description = query_filter_description('(latest_status:successful OR latest_status:partially_successful)', - :no_query => N_("requests which are successful"), - :query => N_("requests which are successful matching text '{{query}}'")) - return filter_description if filter_description - return _("anything matching text '{{query}}'", :query => track_query) + # When constructing a new track, use this to avoid duplicates / double + # posting + def self.find_existing(tracking_user, track) + return nil if tracking_user.nil? + where(:tracking_user_id => tracking_user.id, + :track_query => track.track_query, + :track_type => track.track_type).first end - # Return a readable query description for queries involving commonly used filter clauses - def query_filter_description(string, options) - parsed_query = track_query.gsub(string, '') - if parsed_query != track_query - parsed_query.strip! - if parsed_query.empty? - _(options[:no_query]) - else - _(options[:query], :query => parsed_query) - end - end + def self.track_type_description(track_type) + TRACK_TYPES.fetch(track_type) { raise "internal error #{ track_type }" } end - def TrackThing.create_track_for_request(info_request) + def self.create_track_for_request(info_request) track_thing = TrackThing.new track_thing.track_type = 'request_updates' track_thing.info_request = info_request - track_thing.track_query = "request:" + info_request.url_title - return track_thing + track_thing.track_query = "request:#{ info_request.url_title }" + track_thing end - def TrackThing.create_track_for_all_new_requests + def self.create_track_for_all_new_requests track_thing = TrackThing.new track_thing.track_type = 'all_new_requests' track_thing.track_query = "variety:sent" - return track_thing + track_thing end - def TrackThing.create_track_for_all_successful_requests + def self.create_track_for_all_successful_requests track_thing = TrackThing.new track_thing.track_type = 'all_successful_requests' track_thing.track_query = 'variety:response (status:successful OR status:partially_successful)' - return track_thing + track_thing end - def TrackThing.create_track_for_public_body(public_body, event_type = nil) + def self.create_track_for_public_body(public_body, event_type = nil) track_thing = TrackThing.new track_thing.track_type = 'public_body_updates' track_thing.public_body = public_body - query = "requested_from:" + public_body.url_name + query = "requested_from:#{ public_body.url_name }" if InfoRequestEvent.enumerate_event_types.include?(event_type) - query += " variety:" + event_type + query += " variety:#{ event_type }" end track_thing.track_query = query - return track_thing + track_thing end - def TrackThing.create_track_for_user(user) + def self.create_track_for_user(user) track_thing = TrackThing.new track_thing.track_type = 'user_updates' track_thing.tracked_user = user - track_thing.track_query = "requested_by:" + user.url_name + " OR commented_by:" + user.url_name - return track_thing + track_thing.track_query = "requested_by:#{ user.url_name } OR commented_by: #{ user.url_name }" + track_thing end - def TrackThing.create_track_for_search_query(query, variety_postfix = nil) + def self.create_track_for_search_query(query, variety_postfix = nil) track_thing = TrackThing.new track_thing.track_type = 'search_query' - if !(query =~ /variety:/) + unless query =~ /variety:/ case variety_postfix when "requests" query += " variety:sent" @@ -154,146 +120,180 @@ class TrackThing < ActiveRecord::Base # Should also update "params" to make the list_description # nicer and more generic. It will need to do some clever # parsing of the query to do this nicely - return track_thing + track_thing end - # Return hash of text parameters describing the request etc. - def params - if @params.nil? - if self.track_type == 'request_updates' - @params = { - # Website + def track_type_description + TrackThing.track_type_description(track_type) + end + + def track_query_description + filter_description = query_filter_description('(variety:sent OR variety:followup_sent OR variety:response OR variety:comment)', + :no_query => N_("all requests or comments"), + :query => N_("all requests or comments matching text '{{query}}'")) + return filter_description if filter_description + + filter_description = query_filter_description('(latest_status:successful OR latest_status:partially_successful)', + :no_query => N_("requests which are successful"), + :query => N_("requests which are successful matching text '{{query}}'")) + return filter_description if filter_description - :verb_on_page => _("Follow this request"), - :verb_on_page_already => _("You are already following this request"), - # Email - :title_in_email => _("New updates for the request '{{request_title}}'", - :request_title => self.info_request.title.html_safe), - :title_in_rss => _("New updates for the request '{{request_title}}'", - :request_title => self.info_request.title), - # Authentication - :web => _("To follow the request '{{request_title}}'", - :request_title => self.info_request.title), - :email => _("Then you will be updated whenever the request '{{request_title}}' is updated.", - :request_title => self.info_request.title), - :email_subject => _("Confirm you want to follow the request '{{request_title}}'", - :request_title => self.info_request.title), - # RSS sorting - :feed_sortby => 'newest' - } - elsif self.track_type == 'all_new_requests' - @params = { - # Website - :verb_on_page => _("Follow all new requests"), - :verb_on_page_already => _("You are already following new requests"), - # Email - :title_in_email => _("New Freedom of Information requests"), - :title_in_rss => _("New Freedom of Information requests"), - # Authentication - :web => _("To follow new requests"), - :email => _("Then you will be following all new FOI requests."), - :email_subject => _("Confirm you want to follow new requests"), - # RSS sorting - :feed_sortby => 'newest' - } - elsif self.track_type == 'all_successful_requests' - @params = { - # Website - :verb_on_page => _("Follow new successful responses"), - :verb_on_page_already => _("You are following all new successful responses"), - # Email - :title_in_email => _("Successful Freedom of Information requests"), - :title_in_rss => _("Successful Freedom of Information requests"), - # Authentication - :web => _("To follow all successful requests"), - :email => _("Then you will be notified whenever an FOI request succeeds."), - :email_subject => _("Confirm you want to follow all successful FOI requests"), - # RSS sorting - used described date, as newest would give a - # date for responses possibly days before description, so - # wouldn't appear at top of list when description (known - # success) causes match. - :feed_sortby => 'described' - } - elsif self.track_type == 'public_body_updates' - @params = { - # Website - :verb_on_page => _("Follow requests to {{public_body_name}}", - :public_body_name => self.public_body.name), - :verb_on_page_already => _("You are already following requests to {{public_body_name}}", - :public_body_name => self.public_body.name), - # Email - :title_in_email => _("{{foi_law}} requests to '{{public_body_name}}'", - :foi_law => self.public_body.law_only_short, - :public_body_name => self.public_body.name), - :title_in_rss => _("{{foi_law}} requests to '{{public_body_name}}'", - :foi_law => self.public_body.law_only_short, - :public_body_name => self.public_body.name), - # Authentication - :web => _("To follow requests made using {{site_name}} to the public authority '{{public_body_name}}'", - :site_name => AlaveteliConfiguration::site_name, - :public_body_name => self.public_body.name), - :email => _("Then you will be notified whenever someone requests something or gets a response from '{{public_body_name}}'.", - :public_body_name => self.public_body.name), - :email_subject => _("Confirm you want to follow requests to '{{public_body_name}}'", - :public_body_name => self.public_body.name), - # RSS sorting - :feed_sortby => 'newest' - } - elsif self.track_type == 'user_updates' - @params = { - # Website - :verb_on_page => _("Follow this person"), - :verb_on_page_already => _("You are already following this person"), - # Email - :title_in_email => _("FOI requests by '{{user_name}}'", - :user_name => self.tracked_user.name.html_safe), - :title_in_rss => _("FOI requests by '{{user_name}}'", - :user_name => self.tracked_user.name), - # Authentication - :web => _("To follow requests by '{{user_name}}'", - :user_name=> self.tracked_user.name), - :email => _("Then you will be notified whenever '{{user_name}}' requests something or gets a response.", - :user_name => self.tracked_user.name), - :email_subject => _("Confirm you want to follow requests by '{{user_name}}'", - :user_name => self.tracked_user.name), - # RSS sorting - :feed_sortby => 'newest' - } - elsif self.track_type == 'search_query' - @params = { - # Website - :verb_on_page => _("Follow things matching this search"), - :verb_on_page_already => _("You are already following things matching this search"), - # Email - :title_in_email => _("Requests or responses matching your saved search"), - :title_in_rss => _("Requests or responses matching your saved search"), - # Authentication - :web => _("To follow requests and responses matching your search"), - :email => _("Then you will be notified whenever a new request or response matches your search."), - :email_subject => _("Confirm you want to follow new requests or responses matching your search"), - # RSS sorting - TODO: hmmm, we don't really know which to use - # here for sorting. Might be a query term (e.g. 'cctv'), in - # which case newest is good, or might be something like - # all refused requests in which case want to sort by - # described (when we discover criteria is met). Rather - # conservatively am picking described, as that will make - # things appear in feed more than the should, rather than less. - :feed_sortby => 'described' - } - else - raise "unknown tracking type " + self.track_type + _("anything matching text '{{query}}'", :query => track_query) + end + + # Return a readable query description for queries involving commonly used + # filter clauses + def query_filter_description(string, options) + parsed_query = track_query.gsub(string, '') + if parsed_query != track_query + parsed_query.strip! + if parsed_query.empty? + _(options[:no_query]) + else + _(options[:query], :query => parsed_query) end end - return @params end - # When constructing a new track, use this to avoid duplicates / double posting - def TrackThing.find_existing(tracking_user, track) - if tracking_user.nil? - return nil + # Return hash of text parameters based on the track_type describing the + # request etc. + def params + @params ||= params_for(track_type) + end + + private + + def params_for(track_type) + if respond_to?("#{ track_type }_params", true) + send("#{ track_type }_params") + else + raise "unknown tracking type #{ track_type }" end - return TrackThing.find(:first, :conditions => [ 'tracking_user_id = ? and track_query = ? and track_type = ?', tracking_user.id, track.track_query, track.track_type ] ) end -end + def request_updates_params + { # Website + :verb_on_page => _("Follow this request"), + :verb_on_page_already => _("You are already following this request"), + # Email + :title_in_email => _("New updates for the request '{{request_title}}'", + :request_title => info_request.title.html_safe), + :title_in_rss => _("New updates for the request '{{request_title}}'", + :request_title => info_request.title), + # Authentication + :web => _("To follow the request '{{request_title}}'", + :request_title => info_request.title), + :email => _("Then you will be updated whenever the request '{{request_title}}' is updated.", + :request_title => info_request.title), + :email_subject => _("Confirm you want to follow the request '{{request_title}}'", + :request_title => info_request.title), + # RSS sorting + :feed_sortby => 'newest' + } + end + + def all_new_requests_params + { # Website + :verb_on_page => _("Follow all new requests"), + :verb_on_page_already => _("You are already following new requests"), + # Email + :title_in_email => _("New Freedom of Information requests"), + :title_in_rss => _("New Freedom of Information requests"), + # Authentication + :web => _("To follow new requests"), + :email => _("Then you will be following all new FOI requests."), + :email_subject => _("Confirm you want to follow new requests"), + # RSS sorting + :feed_sortby => 'newest' + } + end + def all_successful_requests_params + { # Website + :verb_on_page => _("Follow new successful responses"), + :verb_on_page_already => _("You are following all new successful responses"), + # Email + :title_in_email => _("Successful Freedom of Information requests"), + :title_in_rss => _("Successful Freedom of Information requests"), + # Authentication + :web => _("To follow all successful requests"), + :email => _("Then you will be notified whenever an FOI request succeeds."), + :email_subject => _("Confirm you want to follow all successful FOI requests"), + # RSS sorting - used described date, as newest would give a + # date for responses possibly days before description, so + # wouldn't appear at top of list when description (known + # success) causes match. + :feed_sortby => 'described' + } + end + + def public_body_updates_params + { # Website + :verb_on_page => _("Follow requests to {{public_body_name}}", + :public_body_name => public_body.name), + :verb_on_page_already => _("You are already following requests to {{public_body_name}}", + :public_body_name => public_body.name), + # Email + :title_in_email => _("{{foi_law}} requests to '{{public_body_name}}'", + :foi_law => public_body.law_only_short, + :public_body_name => public_body.name), + :title_in_rss => _("{{foi_law}} requests to '{{public_body_name}}'", + :foi_law => public_body.law_only_short, + :public_body_name => public_body.name), + # Authentication + :web => _("To follow requests made using {{site_name}} to the public authority '{{public_body_name}}'", + :site_name => AlaveteliConfiguration.site_name, + :public_body_name => public_body.name), + :email => _("Then you will be notified whenever someone requests something or gets a response from '{{public_body_name}}'.", + :public_body_name => public_body.name), + :email_subject => _("Confirm you want to follow requests to '{{public_body_name}}'", + :public_body_name => public_body.name), + # RSS sorting + :feed_sortby => 'newest' + } + end + + def user_updates_params + { # Website + :verb_on_page => _("Follow this person"), + :verb_on_page_already => _("You are already following this person"), + # Email + :title_in_email => _("FOI requests by '{{user_name}}'", + :user_name => tracked_user.name.html_safe), + :title_in_rss => _("FOI requests by '{{user_name}}'", + :user_name => tracked_user.name), + # Authentication + :web => _("To follow requests by '{{user_name}}'", + :user_name => tracked_user.name), + :email => _("Then you will be notified whenever '{{user_name}}' requests something or gets a response.", + :user_name => tracked_user.name), + :email_subject => _("Confirm you want to follow requests by '{{user_name}}'", + :user_name => tracked_user.name), + # RSS sorting + :feed_sortby => 'newest' + } + end + + def search_query_params + { # Website + :verb_on_page => _("Follow things matching this search"), + :verb_on_page_already => _("You are already following things matching this search"), + # Email + :title_in_email => _("Requests or responses matching your saved search"), + :title_in_rss => _("Requests or responses matching your saved search"), + # Authentication + :web => _("To follow requests and responses matching your search"), + :email => _("Then you will be notified whenever a new request or response matches your search."), + :email_subject => _("Confirm you want to follow new requests or responses matching your search"), + # RSS sorting - TODO: hmmm, we don't really know which to use + # here for sorting. Might be a query term (e.g. 'cctv'), in + # which case newest is good, or might be something like + # all refused requests in which case want to sort by + # described (when we discover criteria is met). Rather + # conservatively am picking described, as that will make + # things appear in feed more than the should, rather than less. + :feed_sortby => 'described' + } + end + +end diff --git a/app/models/user.rb b/app/models/user.rb index 4b83d8572..1c6dc0eb0 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -28,11 +28,7 @@ require 'digest/sha1' class User < ActiveRecord::Base strip_attributes! - validates_presence_of :email, :message => _("Please enter your email address") - - validates_presence_of :name, :message => _("Please enter your name") - - validates_presence_of :hashed_password, :message => _("Please enter a password") + attr_accessor :password_confirmation, :no_xapian_reindex has_many :info_requests, :order => 'created_at desc' has_many :user_info_request_sent_alerts @@ -43,9 +39,10 @@ class User < ActiveRecord::Base has_many :censor_rules, :order => 'created_at desc' has_many :info_request_batches, :order => 'created_at desc' - attr_accessor :password_confirmation, :no_xapian_reindex + validates_presence_of :email, :message => _("Please enter your email address") + validates_presence_of :name, :message => _("Please enter your name") + validates_presence_of :hashed_password, :message => _("Please enter a password") validates_confirmation_of :password, :message => _("Please enter the same password twice") - validates_inclusion_of :admin_level, :in => [ 'none', 'super', @@ -53,6 +50,10 @@ class User < ActiveRecord::Base validate :email_and_name_are_valid + after_initialize :set_defaults + after_save :purge_in_cache + after_update :reindex_referencing_models + acts_as_xapian :texts => [ :name, :about_me ], :values => [ [ :created_at_numeric, 1, "created_at", :number ] # for sorting @@ -60,11 +61,111 @@ class User < ActiveRecord::Base :terms => [ [ :variety, 'V', "variety" ] ], :if => :indexed_by_search? - after_initialize :set_defaults + # Return user given login email, password and other form parameters (e.g. name) + # + # The specific_user_login parameter says that login as a particular user is + # expected, so no parallel registration form is being displayed. + def self.authenticate_from_form(params, specific_user_login = false) + params[:email].strip! + + if specific_user_login + auth_fail_message = _("Either the email or password was not recognised, please try again.") + else + auth_fail_message = _("Either the email or password was not recognised, please try again. Or create a new account using the form on the right.") + end + + user = find_user_by_email(params[:email]) + if user + # There is user with email, check password + unless user.has_this_password?(params[:password]) + user.errors.add(:base, auth_fail_message) + end + else + # No user of same email, make one (that we don't save in the database) + # for the forms code to use. + user = User.new(params) + # deliberately same message as above so as not to leak whether registered + user.errors.add(:base, auth_fail_message) + end + user + end + + # Case-insensitively find a user from their email + def self.find_user_by_email(email) + self.find(:first, :conditions => [ 'lower(email) = lower(?)', email ] ) + end + + # The "internal admin" is a special user for internal use. + def self.internal_admin_user + user = User.find_by_email(AlaveteliConfiguration::contact_email) + if user.nil? + password = PostRedirect.generate_random_token + user = User.new( + :name => 'Internal admin user', + :email => AlaveteliConfiguration.contact_email, + :password => password, + :password_confirmation => password + ) + user.save! + end + + user + end + + def self.owns_every_request?(user) + !user.nil? && user.owns_every_request? + end + + # Can the user see every request, response, and outgoing message, even hidden ones? + def self.view_hidden?(user) + !user.nil? && user.super? + end + + # Should the user be kept logged into their own account + # if they follow a /c/ redirect link belonging to another user? + def self.stay_logged_in_on_redirect?(user) + !user.nil? && user.super? + end + + # Used for default values of last_daily_track_email + def self.random_time_in_last_day + earliest_time = Time.now - 1.day + latest_time = Time.now + earliest_time + rand(latest_time - earliest_time).seconds + end + + # Alters last_daily_track_email for every user, so alerts will be sent + # spread out fairly evenly throughout the day, balancing load on the + # server. This is intended to be called by hand from the Ruby console. It + # will mean quite a few users may get more than one email alert the day you + # do it, so have a care and run it rarely. + # + # This SQL statement is useful for seeing how spread out users are at the moment: + # select extract(hour from last_daily_track_email) as h, count(*) from users group by extract(hour from last_daily_track_email) order by h; + def self.spread_alert_times_across_day + self.find(:all).each do |user| + user.last_daily_track_email = User.random_time_in_last_day + user.save! + end + nil # so doesn't print all users on console + end + + def self.encrypted_password(password, salt) + string_to_hash = password + salt # TODO: need to add a secret here too? + Digest::SHA1.hexdigest(string_to_hash) + end + + def self.record_bounce_for_email(email, message) + user = User.find_user_by_email(email) + return false if user.nil? + + user.record_bounce(message) if user.email_bounced_at.nil? + return true + end def created_at_numeric # format it here as no datetime support in Xapian's value ranges - return self.created_at.strftime("%Y%m%d%H%M%S") + created_at.strftime("%Y%m%d%H%M%S") end def variety @@ -72,18 +173,18 @@ class User < ActiveRecord::Base end # requested_by: and commented_by: search queries also need updating after save - after_update :reindex_referencing_models def reindex_referencing_models return if no_xapian_reindex == true - if self.changes.include?('url_name') - for comment in self.comments - for info_request_event in comment.info_request_events + if changes.include?('url_name') + comments.each do |comment| + comment.info_request_events.each do |info_request_event| info_request_event.xapian_mark_needs_index end end - for info_request in self.info_requests - for info_request_event in info_request.info_request_events + + info_requests.each do |info_request| + info_request.info_request_events.each do |info_request_event| info_request_event.xapian_mark_needs_index end end @@ -91,11 +192,11 @@ class User < ActiveRecord::Base end def get_locale - (self.locale || I18n.locale).to_s + (locale || I18n.locale).to_s end def visible_comments - self.comments.find(:all, :conditions => 'visible') + comments.find(:all, :conditions => 'visible') end # Don't display any leading/trailing spaces @@ -106,62 +207,29 @@ class User < ActiveRecord::Base if not name.nil? name.strip! end - if self.public_banned? + if public_banned? # Use interpolation to return a string rather than a SafeBuffer so that # gsub can be called on it until we upgrade to Rails 3.2. The name returned # is not marked as HTML safe so will be escaped automatically in views. We # do this in two steps so the string still gets picked up for translation - name = _("{{user_name}} (Account suspended)", :user_name=> name.html_safe) + name = _("{{user_name}} (Account suspended)", :user_name => name.html_safe) name = "#{name}" end name end - # Return user given login email, password and other form parameters (e.g. name) - # - # The specific_user_login parameter says that login as a particular user is - # expected, so no parallel registration form is being displayed. - def User.authenticate_from_form(params, specific_user_login = false) - params[:email].strip! - - if specific_user_login - auth_fail_message = _("Either the email or password was not recognised, please try again.") - else - auth_fail_message = _("Either the email or password was not recognised, please try again. Or create a new account using the form on the right.") - end - - user = self.find_user_by_email(params[:email]) - if user - # There is user with email, check password - if !user.has_this_password?(params[:password]) - user.errors.add(:base, auth_fail_message) - end - else - # No user of same email, make one (that we don't save in the database) - # for the forms code to use. - user = User.new(params) - # deliberately same message as above so as not to leak whether registered - user.errors.add(:base, auth_fail_message) - end - user - end - - # Case-insensitively find a user from their email - def User.find_user_by_email(email) - return self.find(:first, :conditions => [ 'lower(email) = lower(?)', email ] ) - end - # When name is changed, also change the url name def name=(name) write_attribute(:name, name) - self.update_url_name + update_url_name end + def update_url_name - url_name = MySociety::Format.simplify_url_part(self.name, 'user', 32) + url_name = MySociety::Format.simplify_url_part(name, 'user', 32) # For user with same name as others, add on arbitary numeric identifier unique_url_name = url_name suffix_num = 2 # as there's already one without numeric suffix - while not User.find_by_url_name(unique_url_name, :conditions => self.id.nil? ? nil : ["id <> ?", self.id] ).nil? + while not User.find_by_url_name(unique_url_name, :conditions => id.nil? ? nil : ["id <> ?", id] ).nil? unique_url_name = url_name + "_" + suffix_num.to_s suffix_num = suffix_num + 1 end @@ -172,6 +240,7 @@ class User < ActiveRecord::Base def password @password end + def password=(pwd) @password = pwd if pwd.blank? @@ -179,40 +248,23 @@ class User < ActiveRecord::Base return end create_new_salt - self.hashed_password = User.encrypted_password(self.password, self.salt) + self.hashed_password = User.encrypted_password(password, salt) end def has_this_password?(password) - expected_password = User.encrypted_password(password, self.salt) - return self.hashed_password == expected_password + expected_password = User.encrypted_password(password, salt) + hashed_password == expected_password end # For use in to/from in email messages def name_and_email - return MailHandler.address_from_name_and_email(self.name, self.email) - end - - # The "internal admin" is a special user for internal use. - def User.internal_admin_user - u = User.find_by_email(AlaveteliConfiguration::contact_email) - if u.nil? - password = PostRedirect.generate_random_token - u = User.new( - :name => 'Internal admin user', - :email => AlaveteliConfiguration::contact_email, - :password => password, - :password_confirmation => password - ) - u.save! - end - - return u + MailHandler.address_from_name_and_email(name, email) end # Returns list of requests which the user hasn't described (and last # changed more than a day ago) def get_undescribed_requests - self.info_requests.find( + info_requests.find( :all, :conditions => [ 'awaiting_description = ? and ' + InfoRequest.last_event_time_clause + ' < ?', true, Time.now() - 1.day @@ -223,7 +275,7 @@ class User < ActiveRecord::Base # Can the user make new requests, without having to describe state of (most) existing ones? def can_leave_requests_undescribed? # TODO: should be flag in database really - if self.url_name == "heather_brooke" || self.url_name == "heather_brooke_2" + if url_name == "heather_brooke" || url_name == "heather_brooke_2" return true end return false @@ -232,140 +284,102 @@ class User < ActiveRecord::Base # Does the user magically gain powers as if they owned every request? # e.g. Can classify it def owns_every_request? - self.super? + super? end # Does this user have extraordinary powers? def super? - self.admin_level == 'super' - end - - def User.owns_every_request?(user) - !user.nil? && user.owns_every_request? - end - - # Can the user see every request, response, and outgoing message, even hidden ones? - def User.view_hidden?(user) - !user.nil? && user.super? - end - - # Should the user be kept logged into their own account - # if they follow a /c/ redirect link belonging to another user? - def User.stay_logged_in_on_redirect?(user) - !user.nil? && user.super? + admin_level == 'super' end # Does the user get "(admin)" links on each page on the main site? def admin_page_links? - self.super? + super? end # Is it public that they are banned? def public_banned? - !self.ban_text.empty? + !ban_text.empty? end # Various ways the user can be banned, and text to describe it if failed def can_file_requests? - self.ban_text.empty? && !self.exceeded_limit? + ban_text.empty? && !exceeded_limit? end def exceeded_limit? # Some users have no limit - return false if self.no_limit + return false if no_limit # Batch request users don't have a limit - return false if self.can_make_batch_requests? + return false if can_make_batch_requests? # Has the user issued as many as MAX_REQUESTS_PER_USER_PER_DAY requests in the past 24 hours? - return false if AlaveteliConfiguration::max_requests_per_user_per_day.blank? - recent_requests = InfoRequest.count(:conditions => ["user_id = ? and created_at > now() - '1 day'::interval", self.id]) + return false if AlaveteliConfiguration.max_requests_per_user_per_day.blank? + recent_requests = InfoRequest.count(:conditions => ["user_id = ? and created_at > now() - '1 day'::interval", id]) - return (recent_requests >= AlaveteliConfiguration::max_requests_per_user_per_day) + recent_requests >= AlaveteliConfiguration.max_requests_per_user_per_day end def next_request_permitted_at - return nil if self.no_limit + return nil if no_limit - n_most_recent_requests = InfoRequest.all(:conditions => ["user_id = ? and created_at > now() - '1 day'::interval", self.id], :order => "created_at DESC", :limit => AlaveteliConfiguration::max_requests_per_user_per_day) + n_most_recent_requests = InfoRequest.all(:conditions => ["user_id = ? and created_at > now() - '1 day'::interval", id], + :order => "created_at DESC", + :limit => AlaveteliConfiguration::max_requests_per_user_per_day) return nil if n_most_recent_requests.size < AlaveteliConfiguration::max_requests_per_user_per_day nth_most_recent_request = n_most_recent_requests[-1] - return nth_most_recent_request.created_at + 1.day + nth_most_recent_request.created_at + 1.day end def can_make_followup? - self.ban_text.empty? + ban_text.empty? end def can_make_comments? - self.ban_text.empty? + ban_text.empty? end def can_contact_other_users? - self.ban_text.empty? + ban_text.empty? end def can_fail_html if ban_text - text = self.ban_text.strip + text = ban_text.strip else raise "Unknown reason for ban" end text = CGI.escapeHTML(text) text = MySociety::Format.make_clickable(text, :contract => 1) text = text.gsub(/\n/, '<br>') - return text.html_safe + text.html_safe end # Returns domain part of user's email address def email_domain - return PublicBody.extract_domain_from_email(self.email) + PublicBody.extract_domain_from_email(email) end # A photograph of the user (to make it all more human) def set_profile_photo(new_profile_photo) ActiveRecord::Base.transaction do - if !self.profile_photo.nil? - self.profile_photo.destroy - end + profile_photo.destroy unless profile_photo.nil? self.profile_photo = new_profile_photo - self.save + save end end - # Used for default values of last_daily_track_email - def User.random_time_in_last_day - earliest_time = Time.now() - 1.day - latest_time = Time.now - return earliest_time + rand(latest_time - earliest_time).seconds - end - - # Alters last_daily_track_email for every user, so alerts will be sent - # spread out fairly evenly throughout the day, balancing load on the - # server. This is intended to be called by hand from the Ruby console. It - # will mean quite a few users may get more than one email alert the day you - # do it, so have a care and run it rarely. - # - # This SQL statement is useful for seeing how spread out users are at the moment: - # select extract(hour from last_daily_track_email) as h, count(*) from users group by extract(hour from last_daily_track_email) order by h; - def User.spread_alert_times_across_day - for user in self.find(:all) - user.last_daily_track_email = User.random_time_in_last_day - user.save! - end - nil # so doesn't print all users on console - end - # Return about me text for display as HTML # TODO: Move this to a view helper def get_about_me_for_html_display - text = self.about_me.strip + text = about_me.strip text = CGI.escapeHTML(text) text = MySociety::Format.make_clickable(text, :contract => 1) text = text.gsub(/\n/, '<br>') - return text.html_safe + text.html_safe end def json_for_api - return { - :id => self.id, - :url_name => self.url_name, - :name => self.name, - :ban_text => self.ban_text, - :about_me => self.about_me, + { + :id => id, + :url_name => url_name, + :name => name, + :ban_text => ban_text, + :about_me => about_me, # :profile_photo => self.profile_photo # ought to have this, but too hard to get URL out for now # created_at / updated_at we only show the year on the main page for privacy reasons, so don't put here } @@ -374,40 +388,41 @@ class User < ActiveRecord::Base def record_bounce(message) self.email_bounced_at = Time.now self.email_bounce_message = message - self.save! + save! end def should_be_emailed? - return (self.email_confirmed && self.email_bounced_at.nil?) + email_confirmed && email_bounced_at.nil? end def indexed_by_search? - return self.email_confirmed + email_confirmed end def for_admin_column(complete = false) if complete columns = self.class.content_columns else - columns = self.class.content_columns.map{|c| c if %w(created_at updated_at admin_level email_confirmed).include?(c.name) }.compact + columns = self.class.content_columns.map do |c| + c if %w(created_at updated_at admin_level email_confirmed).include?(c.name) + end.compact end columns.each do |column| - yield(column.human_name, self.send(column.name), column.type.to_s, column.name) + yield(column.human_name, send(column.name), column.type.to_s, column.name) end end - ## Private instance methods private def create_new_salt - self.salt = self.object_id.to_s + rand.to_s + self.salt = object_id.to_s + rand.to_s end def set_defaults - if self.admin_level.nil? + if admin_level.nil? self.admin_level = 'none' end - if self.new_record? + if new_record? # make alert emails go out at a random time for each new user, so # overall they are spread out throughout the day. self.last_daily_track_email = User.random_time_in_last_day @@ -415,35 +430,16 @@ class User < ActiveRecord::Base end def email_and_name_are_valid - if self.email != "" && !MySociety::Validate.is_valid_email(self.email) + if email != "" && !MySociety::Validate.is_valid_email(email) errors.add(:email, _("Please enter a valid email address")) end - if MySociety::Validate.is_valid_email(self.name) + if MySociety::Validate.is_valid_email(name) errors.add(:name, _("Please enter your name, not your email address, in the name field.")) end end - ## Class methods - def User.encrypted_password(password, salt) - string_to_hash = password + salt # TODO: need to add a secret here too? - Digest::SHA1.hexdigest(string_to_hash) - end - - def User.record_bounce_for_email(email, message) - user = User.find_user_by_email(email) - return false if user.nil? - - if user.email_bounced_at.nil? - user.record_bounce(message) - end - return true - end - - after_save(:purge_in_cache) - def purge_in_cache - if self.name_changed? - self.info_requests.each {|x| x.purge_in_cache} - end + def purge_in_cache + info_requests.each { |x| x.purge_in_cache } if name_changed? end end diff --git a/app/models/user_info_request_sent_alert.rb b/app/models/user_info_request_sent_alert.rb index 098b773f8..cd163d14b 100644 --- a/app/models/user_info_request_sent_alert.rb +++ b/app/models/user_info_request_sent_alert.rb @@ -17,18 +17,22 @@ # Email: hello@mysociety.org; WWW: http://www.mysociety.org/ class UserInfoRequestSentAlert < ActiveRecord::Base - belongs_to :user - belongs_to :info_request - - validates_inclusion_of :alert_type, :in => [ + ALERT_TYPES = [ 'overdue_1', # tell user that info request has become overdue 'very_overdue_1', # tell user that info request has become very overdue - 'new_response_reminder_1', # reminder user to classify the recent response - 'new_response_reminder_2', # repeat reminder user to classify the recent response - 'new_response_reminder_3', # repeat reminder user to classify the recent response - 'not_clarified_1', # reminder that user has to explain part of the request - 'comment_1', # tell user that info request has a new comment + 'new_response_reminder_1', # reminder user to classify the recent + # response + 'new_response_reminder_2', # repeat reminder user to classify the + # recent response + 'new_response_reminder_3', # repeat reminder user to classify the + # recent response + 'not_clarified_1', # reminder that user has to explain part of the + # request + 'comment_1' # tell user that info request has a new comment ] -end + belongs_to :user + belongs_to :info_request + validates_inclusion_of :alert_type, :in => ALERT_TYPES +end diff --git a/spec/models/change_email_validator_spec.rb b/spec/models/change_email_validator_spec.rb new file mode 100644 index 000000000..b667a23d1 --- /dev/null +++ b/spec/models/change_email_validator_spec.rb @@ -0,0 +1,124 @@ +require File.expand_path(File.dirname(__FILE__) + '/../spec_helper') + +def validator_with_user_and_params(user, params = {}) + validator = ChangeEmailValidator.new(params) + validator.logged_in_user = user + validator +end + +describe ChangeEmailValidator do + + let(:user) { FactoryGirl.create(:user) } + + describe :old_email do + + it 'must have an old email' do + params = { :old_email => nil, + :new_email => 'new@example.com', + :user_circumstance => 'change_email', + :password => 'jonespassword' } + validator = validator_with_user_and_params(user, params) + + msg = 'Please enter your old email address' + expect(validator.errors_on(:old_email)).to include(msg) + end + + it 'must be a valid email' do + params = { :old_email => 'old', + :new_email => 'new@example.com', + :user_circumstance => 'change_email', + :password => 'jonespassword' } + validator = validator_with_user_and_params(user, params) + + msg = "Old email doesn't look like a valid address" + expect(validator.errors_on(:old_email)).to include(msg) + end + + it 'must have the same email as the logged in user' do + params = { :old_email => user.email, + :new_email => 'new@example.com', + :user_circumstance => 'change_email', + :password => 'jonespassword' } + validator = validator_with_user_and_params(user, params) + validator.logged_in_user = FactoryGirl.build(:user) + + msg = "Old email address isn't the same as the address of the account you are logged in with" + expect(validator.errors_on(:old_email)).to include(msg) + end + + end + + describe :new_email do + + it 'must have a new email' do + params = { :old_email => user.email, + :new_email => nil, + :user_circumstance => 'change_email', + :password => 'jonespassword' } + validator = validator_with_user_and_params(user, params) + + msg = 'Please enter your new email address' + expect(validator.errors_on(:new_email)).to include(msg) + end + + it 'must be a valid email' do + params = { :old_email => user.email, + :new_email => 'new', + :user_circumstance => 'change_email', + :password => 'jonespassword' } + validator = validator_with_user_and_params(user, params) + + msg = "New email doesn't look like a valid address" + expect(validator.errors_on(:new_email)).to include(msg) + end + + end + + describe :password do + + pending 'password_and_format_of_email validation fails when password is nil' do + it 'must have a password' do + params = { :old_email => user.email, + :new_email => 'new@example.com', + :password => nil } + validator = validator_with_user_and_params(user, params) + + msg = 'Please enter your password' + expect(validator.errors_on(:password)).to include(msg) + end + end + + it 'does not require a password if changing email' do + params = { :old_email => user.email, + :new_email => 'new@example.com', + :user_circumstance => 'change_email', + :password => '' } + validator = validator_with_user_and_params(user, params) + + expect(validator).to have(0).errors_on(:password) + end + + it 'must have a password if not changing email' do + params = { :old_email => user.email, + :new_email => 'new@example.com', + :user_circumstance => 'unknown', + :password => '' } + validator = validator_with_user_and_params(user, params) + + msg = 'Please enter your password' + expect(validator.errors_on(:password)).to include(msg) + end + + it 'must be the correct password' do + params = { :old_email => user.email, + :new_email => 'new@example.com', + :password => 'incorrectpass' } + validator = validator_with_user_and_params(user, params) + + msg = 'Password is not correct' + expect(validator.errors_on(:password)).to include(msg) + end + + end + +end |