-
Notifications
You must be signed in to change notification settings - Fork 4
fix: guest users can now read articles #22
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
WalkthroughThe article show view now conditionally renders the comment form based on article existence, user authentication, and author status. Non-authenticated users or article authors see a sign-up/sign-in prompt instead of the comment form. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
app/views/articles/show.html.erb (4)
72-86: Misleading message shown to authenticated article authors.The current logic treats two distinct cases identically:
- Guest users (not logged in)
- Authenticated authors viewing their own article
Both see "Sign up/in to add a comment", but this is confusing for authors who are already signed in. Consider differentiating these cases.
Apply this diff to provide appropriate messaging:
- <% if @article %> - <% if @current_user && @current_user.username != @article.author.username %> + <% if @article %> + <% if @current_user %> + <% if @current_user.username != @article.author.username %> <%= form_with(model: [@article, @comment], local: true, url: article_comments_path(@article.id), method: :post, class: 'card comment-form') do |form| %> <div class="card-block"> <%= form.text_area :body, class: 'form-control', placeholder: 'Write a comment...', rows: 3 %> </div> <div class="card-footer"> <img src="<%= current_user.image %>" class="comment-author-img" /> <button class="btn btn-sm btn-primary">Post Comment</button> </div> - <% end %> + <% end %> + <% else %> + <div class="card-block"> + <p>Authors cannot comment on their own articles.</p> + </div> + <% end %> <% else %> <div class="card-block"> <p>Sign up/in to add a comment.</p> </div> <% end %>
91-110: Critical: Comments loop will crash if article is nil.The comments display loop on line 91 is positioned outside the
<% if @article %>check (which ends at line 89). If@articleis nil,@article.comments.eachwill raise a NoMethodError, defeating the purpose of this PR fix.Apply this diff to move the comments loop inside the article check:
- <% else %> - <p>Article not found.</p> - <% end %> - <% @article.comments.each do |comment| %> <div class="card"> <div class="card-block"> <p class="card-text"><%= comment.body %></p> </div> <div class="card-footer"> <%= link_to show_users_path(comment.author.username), class: 'comment-author' do %> <%= image_tag comment.author.image, class: 'comment-author-img' %> <% end %> <%= link_to comment.author.username, show_users_path(comment.author.username), class: 'comment-author' %> <span class="date-posted"><%= comment.created_at.strftime('%B %d, %Y') %></span> <% if current_user == comment.author %> <span class="mod-options"> <%= button_to '', article_comments_path(@article.id, comment), method: :delete, class: 'ion-trash-a' %> </span> <% end %> </div> </div> <% end %> + <% else %> + <p>Article not found.</p> + <% end %>
20-24: Favorite buttons will fail for guest users.The Favorite/Unfavorite buttons are displayed to all users, including guests, without authentication checks. When clicked by a guest user, these will trigger authentication errors.
Apply this diff to conditionally show these buttons only to authenticated users:
- <% if @is_favorited %> - <%= button_to 'Unfavorite Post', unfavorite_article_path(@article.slug), method: :delete, class: 'btn btn-sm btn-outline-primary' %> - <% else %> - <%= button_to 'Favorite Post', favorite_article_path(@article.slug), method: :post, class: 'btn btn-sm btn-outline-primary' %> - <% end %> + <% if @current_user %> + <% if @is_favorited %> + <%= button_to 'Unfavorite Post', unfavorite_article_path(@article.slug), method: :delete, class: 'btn btn-sm btn-outline-primary' %> + <% else %> + <%= button_to 'Favorite Post', favorite_article_path(@article.slug), method: :post, class: 'btn btn-sm btn-outline-primary' %> + <% end %> + <% end %>
56-62: Follow and Favorite buttons will fail for guest users.Similar to the issue above, the Follow and Favorite/Unfavorite buttons in the article actions section are displayed without authentication checks. Guest users will encounter errors when clicking these buttons.
Apply this diff to conditionally show these buttons only to authenticated users:
- <%= button_to 'Follow', follow_profile_path(@article.author.username), method: :post, class: 'btn btn-sm btn-outline-secondary' %> - - <% if @is_favorited %> - <%= button_to 'Unfavorite Post', unfavorite_article_path(@article.slug), method: :delete, class: 'btn btn-sm btn-outline-primary' %> - <% else %> - <%= button_to 'Favorite Post', favorite_article_path(@article.slug), method: :post, class: 'btn btn-sm btn-outline-primary' %> - <% end %> + <% if @current_user %> + <%= button_to 'Follow', follow_profile_path(@article.author.username), method: :post, class: 'btn btn-sm btn-outline-secondary' %> + + <% if @is_favorited %> + <%= button_to 'Unfavorite Post', unfavorite_article_path(@article.slug), method: :delete, class: 'btn btn-sm btn-outline-primary' %> + <% else %> + <%= button_to 'Favorite Post', favorite_article_path(@article.slug), method: :post, class: 'btn btn-sm btn-outline-primary' %> + <% end %> + <% end %>
🧹 Nitpick comments (1)
app/views/articles/show.html.erb (1)
14-18: Inconsistent use of@current_uservscurrent_user.The template mixes
@current_user(instance variable) andcurrent_user(helper method) throughout. Lines 14 and 72 use@current_user, while lines 15, 25, 63, 78, and 103 usecurrent_user. This inconsistency can lead to subtle bugs if they're not always synchronized.Standardize on the Rails convention of using the
current_userhelper method throughout the template. Replace all instances of@current_userwithcurrent_userfor consistency.Also applies to: 25-25, 63-63, 72-72, 78-78, 103-103
This address a problem where, if you create an article and try to access without signing, would throw an error.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.