Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Adds new private thread notification #25

Closed
wants to merge 4 commits into from

3 participants

@TheDudeWithTheThing
Collaborator

For now it compares the latest private topic updated_at with the last
time a user read a private topic

@TheDudeWithTheThing TheDudeWithTheThing Adds new private thread notification
For now it compares the latest private topic updated_at with the last
time a user read a private topic
801eb85
app/models/thredded/private_topic.rb
@@ -38,5 +38,19 @@ def user_id=(ids)
def users_to_sentence
users.map{ |user| user.to_s.capitalize }.to_sentence
end
+
+ def self.unread_privates?(user)
+ max_private_topic_read =

naming is a little vague here - does "max" mean the very latest one? If so how about, like, most_recently_read_private_topic?

@TheDudeWithTheThing Collaborator

or just latest, we definitely want to indicate it's a date that we're getting here

latest_private_topic_read_date
and
latest_private_topic_date
?

@jayroh Owner
jayroh added a note

Is it a model/private_topic object we're getting back though? Or a date?

@TheDudeWithTheThing Collaborator

a date

.maximum('thredded_user_topic_reads.updated_at')
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
app/models/thredded/private_topic.rb
@@ -38,5 +38,19 @@ def user_id=(ids)
def users_to_sentence
users.map{ |user| user.to_s.capitalize }.to_sentence
end
+
+ def self.unread_privates?(user)
+ max_private_topic_read =
+ user
+ .thredded_private_topics
+ .includes(:user_topic_reads)
+ .where('thredded_user_topic_reads.user_id = ?', user.id)
+ .references(:user_topic_reads)
+ .maximum('thredded_user_topic_reads.updated_at')
+ return true if max_private_topic_read.blank?

Having to parse this mentally a little bit -
If there is no record found then we want to say "yes", there are unread messages? Is that right?
Wouldn't we want to it to return false? If I have no topics

@jayroh Owner
jayroh added a note

ha - wrong account.

@TheDudeWithTheThing Collaborator

no it means you never read a private topic, I guess the real check would be user.thredded_private_topics.count > 0 and max_private_topic_read.blank?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
app/models/thredded/private_topic.rb
@@ -38,5 +38,19 @@ def user_id=(ids)
def users_to_sentence
users.map{ |user| user.to_s.capitalize }.to_sentence
end
+
+ def self.unread_privates?(user)
+ max_private_topic_read =
+ user
+ .thredded_private_topics
+ .includes(:user_topic_reads)
+ .where('thredded_user_topic_reads.user_id = ?', user.id)
+ .references(:user_topic_reads)
+ .maximum('thredded_user_topic_reads.updated_at')
+ return true if max_private_topic_read.blank?
+
+ max_private_topic_date = user.thredded_private_topics.maximum('updated_at')
+ max_private_topic_date > max_private_topic_read
@jayroh Owner
jayroh added a note

here's where I'm getting tripped up. Looking at this it seems like we're comparing a date with an object

@jayroh Owner
jayroh added a note

maybe latest_updated_private_topic_date > latest_read_private_topic_date?

It might make it more obvious if the two objects were private methods and we had a predicate method with an obvious intention revealing name?

private_topics_newer_than_last_read_private_topic?

spit-balling here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jayroh jayroh commented on the diff
app/models/thredded/private_topic.rb
@@ -36,7 +36,21 @@ def user_id=(ids)
end
def users_to_sentence
- users.map{ |user| user.to_s.capitalize }.to_sentence
+ users.map { |user| user.to_s.capitalize }.to_sentence
+ end
+
+ def self.unread_privates?(user)
@jayroh Owner
jayroh added a note
def self.unread_privates?(user)
  completely_unread_private_topics_exist || latest_private_topic_date_newer_than_last_read
end

HOW ABOUT THAT SHIT?! This throws a whole lot of the onus in private instance methods but I think that's worthwhile. It's easier to figure out the work being done in those methods with names like that.

@TheDudeWithTheThing Collaborator
completely_unread_private_topics? || latest_private_topic_unread?
@jayroh Owner
jayroh added a note

:+1:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jayroh
Owner

Closing this PR since the other stuff made its way in. @TheDudeWithTheThing thank you for the hard work /bow

@TheDudeWithTheThing
Collaborator

you didn't close this ya weirdo

BYE

@change-collective
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Mar 19, 2014
  1. @TheDudeWithTheThing

    Adds new private thread notification

    TheDudeWithTheThing authored
    For now it compares the latest private topic updated_at with the last
    time a user read a private topic
Commits on Mar 20, 2014
  1. @TheDudeWithTheThing
  2. @TheDudeWithTheThing
Commits on Mar 21, 2014
  1. @TheDudeWithTheThing
This page is out of date. Refresh to see the latest.
View
33 app/models/thredded/private_topic.rb
@@ -36,7 +36,38 @@ def user_id=(ids)
end
def users_to_sentence
- users.map{ |user| user.to_s.capitalize }.to_sentence
+ users.map { |user| user.to_s.capitalize }.to_sentence
+ end
+
+ def self.unread_privates?(user)
@jayroh Owner
jayroh added a note
def self.unread_privates?(user)
  completely_unread_private_topics_exist || latest_private_topic_date_newer_than_last_read
end

HOW ABOUT THAT SHIT?! This throws a whole lot of the onus in private instance methods but I think that's worthwhile. It's easier to figure out the work being done in those methods with names like that.

@TheDudeWithTheThing Collaborator
completely_unread_private_topics? || latest_private_topic_unread?
@jayroh Owner
jayroh added a note

:+1:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ brand_new_private_topics?(user) || latest_private_topic_unread?(user)
+ end
+
+ private
+
+ def self.brand_new_private_topics?(user)
+ user_private_topic_count(user) > 0 && latest_private_topic_read_date(user).blank?
+ end
+
+ def self.latest_private_topic_unread?(user)
+ latest_private_topic_date(user) > latest_private_topic_read_date(user)
+ end
+
+ def self.latest_private_topic_date(user)
+ user.thredded_private_topics.maximum('updated_at')
+ end
+
+ def self.latest_private_topic_read_date(user)
+ user
+ .thredded_private_topics
+ .includes(:user_topic_reads)
+ .where('thredded_user_topic_reads.user_id = ?', user.id)
+ .references(:user_topic_reads)
+ .maximum('thredded_user_topic_reads.updated_at')
+ end
+
+ def self.user_private_topic_count(user)
+ user.thredded_private_topics.count
end
end
end
View
9 app/views/thredded/shared/_topic_nav.html.erb
@@ -9,11 +9,10 @@
<% end %>
<ul class="listings">
- <% cache ['topic-nav-list', current_user, messageboard] do %>
- <li class="topic_list"><%= link_to 'topics', messageboard_topics_path %></li>
- <% if can? :list, messageboard.private_topics.build %>
- <li class="private_topic_list"><%= link_to 'private topics', messageboard_private_topics_path %></li>
- <% end %>
+ <li class="topic_list"><%= link_to 'topics', messageboard_topics_path %></li>
+ <% if can? :list, messageboard.private_topics.build %>
+ <% unread = Thredded::PrivateTopic.unread_privates?(current_user) %>
+ <li class="private_topic_list"><%= link_to 'private topics', messageboard_private_topics_path, class: unread ? 'unread' : '' %></li>
<% end %>
<%= render 'thredded/search/form' %>
View
23 spec/models/thredded/private_topic_spec.rb
@@ -50,5 +50,28 @@ module Thredded
@topic.users_to_sentence.should eq 'Privateuser1 and Privateuser2'
end
end
+
+ describe '.unread_privates?' do
+ context 'when a user sends another user a PM' do
+ it 'the user should have unread private topics' do
+ expect(PrivateTopic.unread_privates?(@user2)).to be_true
+ end
+ end
+
+ context 'when a user reads a private thread' do
+ it 'the user should not have unread private topics' do
+ create(:user_topic_read, user: @user2, topic: @topic)
+ expect(PrivateTopic.unread_privates?(@user2)).to be_false
+ end
+ end
+
+ context 'when a user reads a PM and gets another PM' do
+ it 'the user should have unread private topics' do
+ create(:user_topic_read, user: @user2, topic: @topic)
+ create(:private_topic, messageboard: @messageboard, users: [@user1, @user2])
+ expect(PrivateTopic.unread_privates?(@user2)).to be_true
+ end
+ end
+ end
end
end
Something went wrong with that request. Please try again.