Skip to content
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

Jump to bottom

Facilitate comment reuse with a new History tab #3437

Merged
merged 6 commits into from Mar 27, 2017
Merged

Facilitate comment reuse with a new History tab #3437

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
392e626
Remove orphaned small_comment view
nilbus Mar 26, 2017
0e72a56
Implement efficient recent comment lookup
nilbus Mar 26, 2017
382c2a0
Plumb comment history to the submit_comment view
nilbus Mar 26, 2017
4ddfde6
Use a class instead of id for write_tab
nilbus Mar 27, 2017
13e2b0f
Display prior comments in the History tab
nilbus Mar 27, 2017
f910947
Make comment history quantity somewhat configurable via feature flag
nilbus Mar 27, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 7 additions & 0 deletions app/routes/submissions.rb
View file
Open in desktop
Expand Up @@ -15,8 +15,15 @@ class Submissions < Core

title("%s by %s in %s" % [submission.problem.name, submission.user.username, submission.problem.language])

comment_history = if current_user.guest? || !$flipper[:comment_history].enabled?(current_user)
Copy link

Choose a reason for hiding this comment

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

May I suggest an structure like lib/flipper/comment_history.rb ?

This class would be initialized with the current user and flipper and it would be easy to test. Another benefit is that it would be easy to remove this feature in the future. Future toggle features would also be easily spotted since they would all be located in the same folder.

Copy link
Author

Choose a reason for hiding this comment

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

If I follow you, you mean something like…

module Flipper
  class CommentHistory
    def comment_history(user:)
      if $flipper[:comment_history].enabled?(current_user)
        current_user.comments.recent(25)
      else
        []
      end
    end
  end
end

I'm not convinced for this particular scenario yet, but I want to make sure I understand what you're suggesting too. There are several non-user-visible changes that aren't turned off by the feature flag (e.g. schema changes, plumbing locals through erb views, coffeescript classes that are defined but unused) that it seems like, at least with how I implemented most of these features, that I don't feel like it would be easier or less complex to remove. Easier to test… I could be convinced that's true.

I'd love to hear about some examples where this approach has helped you. I assume it has, or I bet you wouldn't be suggesting it. 😄

Copy link

Choose a reason for hiding this comment

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

The main benefit was being able to test that the data returned for a feature was correct/expected, same when the feature was not enabled. If I remember correctly I also made something like current_user.feature?(:my_feature) which returned the appropriate permission for an user or a guest. Not really saying this would work in exercism context though, just sharing experiences. 😄

The presentation layer is always complicated, I relied on presenters to do the trick when the feature changed the presentation layer too much, but in this case I think it would be overkill. 🤔

Copy link
Author

@nilbus nilbus Mar 29, 2017 •
edited

Choose a reason for hiding this comment

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

@bernardoamc Cool. I actually think that makes a lot of sense, especially when you're replacing one implementation with another. If you can extract the behavior into its own class, then you can duplicate that class and modify it to be the new behavior, and switch between implementations using a feature flag. Is that along the lines of what you're thinking? It wouldn't even necessarily need to go in a separate Flipper module namespace.

If the behavior/output of both implementations is intended to be the same, the scientist gem is also particularly helpful.

[]
else
current_user.comments.recent(25)
Copy link

Choose a reason for hiding this comment

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

Every time an user checks a submission a query like this will be triggered, let's keep an eye on performance issues.

Copy link
Author

Choose a reason for hiding this comment

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

The new index should theoretically make this perform well, but it wouldn't hurt to check after its release. @kytrinyx Does Heroku provide you database load metrics?

end

locals = {
submission: submission,
comment_history: comment_history,
own_uuid: submission.exercise_uuid_by(current_user),
}

Expand Down
15 changes: 14 additions & 1 deletion app/views/submissions/comment.erb
View file
Open in desktop
@@ -1,6 +1,6 @@
<div ng-controller="MarkdownCtrl" class="md-markdown-preview">
<tabset>
<tab id="write_tab" heading="Write">
<tab class="write_tab" heading="Write">
<textarea
id="submission_comment"
name="body"
Expand All @@ -15,5 +15,18 @@
<div class="preview" ng-bind-html-unsafe="data.preview"></div>
<hr>
</tab>
<% if $flipper[:comment_history].enabled?(current_user) && !locals[:comment_history].blank? %>
<tab heading="History" class="hidden history_tab">
<h5>Select a comment to insert as a starting point</h5>
<div class="list-group">
<% locals[:comment_history].each do |comment| %>
<button type="button" class="list-group-item history_item">
<%= comment.body %>
</button>
<% end %>
</div>
<hr>
</tab>
<% end %>
</tabset>
</div>
2 changes: 1 addition & 1 deletion app/views/submissions/nitpicks.erb
View file
Open in desktop
Expand Up @@ -14,5 +14,5 @@
<% end %>

<hr>
<%= erb :"submissions/submit_comment", locals: {submission: submission} %>
<%= erb :"submissions/submit_comment", locals: {submission: submission, comment_history: locals[:comment_history]} %>
</div>
2 changes: 1 addition & 1 deletion app/views/submissions/show.erb
View file
Open in desktop
Expand Up @@ -87,7 +87,7 @@
<div class="submission-iteration">
<div class="row">
<%= erb :"submissions/current_submission", locals: { submission: submission } %>
<%= erb :"submissions/nitpicks", locals: { submission: submission, subscribed: ConversationSubscription.subscribed?(current_user, submission) } %>
<%= erb :"submissions/nitpicks", locals: { submission: submission, comment_history: locals[:comment_history], subscribed: ConversationSubscription.subscribed?(current_user, submission) } %>
</div>
</div>
</div>
11 changes: 0 additions & 11 deletions app/views/submissions/small_comment.erb
View file
Open in desktop

This file was deleted.

2 changes: 1 addition & 1 deletion app/views/submissions/submit_comment.erb
View file
Open in desktop
Expand Up @@ -2,7 +2,7 @@
<p> You're not logged in right now. Please login via <a href="/login">GitHub </a> to comment </p>
<% else %>
<form accept-charset="UTF-8" action="/submissions/<%= submission.key %>/nitpick" method="POST">
<%= erb :"submissions/comment", locals: { submission: submission } %>
<%= erb :"submissions/comment", locals: { submission: submission, comment_history: locals[:comment_history] } %>
<button type="submit" class="pull-right btn btn-primary btn-lg" name="nitpick">
Submit Comment
</button>
Expand Down
5 changes: 5 additions & 0 deletions db/migrate/201703261609_index_comments_by_user_id.rb
View file
Open in desktop
@@ -0,0 +1,5 @@
class IndexCommentsByUserId < ActiveRecord::Migration
def change
add_index :comments, [:user_id, :updated_at]
end
end
37 changes: 37 additions & 0 deletions frontend/app/js/app.coffee
View file
Open in desktop
Expand Up @@ -7,6 +7,9 @@ $ ->
$('.submission-prompt').each (i, promptContainer) ->
new SubmissionPrompt(promptContainer)

$('.history_tab').each (i, historyTab) ->
new CommentHistoryTab(historyTab)

$("[data-toggle=tooltip]").tooltip()

$("[data-search=tags]").autoComplete
Expand Down Expand Up @@ -185,3 +188,37 @@ class SubmissionPrompt
"Will this solution be performant at scale? In what cases might efficient performance matter?"
"Would you want to want to work in a codebase composed of code similar to this? What do you like and dislike about it?"
]

class CommentHistoryTab
constructor: (historyTabElement) ->
@historyTab = $(historyTabElement)
@writeTabLink = @historyTab.siblings('.write_tab').find('a')
@writeTabTextarea = @historyTab.closest('.tabbable').find('textarea.comment')
@historyItems = @historyTab.closest('.tabbable').find('.history_item')
@initializeUI()

initializeUI: ->
@historyItems.click @selectItemCallback
@historyTab.removeClass('hidden') # Leave hidden when JS not enabled

selectItemCallback: (event) =>
event.preventDefault()
historyItem = $(event.target).closest('.history_item')
historicalComment = historyItem.text()
@appendText(historicalComment)
@refreshPreviewContent()
@activateWriteTab()

appendText: (newText) ->
existingText = @writeTabTextarea.val().trim()
replacementText = ''
replacementText += existingText + "\n\n---\n\n" unless _.isEmpty(existingText)
replacementText += newText.trim()
@writeTabTextarea.val replacementText

refreshPreviewContent: ->
@writeTabTextarea.trigger 'input'

activateWriteTab: ->
@writeTabLink.trigger 'click'
@writeTabTextarea.focus()
2 changes: 1 addition & 1 deletion frontend/app/js/script.js
View file
Open in desktop
Expand Up @@ -145,7 +145,7 @@ $(function() {
nitQuoted += '\n';

// switch to 'Write' tab incase 'Preview' tab was selected
$("#write_tab").find('a').trigger('click');
$(".write_tab").find('a').trigger('click');

// set reply
var submissionCommentTextArea = $('#submission_comment');
Expand Down
6 changes: 5 additions & 1 deletion lib/exercism/user.rb
View file
Open in desktop
Expand Up @@ -6,7 +6,11 @@ class User < ActiveRecord::Base

has_many :submissions
has_many :notifications
has_many :comments
has_many :comments do
def recent(count)
order('comments.updated_at DESC').limit(count)
end
end
has_many :dailies, -> (user) { limit(Daily::LIMIT - user.daily_count) }
has_many :daily_counts
has_many :exercises, class_name: "UserExercise"
Expand Down
5 changes: 5 additions & 0 deletions test/exercism/user_test.rb
View file
Open in desktop
Expand Up @@ -90,6 +90,11 @@ def test_create_users_unless_present
assert_equal %w(alice bob charlie), User.find_or_create_in_usernames(%w(alice BOB charlie)).map(&:username).sort
end

def test_recent_comments
user = User.create!(username: 'bob')
assert_equal 0, user.comments.recent(10).count
end

def test_delete_team_memberships_with_user
alice = User.create(username: 'alice')
bob = User.create(username: 'bob')
Expand Down