Skip to content

Commit

Permalink
extract TopicNotifier class from topic
Browse files Browse the repository at this point in the history
  • Loading branch information
mattvanhorn committed May 24, 2013
1 parent 247a0b3 commit d7817cf
Show file tree
Hide file tree
Showing 4 changed files with 69 additions and 28 deletions.
2 changes: 1 addition & 1 deletion app/controllers/topics_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -203,7 +203,7 @@ def toggle_mute(v)
@topic = Topic.where(id: params[:topic_id].to_i).first
guardian.ensure_can_see!(@topic)

@topic.toggle_mute(current_user, v)
@topic.toggle_mute(current_user)
render nothing: true
end

Expand Down
49 changes: 24 additions & 25 deletions app/models/topic.rb
Original file line number Diff line number Diff line change
Expand Up @@ -112,9 +112,7 @@ def initialize(user)

after_create do
changed_to_category(category)
TopicUser.change(user_id, id,
notification_level: TopicUser.notification_levels[:watching],
notifications_reason_id: TopicUser.notification_reasons[:created_topic])
notifier.created_topic! user_id
if archetype == Archetype.private_message
DraftSequence.next!(user, Draft::NEW_PRIVATE_MESSAGE)
else
Expand Down Expand Up @@ -162,10 +160,7 @@ def fancy_title
end

def sanitize_title
if self.title.present?
self.title = sanitize(title, tags: [], attributes: [])
self.title.strip!
end
self.title = sanitize(title.to_s, tags: [], attributes: []).strip.presence
end

def new_version_required?
Expand Down Expand Up @@ -641,11 +636,6 @@ def self.starred_counts_per_day(sinceDaysAgo=30)
TopicUser.where('starred_at > ?', sinceDaysAgo.days.ago).group('date(starred_at)').order('date(starred_at)').count
end

# Enable/disable the mute on the topic
def toggle_mute(user, muted)
TopicUser.change(user, self.id, notification_level: muted?(user) ? TopicUser.notification_levels[:regular] : TopicUser.notification_levels[:muted] )

This comment has been minimized.

Copy link
@mattvanhorn

mattvanhorn May 24, 2013

Author Owner

the parameter muted is not ever used in this method.

This comment has been minimized.

Copy link
@eviltrout

eviltrout May 24, 2013

huh, so it isn't!

end

def slug
unless slug = read_attribute(:slug)
return '' unless title.present?
Expand All @@ -661,17 +651,17 @@ def slug
end

def title=(t)
slug = ""
slug = (Slug.for(t).presence || "topic") if t.present?
slug = (Slug.for(t.to_s).presence || "topic")
write_attribute(:slug, slug)
write_attribute(:title,t)
end

# NOTE: These are probably better off somewhere else.
# Having a model know about URLs seems a bit strange.
def last_post_url
"/t/#{slug}/#{id}/#{posts_count}"
end


def self.url(id, slug, post_number=nil)
url = "#{Discourse.base_url}/t/#{slug}/#{id}"
url << "/#{post_number}" if post_number.to_i > 1
Expand All @@ -684,12 +674,6 @@ def relative_url(post_number=nil)
url
end

def muted?(user)
return false unless user && user.id
tu = topic_users.where(user_id: user.id).first
tu && tu.notification_level == TopicUser.notification_levels[:muted]
end

def clear_pin_for(user)
return unless user.present?
TopicUser.change(user.id, id, cleared_pinned_at: Time.now)
Expand All @@ -703,21 +687,36 @@ def draft_key
"#{Draft::EXISTING_TOPIC}#{id}"
end

def notifier
@topic_notifier ||= TopicNotifier.new(self)
end

# notification stuff
def notify_watch!(user)
TopicUser.change(user, id, notification_level: TopicUser.notification_levels[:watching])
notifier.watch! user
end

def notify_tracking!(user)
TopicUser.change(user, id, notification_level: TopicUser.notification_levels[:tracking])
notifier.tracking! user
end

def notify_regular!(user)
TopicUser.change(user, id, notification_level: TopicUser.notification_levels[:regular])
notifier.regular! user
end

def notify_muted!(user)
TopicUser.change(user, id, notification_level: TopicUser.notification_levels[:muted])
notifier.muted! user
end

def muted?(user)
if user && user.id
notifier.muted?(user.id)
end
end

# Enable/disable the mute on the topic
def toggle_mute(user_id)
notifier.toggle_mute user_id
end

def auto_close_days=(num_days)
Expand Down
42 changes: 42 additions & 0 deletions app/models/topic_notifier.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
class TopicNotifier
def initialize(topic)
@topic = topic
end

{ :watch! => :watching,
:tracking! => :tracking,
:regular! => :regular,
:muted! => :muted }.each_pair do |method_name, level|

define_method method_name do |user_id|
change_level user_id, level
end

end

def created_topic!(user_id)
change_level user_id, :watching, :created_topic
end

# Enable/disable the mute on the topic
def toggle_mute(user_id)
change_level user_id, (muted?(user_id) ? levels[:regular] : levels[:muted])
end

def muted?(user_id)
tu = @topic.topic_users.where(user_id: user_id).first
tu && tu.notification_level == levels[:muted]
end

private

def levels
@notification_levels ||= TopicUser.notification_levels
end

def change_level(user_id, level, reason=nil)
attrs = {notification_level: levels[level]}
attrs.merge!(notifications_reason_id: TopicUser.notification_reasons[reason]) if reason
TopicUser.change(user_id, @topic.id, attrs)
end
end
4 changes: 2 additions & 2 deletions spec/controllers/topics_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -282,12 +282,12 @@
end

it "changes the user's starred flag when the parameter is present" do
Topic.any_instance.expects(:toggle_mute).with(@topic.user, true)
Topic.any_instance.expects(:toggle_mute).with(@topic.user)
xhr :put, :mute, topic_id: @topic.id, starred: 'true'
end

it "removes the user's starred flag when the parameter is not true" do
Topic.any_instance.expects(:toggle_mute).with(@topic.user, false)
Topic.any_instance.expects(:toggle_mute).with(@topic.user)
xhr :put, :unmute, topic_id: @topic.id, starred: 'false'
end

Expand Down

2 comments on commit d7817cf

@eviltrout
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm is there a concern that the toggle_mute no longer takes a value?

I agree toggle implies that you're swapping it with whatever was there so maybe a term like adjust_mute would be more accurate. But previously, if a request came in for mute twice in a row, it would stay muted wouldn't it? Whereas now a second request for mute would unmute it?

@mattvanhorn
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The value was not being used anywhere, and it was (and is) a true toggle.
I thought about adding an unmute! method to go with mute!, if you think we could use it.
then it would look like:
def toggle_mute
muted?(user_id) ? unmute! : mute!
end

Please sign in to comment.