Add new feature : Filter posts by author #262

Closed
wants to merge 32 commits into
from

Conversation

Projects
None yet
6 participants
@bricesanchez
Member

bricesanchez commented Aug 7, 2012

No description provided.

@parndt

This comment has been minimized.

Show comment Hide comment
@parndt

parndt Aug 7, 2012

Member

I'm not sure about this one. It couples to Refinery::User and introduces a bit of overhead. The feature itself seems reasonable though.

Member

parndt commented Aug 7, 2012

I'm not sure about this one. It couples to Refinery::User and introduces a bit of overhead. The feature itself seems reasonable though.

@joemsak

This comment has been minimized.

Show comment Hide comment
@joemsak

joemsak Aug 7, 2012

Contributor

Agreed, I think if you can reference the Author without mentioning Refinery::User for sure, or maybe a

BlogPost.author_source ||= Refinery::User

, >_>

Contributor

joemsak commented Aug 7, 2012

Agreed, I think if you can reference the Author without mentioning Refinery::User for sure, or maybe a

BlogPost.author_source ||= Refinery::User

, >_>

@parndt

View changes

app/helpers/refinery/blog/controller_helper.rb
+ :joins => "LEFT JOIN refinery_blog_posts ON refinery_blog_posts.user_id = refinery_users.id",
+ :select => "refinery_users.id, refinery_users.username, count(*) AS post_count",
+ :group => "refinery_users.id")
+ end

This comment has been minimized.

Show comment Hide comment
@parndt

parndt Aug 7, 2012

Member

Why does this only find authors who have made a blog post before? I'm confused

@parndt

parndt Aug 7, 2012

Member

Why does this only find authors who have made a blog post before? I'm confused

This comment has been minimized.

Show comment Hide comment
@joemsak

joemsak Aug 7, 2012

Contributor

I mean, that /kind/ of makes sense, no?

@joemsak

joemsak Aug 7, 2012

Contributor

I mean, that /kind/ of makes sense, no?

This comment has been minimized.

Show comment Hide comment
@parndt

parndt Aug 7, 2012

Member

sort of.. here's another way:

@authors = Refinery::User.joins(:posts).select('id, username, count(*) AS post_count').group(Refinery::User.arel_table[:id])

But this also couples to Refinery::User

@parndt

parndt Aug 7, 2012

Member

sort of.. here's another way:

@authors = Refinery::User.joins(:posts).select('id, username, count(*) AS post_count').group(Refinery::User.arel_table[:id])

But this also couples to Refinery::User

This comment has been minimized.

Show comment Hide comment
@parndt

parndt Aug 7, 2012

Member

Does this list every single author and highlight the current author? Shouldn't it just show the author who actually wrote it? I'm a little confused about this part. If it just showed the current author then we wouldn't need the action whatsoever because we can do this:

<%= @post.author %>
@parndt

parndt Aug 7, 2012

Member

Does this list every single author and highlight the current author? Shouldn't it just show the author who actually wrote it? I'm a little confused about this part. If it just showed the current author then we wouldn't need the action whatsoever because we can do this:

<%= @post.author %>

This comment has been minimized.

Show comment Hide comment
@phiggins

phiggins Aug 7, 2012

Contributor

This should be in a model, no matter what it does, something like Refinery::User.with_post_counts.all or something more clever.

@phiggins

phiggins Aug 7, 2012

Contributor

This should be in a model, no matter what it does, something like Refinery::User.with_post_counts.all or something more clever.

This comment has been minimized.

Show comment Hide comment
@parndt

parndt Aug 7, 2012

Member

@phiggins yes; if this functionality is after all required then agreed.

@parndt

parndt Aug 7, 2012

Member

@phiggins yes; if this functionality is after all required then agreed.

This comment has been minimized.

Show comment Hide comment
@phiggins

phiggins Aug 7, 2012

Contributor

@parndt If this is indeed used, adding a post counter on Refinery::User might be a better way to do this.

@phiggins

phiggins Aug 7, 2012

Contributor

@parndt If this is indeed used, adding a post counter on Refinery::User might be a better way to do this.

This comment has been minimized.

Show comment Hide comment
@parndt

parndt Aug 7, 2012

Member

Agreed. I think first we would need to abstract Refinery::User to something else as @joemsak suggested

@parndt

parndt Aug 7, 2012

Member

Agreed. I think first we would need to abstract Refinery::User to something else as @joemsak suggested

This comment has been minimized.

Show comment Hide comment
@bricesanchez

bricesanchez Aug 8, 2012

Member

I'm sorry but i don't understand what i need to do...

Do i need to move this portion of code in a model ?
Do i need to use this line of code :

@authors = Refinery::User.joins(:posts).select('id, username, count(*) AS post_count').group(Refinery::User.arel_table[:id])

If yes, i have an error : "Association named posts was not found; perhaps you misspelled it?"

@bricesanchez

bricesanchez Aug 8, 2012

Member

I'm sorry but i don't understand what i need to do...

Do i need to move this portion of code in a model ?
Do i need to use this line of code :

@authors = Refinery::User.joins(:posts).select('id, username, count(*) AS post_count').group(Refinery::User.arel_table[:id])

If yes, i have an error : "Association named posts was not found; perhaps you misspelled it?"

@parndt

View changes

app/controllers/refinery/blog/authors_controller.rb
+
+ def show
+ @author = Refinery::User.find(params[:id])
+ @posts = Refinery::Blog::Post.where(:user_id => @author.id).live.includes(:comments, :categories).page(params[:page])

This comment has been minimized.

Show comment Hide comment
@parndt

parndt Aug 7, 2012

Member

This can be done using the @author:

@posts = @author.posts.live.includes(:comments, :categories).page(params[:page])
@parndt

parndt Aug 7, 2012

Member

This can be done using the @author:

@posts = @author.posts.live.includes(:comments, :categories).page(params[:page])

This comment has been minimized.

Show comment Hide comment
@bricesanchez

bricesanchez Aug 8, 2012

Member

I have this error when a use your line of code :
undefined method `posts' for #Refinery::User:0x007fad5233e788

It's why i used ruby Refinery::Blog::Post...

@bricesanchez

bricesanchez Aug 8, 2012

Member

I have this error when a use your line of code :
undefined method `posts' for #Refinery::User:0x007fad5233e788

It's why i used ruby Refinery::Blog::Post...

@parndt

View changes

app/views/refinery/blog/shared/_authors.html.erb
+ <nav id="authors">
+ <ul>
+ <% @authors.each do |author| %>
+ <li<%= " class='selected'" if @author.present? and @author.id == author.id %>>

This comment has been minimized.

Show comment Hide comment
@parndt

parndt Aug 7, 2012

Member

does this mean a blog post can only have a single author?

@parndt

parndt Aug 7, 2012

Member

does this mean a blog post can only have a single author?

This comment has been minimized.

Show comment Hide comment
@phiggins

phiggins Aug 7, 2012

Contributor

@author == author will suffice as the condition.

@phiggins

phiggins Aug 7, 2012

Contributor

@author == author will suffice as the condition.

@parndt

View changes

app/views/refinery/blog/shared/_authors.html.erb
+ <% end %>
+ </ul>
+ </nav>
+<% end %>

This comment has been minimized.

Show comment Hide comment
@parndt

parndt Aug 7, 2012

Member

Where is this new partial actually used? :)

@parndt

parndt Aug 7, 2012

Member

Where is this new partial actually used? :)

This comment has been minimized.

Show comment Hide comment
@parndt

parndt Aug 7, 2012

Member

Also the partial uses 4 space indentation whereas the rest of the project uses 2 spaces.

@parndt

parndt Aug 7, 2012

Member

Also the partial uses 4 space indentation whereas the rest of the project uses 2 spaces.

@parndt

View changes

app/views/refinery/blog/authors/show.html.erb
@@ -0,0 +1,23 @@
+<% content_for :title, "#{t('.posts_written_by')} '#{@author.username}'" %>
+
+<% content_for :body_content_title, "#{t('.posts_written_by')} &#8220;#{@author.username}&#8221;".html_safe -%>

This comment has been minimized.

Show comment Hide comment
@parndt

parndt Aug 7, 2012

Member

I would introduce the username as a parameter to the localisation so that it can read properly in other languages that might put the username somewhere else in the sentence e.g.:

<% content_for :body_content_title, t('.posts_written_by', :name => @author.username) %>
@parndt

parndt Aug 7, 2012

Member

I would introduce the username as a parameter to the localisation so that it can read properly in other languages that might put the username somewhere else in the sentence e.g.:

<% content_for :body_content_title, t('.posts_written_by', :name => @author.username) %>

This comment has been minimized.

Show comment Hide comment
@phiggins

phiggins Aug 7, 2012

Contributor

👍 translation should take care of interpolating.

@phiggins

phiggins Aug 7, 2012

Contributor

👍 translation should take care of interpolating.

@parndt

View changes

app/views/refinery/blog/authors/show.html.erb
@@ -0,0 +1,23 @@
+<% content_for :title, "#{t('.posts_written_by')} '#{@author.username}'" %>

This comment has been minimized.

Show comment Hide comment
@parndt

parndt Aug 7, 2012

Member

Why does this exist as well as the one for body_content_title and why do they differ?

@parndt

parndt Aug 7, 2012

Member

Why does this exist as well as the one for body_content_title and why do they differ?

@parndt

This comment has been minimized.

Show comment Hide comment
@parndt

parndt Aug 7, 2012

Member

Hi @bricesanchez thanks for the pull request. I've added quite a few comments to it sorry :-)

Another thing worth mentioning is that you've targeted this at the 2-0-stable branch but we can't introduce new features like this into a stable branch. It would have to go into master.

Thanks again :)

Member

parndt commented Aug 7, 2012

Hi @bricesanchez thanks for the pull request. I've added quite a few comments to it sorry :-)

Another thing worth mentioning is that you've targeted this at the 2-0-stable branch but we can't introduce new features like this into a stable branch. It would have to go into master.

Thanks again :)

@bricesanchez

This comment has been minimized.

Show comment Hide comment
@bricesanchez

bricesanchez Aug 7, 2012

Member

Hi everybody,

I have followed your comments in live, I was stressed like during an exam =)

I'm new in Rails developpement and i understand that my work is not the most beautiful... it's normal !

Could you make changes in my code in order to fit like you think ?

It will be nice if this feature is going into master =)

Thanks for your job on refinerycms, i love it.
At this time and for me, it's the best CMS in open source (and i have worked on TYPO3, Wordpress, Drupal, Joomla!,... so i can compare!)

Member

bricesanchez commented Aug 7, 2012

Hi everybody,

I have followed your comments in live, I was stressed like during an exam =)

I'm new in Rails developpement and i understand that my work is not the most beautiful... it's normal !

Could you make changes in my code in order to fit like you think ?

It will be nice if this feature is going into master =)

Thanks for your job on refinerycms, i love it.
At this time and for me, it's the best CMS in open source (and i have worked on TYPO3, Wordpress, Drupal, Joomla!,... so i can compare!)

@ugisozols

This comment has been minimized.

Show comment Hide comment
@ugisozols

ugisozols Aug 8, 2012

Member

@bricesanchez it would be nice if you could add the changes yourself. @parndt already provided code so you have a base to work on.

One more thing - specs! If you can't do them I'll be more than happy to help out.

Member

ugisozols commented Aug 8, 2012

@bricesanchez it would be nice if you could add the changes yourself. @parndt already provided code so you have a base to work on.

One more thing - specs! If you can't do them I'll be more than happy to help out.

@parndt

This comment has been minimized.

Show comment Hide comment
@parndt

parndt Aug 8, 2012

Member

@joemsak yeah exactly. I think that it would probably be best as a configuration option like:

Refinery::Blog.configure do |config|
  config.user_model = Refinery::User
end
Member

parndt commented Aug 8, 2012

@joemsak yeah exactly. I think that it would probably be best as a configuration option like:

Refinery::Blog.configure do |config|
  config.user_model = Refinery::User
end
@parndt

This comment has been minimized.

Show comment Hide comment
@parndt

parndt Sep 5, 2012

Member

@ugisozols if you could help with specs that'd be great. This will also need a big squash :) and it can't go in 2-0-stable.

Member

parndt commented Sep 5, 2012

@ugisozols if you could help with specs that'd be great. This will also need a big squash :) and it can't go in 2-0-stable.

@ugisozols

This comment has been minimized.

Show comment Hide comment
@ugisozols

ugisozols Sep 5, 2012

Member

I can help but first we have to decide if we're going to use:

Refinery::Blog.configure do |config|
  config.user_model = Refinery::User
end

... or something else.

Next this PR should be opened against master. @bricesanchez can you squash your commits and send a new PR? Thanks.

Member

ugisozols commented Sep 5, 2012

I can help but first we have to decide if we're going to use:

Refinery::Blog.configure do |config|
  config.user_model = Refinery::User
end

... or something else.

Next this PR should be opened against master. @bricesanchez can you squash your commits and send a new PR? Thanks.

@parndt

This comment has been minimized.

Show comment Hide comment
@parndt

parndt Nov 22, 2012

Member

Agreed this has to go against master and should use the features introduced in #290 (when it gets reviewed too)

Member

parndt commented Nov 22, 2012

Agreed this has to go against master and should use the features introduced in #290 (when it gets reviewed too)

@parndt parndt closed this Nov 22, 2012

@cpreisinger

This comment has been minimized.

Show comment Hide comment
@cpreisinger

cpreisinger Jul 2, 2013

Was this feature never actually pulled into a current release of the gem? @ugisozols @parndt @bricesanchez

Was this feature never actually pulled into a current release of the gem? @ugisozols @parndt @bricesanchez

@parndt

This comment has been minimized.

Show comment Hide comment
@parndt

parndt Jul 2, 2013

Member

@cpreisinger I don't think so. It's just a matter of moving the submitted commits to master and making sure it works.

Member

parndt commented Jul 2, 2013

@cpreisinger I don't think so. It's just a matter of moving the submitted commits to master and making sure it works.

@cpreisinger

This comment has been minimized.

Show comment Hide comment
@cpreisinger

cpreisinger Jul 2, 2013

@parndt Cool thank you, I'll make sure everything works alright.

@parndt Cool thank you, I'll make sure everything works alright.

@parndt

This comment has been minimized.

Show comment Hide comment
@parndt

parndt Jul 2, 2013

Member

Appreciate it ❤️

Member

parndt commented Jul 2, 2013

Appreciate it ❤️

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