Adds new private thread notification #25

Closed
wants to merge 4 commits into
from

Conversation

Projects
None yet
3 participants
Collaborator

TheDudeWithTheThing commented Mar 20, 2014

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

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
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 =
@ghost

ghost Mar 20, 2014

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

@TheDudeWithTheThing

TheDudeWithTheThing Mar 20, 2014

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

jayroh Mar 20, 2014

Owner

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

@TheDudeWithTheThing

TheDudeWithTheThing Mar 20, 2014

Collaborator

a date

.maximum('thredded_user_topic_reads.updated_at')
app/models/thredded/private_topic.rb
+ .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?
@ghost

ghost Mar 20, 2014

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

jayroh Mar 20, 2014

Owner

ha - wrong account.

@TheDudeWithTheThing

TheDudeWithTheThing Mar 20, 2014

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?

app/models/thredded/private_topic.rb
+ 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

jayroh Mar 20, 2014

Owner

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

@jayroh

jayroh Mar 20, 2014

Owner

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

+ users.map { |user| user.to_s.capitalize }.to_sentence
+ end
+
+ def self.unread_privates?(user)
@jayroh

jayroh Mar 20, 2014

Owner
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

TheDudeWithTheThing Mar 20, 2014

Collaborator
completely_unread_private_topics? || latest_private_topic_unread?
Owner

jayroh commented Sep 2, 2014

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

Collaborator

TheDudeWithTheThing commented Oct 17, 2014

you didn't close this ya weirdo

BYE

@ghost

ghost commented Oct 17, 2014

NOOOOOOOO

On Fri, Oct 17, 2014 at 10:09 AM, Shaun notifications@github.com wrote:

Closed #25 jayroh#25.


Reply to this email directly or view it on GitHub
jayroh#25 (comment).

@glebm glebm deleted the private-topic-notification branch Aug 15, 2017

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