Facilitate comment reuse with a new History tab #3437

Merged
merged 6 commits into from Mar 27, 2017

Conversation

Projects
None yet
3 participants
@nilbus
Member

nilbus commented Mar 27, 2017

This is part of the experiment in exercism/discussions#123.

Feature flags:

  • comment_history: enable/disable
  • less_comment_history: show 25 historical comments instead of 50

Goal: Automate away what has already been mastered, so reviewers can focus on the real challenges for them. Focusing on the real challenges and not annoyances builds their sense of competence and therefore their intrinsic motivation.

This PR introduces a History tab that reveals your own previous comments, up to 50. Clicking a prior comment restores focus to the Write tab again and inserts the prior comment under any existing text, with a markdown divider if applicable.

screenshot

This feature requires Javascript; the History tab does not appear otherwise.

@kytrinyx This branch contains a migration that creates an index that makes user comment lookup efficient.

> puts User.find_by_username('alice').comments.recent(50).to_sql
   SELECT  "comments".* FROM "comments" WHERE "comments"."user_id" = 2
   ORDER BY comments.updated_at DESC LIMIT 50


exercism_development=# EXPLAIN ANALYZE
   SELECT  "comments".* FROM "comments" WHERE "comments"."user_id" = 2
   ORDER BY comments.updated_at DESC LIMIT 50;

      QUERY PLAN
------------------------------------------------------------------------------------------------------------------------------------------------------------------------
    Limit  (cost=0.28..173.77 rows=50 width=868) (actual time=0.023..0.121
rows=50 loops=1)
      ->  Index Scan Backward using
index_comments_on_user_id_and_updated_at on comments  (cost=0.28..309.10
rows=89 width=868) (actual time=0.023..0.114 rows=50 loops=1)
            Index Cond: (user_id = 2)
    Planning time: 0.132 ms
    Execution time: 0.149 ms

馃憤馃憤

nilbus added some commits Mar 26, 2017

Remove orphaned small_comment view
The last reference to rendering small_comment was removed in
e3648f0.
Implement efficient recent comment lookup
The new index on comments (user_id, updated_at) makes this lookup
beautifully efficient.

    > puts User.find_by_username('alice').comments.recent(50).to_sql
    SELECT  "comments".* FROM "comments" WHERE "comments"."user_id" = 2  ORDER BY comments.updated_at DESC LIMIT 50

    exercism_development=# EXPLAIN ANALYZE SELECT  "comments".* FROM "comments" WHERE "comments"."user_id" = 2  ORDER BY comments.updated_at DESC LIMIT 50;
                                                                                   QUERY PLAN
    ------------------------------------------------------------------------------------------------------------------------------------------------------------------------
     Limit  (cost=0.28..173.77 rows=50 width=868) (actual time=0.023..0.121 rows=50 loops=1)
       ->  Index Scan Backward using index_comments_on_user_id_and_updated_at on comments  (cost=0.28..309.10 rows=89 width=868) (actual time=0.023..0.114 rows=50 loops=1)
             Index Cond: (user_id = 2)
     Planning time: 0.132 ms
     Execution time: 0.149 ms
Plumb comment history to the submit_comment view
The comment_history local will not be passed from every route or view.
Do not assume that it will be defined; instead look it up in locals.
Use a class instead of id for write_tab
This is thinking ahead to a future change when we may have multiple
copies of these tabs on the page at once. Since this branch is about to
start referencing the write tab, making this change now will make the
future change easier by creating fewer things that need to change.

@nilbus nilbus referenced this pull request in exercism/discussions Mar 27, 2017

Closed

Studying game design for increasing code review participation #123

@@ -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)

This comment has been minimized.

@bernardoamc

bernardoamc Mar 27, 2017

Member

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.

@bernardoamc

bernardoamc Mar 27, 2017

Member

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.

This comment has been minimized.

@nilbus

nilbus Mar 27, 2017

Member

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. 馃槃

@nilbus

nilbus Mar 27, 2017

Member

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. 馃槃

This comment has been minimized.

@bernardoamc

bernardoamc Mar 27, 2017

Member

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. 馃

@bernardoamc

bernardoamc Mar 27, 2017

Member

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. 馃

This comment has been minimized.

@nilbus

nilbus Mar 29, 2017

Member

@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.

@nilbus

nilbus Mar 29, 2017

Member

@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.

app/routes/submissions.rb
+ comment_history = if current_user.guest? || !$flipper[:comment_history].enabled?(current_user)
+ []
+ else
+ current_user.comments.recent(25)

This comment has been minimized.

@bernardoamc

bernardoamc Mar 27, 2017

Member

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

@bernardoamc

bernardoamc Mar 27, 2017

Member

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

This comment has been minimized.

@nilbus

nilbus Mar 27, 2017

Member

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?

@nilbus

nilbus Mar 27, 2017

Member

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?

Make comment history quantity somewhat configurable via feature flag
Grab 50 comments by default, as described in the PR description.
Enable less_comment_history to cut it to 25.

These numbers are somewhat arbitrary. We should adjust after some
real-world experience.
@kytrinyx

This comment has been minimized.

Show comment
Hide comment
@kytrinyx

kytrinyx Mar 27, 2017

Member

This looks good. I'm going to merge it in as is, since the discussion is around an improvement/refactoring, which we can put in a separate pull request if we decide we want to go down that route.

Member

kytrinyx commented Mar 27, 2017

This looks good. I'm going to merge it in as is, since the discussion is around an improvement/refactoring, which we can put in a separate pull request if we decide we want to go down that route.

@kytrinyx kytrinyx merged commit a4ed8c2 into exercism:master Mar 27, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.009%) to 85.479%
Details

@nilbus nilbus deleted the nilbus:reuse-comments branch Mar 29, 2017

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