[in-progress]Fix/change drupal comments to comment. Part of #956 #978

ghost · 2016-11-10T03:20:01Z

Currently, there is an issue with the "comments" method in node_shared.rb conflicting with its comments object, whereas previously it simply referred to self.drupal_comments, now it refers to self.comments and thus recurses. @aayushgupta1 and @jywarren we need to work together and figure out the best way to refactor the method name so it doesn't conflict with the comments object itself. (just removing the method causes all kinds of problems in tests with comment ordering.

Related: http://stackoverflow.com/a/5446209

Anyone curious see discussion in #956

all tests pass -- rake test:all
code is in uniquely-named feature branch, and has been rebased on top of latest master (especially if you've been asked to make additional changes)
pull request is descriptively named with #number reference back to original issue
if possible, multiple commits squashed if they're smaller changes
reviewed/confirmed/tested by another contributor or maintainer
schema.rb.example has been updated if any database migrations were added

Please be sure you've reviewed our contribution guidelines at https://publiclab.org/wiki/contributing-to-public-lab-software

We have a loose schedule of reviewing and pulling in changes every Tuesday and Friday, and publishing changes on Fridays. Please alert developers on plots-dev@googlegroups.com when your request is ready or if you need assistance.

Thanks!

jywarren · 2016-11-14T22:50:04Z

Hi, I left a longer comment with next steps at #956 -- thanks!

jywarren · 2016-11-22T18:48:38Z

Hi, just checking if you've gotten stuck on this at all, or if I could help in any way?

Thanks!

ghost · 2016-11-23T02:42:11Z

Yes I did get a bit stuck and overwhelmed actually, thanks for asking :) I
really wasn't sure whether I was being lazy or just not familiar enough
with the codebase... then I had to go get batteries for my keyboard!

basically I changed the one instance where it called that method, after
eliminating that method, and a bunch of tests to do with the ordering of
the comments started failing - like my first issue all over again lol. I
think it's being called in more places than we realize. I'll re-open the PR
in a bit so you can see the tests that fail :)

On Wed, Nov 23, 2016 at 2:48 AM Jeffrey Warren notifications@github.com
wrote:

Hi, just checking if you've gotten stuck on this at all, or if I could
help in any way?

Thanks!

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#978 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AOKKP9-3tYBGcD2JidYkSj-jogHZ1ZsBks5rAzkHgaJpZM4KuPVF
.

ghost · 2016-11-23T09:20:02Z

@jywarren I just pushed my work and it's testing it now. The problem seems to be with answers not showing up in the correct order. I looked in models/answers.rb and the answers_controller.rb and they have no mention of anything to do with comments. Are you sure that one instance in answers's show.html.erb is the only use of the comments method from node_shared? maybe it's being inadvertently called somewhere? :/

jywarren · 2016-11-23T16:29:22Z

Well, this doesn't look too bad, and it makes some sense too -- since we removed:

.order("timestamp #{direction}")

from here, the failing test which uses .comments should no longer expect that method to be sorted ASC by default. So we should tweak that test to match -- just add the .order(...) method, or potentially just change .first to .last, so it no longer expects the sorting to be done already.

Make sense?

jywarren · 2016-11-23T16:30:07Z

test/unit/answer_test.rb

@@ -46,6 +46,6 @@ class AnswerTest < ActiveSupport::TestCase

 test "should list comments in descending order" do
 answer = answers(:one)
- assert_equal answer.comments.first, DrupalComment.last
+ assert_equal answer.comments.first, Comment.last


Here, as I described in my longer comment, use Comment.last since we can no longer expect default ordering.

jywarren · 2016-11-23T16:31:05Z

And thanks for your hard work, this is a complex one and you're doing really well on it. It's the kind of issue folks have shied away from in the past, honestly, and you're almost there!

ghost · 2016-11-23T17:09:10Z

@jywarren I fixed it using the changes you suggested. I didn't catch that even though I stared at that test, thanks for pointing that out and all your support with my PR(s)!

It happens because comments.order timestamp ASC returns a collection that goes "backwards" - the first element is the most recent as the number is higher in that case. I guess the default comments method returns a descending list by default, so it will be the last comment.

I'm pretty tired so I can't think much more about it tonight, but I'll review it tomorrow. I'd also like to ask some advice about mastering development and which issue would be best to tackle next if it's not too much trouble... What would be the most convenient way for you for me to contact you?

The more time goes on the more I realize I have a personality that doesn't ask for help enough when getting stuck - I feel like I won't understand the problem if I don't solve it by myself, but while I may not understand it 100% as well that way I can gain a more rapid understanding of rails and the codebase by asking for help when I get stuck, which also means making contributions. 80% * 10 issues > 100% * 1 or 2 issues. I should change that as learning by doing (as opposed to boring courses that cover the same stuff again and again and are comprehensively instead of practically focused) working with an actual team and project on a real codebase where I can be mentored and recieve good code review in exchange for helping is one of my main motivations for contributing on github, and my other one is doing meaningful work and being valued :P

jywarren · 2016-11-23T19:23:48Z

Also -- one suggestion I thought of was that for a deeper understanding of the application architecture, you might want to take a look at breaking up a complex feature proposal into smaller parts, which is something we could definitely use help with. Check out some of the ideas for ways to get involved beyond just fixing self-contained issues:

https://publiclab.org/notes/warren/11-08-2016/help-public-lab-s-software-grow-by-joining-a-supportive-team

Specifically, we were going to propose another role called "architects" or something, who are folks who can plan out new features, and likewise break them up into clean modular parts. We have a whole collection of both bugs and feature requests that really aren't broken up yet, and so lack a clear next step:

break-me-up break up for cleaner code separation, discrete tests, and, easier and iterative collaboration

If that's something you're interested in, I'm happy to help you choose one and make some suggestions. Or feel free to tackle another normal-sized issue and circle back to this suggestion later -- i just thought it might be of interest.

jywarren · 2016-11-23T19:30:24Z

OK we're really almost there. Just found a few spots where we have to re-add the .order(...) method. Then I'll merge!

jywarren

Here's the spots I think we should change. I haven't used this "review" feature before so forgive me if I do this a little oddly, but I hope this is clear. Thanks!

jywarren · 2016-11-23T19:26:45Z

app/views/notes/_comments.html.erb


 <div id="comments-container">
- <% @node.drupal_comments.each do |comment| %>
- <% if comment.cid == @node.drupal_comments.last.cid %><a id="last" name="last"></a><% end %>
+ <% @node.comments.each do |comment| %>


Here I guess we'd want to reverse the order with a .order('timestamp ASC') since that's no longer automatic.

jywarren · 2016-11-23T19:27:48Z

app/views/notes/_comments.html.erb

- <% @node.drupal_comments.each do |comment| %>
- <% if comment.cid == @node.drupal_comments.last.cid %><a id="last" name="last"></a><% end %>
+ <% @node.comments.each do |comment| %>
+ <% if comment.cid == @node.comments.last.cid %><a id="last" name="last"></a><% end %>


Likewise here, .last won't return the right cid due to the new ordering.

jywarren · 2016-11-23T19:29:34Z

test/unit/comment_test.rb

- assert_equal user.drupal_comments.last, comment
- assert_equal answer.drupal_comments.last, comment
- assert_not_equal answer.node.drupal_comments.last, comment
+ assert_equal user.comments.last, comment


I think this isn't an issue now because there's only one comment for the user bob, but it will be if the fixtures are ever expanded. Best make this .first or add the ordering to make it extra clear to future contributors.

and on the subsequent two lines as well!

Hmm, 144 fails when its changed to .first? Hmm... ah but I see -- it's not a node.comments call, it's a user.comments call -- so... i guess we didn't change ordering there and .last is fine. Sorry!

jywarren · 2016-11-23T20:01:48Z

Oof, i realized my earlier comment never posted successfully! Imagine I said this like about an hour ago:

Wow, all checks passed! 🎉 🚀 🎸

I'll read through carefully but that's a really great sign.

I'm happy to look at potential next issues, starting with this list: fto-candidate issues which are meant to be solved by first timers but aren't well-formatted yet and for some more challenging ones, https://github.com/publiclab/plots2/labels/help-wanted

I'd also say that given this particular tough nut of an issue, you're in a good position to offer some guidance and support to others tackling similar issues from #956. As you saw, it's a complex one!

I'd say that you're already digging into deeper issues than the typical contributor, and I can tell that you like to know not just that something works, but why -- which is super. Rails is a bit of a magical beast, and sometimes those explanations are not always as easy to find as in other frameworks -- but it's also super powerful and fast to write if you do know the conventions. I'm happy to try to find an issue which can give you some more insight into Rails architecture.

Please feel free to email me at jeff@publiclab.org with other questions you'd prefer to ask offline, and once again, thank you so much for your contribution!

jywarren · 2016-11-28T20:10:23Z

Just these last three changes left, @ashleypt - just wanted to be sure you saw. Eager to merge this in, thanks a million! 🎈

ghost · 2016-11-29T09:20:22Z

@jywarren ah I see you minused those lines, not modified them. is the last one the only one that should be there? the one on line 144 failed with your suggestion

Removed comments method in node_shared.rb and uses of it Fix answer_test.rb: was previously expecting the comments method which returned an ascending list by default Fixed comments ordering in comment_test and notes/_comments partial view Removed some tests in comments_test.rb

jywarren · 2016-11-29T16:19:30Z

Merged, awesome! Thanks a lot! 🎈

ghost mentioned this pull request Nov 10, 2016

Deprecate Drupal legacy database structures/naming #956

Open

16 tasks

jywarren reviewed Nov 23, 2016

View reviewed changes

jywarren requested changes Nov 23, 2016

View reviewed changes

ghost closed this Nov 29, 2016

ghost reopened this Nov 29, 2016

ghost closed this Nov 29, 2016

ghost reopened this Nov 29, 2016

jywarren approved these changes Nov 29, 2016

View reviewed changes

jywarren merged commit daccb7c into publiclab:master Nov 29, 2016

jywarren added a commit to jywarren/plots2 that referenced this pull request Nov 29, 2016

quick fix to migrations order, subtle, related to publiclab#978

d2d0994

jywarren added a commit that referenced this pull request Nov 29, 2016

quick fix to migrations order, subtle, related to #978 (#1030)

9c25b5a

ghost deleted the fix/change-drupal-comments-to-comment branch November 29, 2016 22:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[in-progress]Fix/change drupal comments to comment. Part of #956 #978

[in-progress]Fix/change drupal comments to comment. Part of #956 #978

ghost commented Nov 10, 2016 •

edited by ghost

Loading

jywarren commented Nov 14, 2016

jywarren commented Nov 22, 2016

ghost commented Nov 23, 2016

ghost commented Nov 23, 2016

jywarren commented Nov 23, 2016

jywarren Nov 23, 2016

jywarren commented Nov 23, 2016

ghost commented Nov 23, 2016

jywarren commented Nov 23, 2016

jywarren commented Nov 23, 2016

jywarren left a comment

jywarren Nov 23, 2016

jywarren Nov 23, 2016

jywarren Nov 23, 2016

jywarren Nov 23, 2016

jywarren Nov 29, 2016

jywarren commented Nov 23, 2016

jywarren commented Nov 28, 2016

ghost commented Nov 29, 2016

jywarren commented Nov 29, 2016

[in-progress]Fix/change drupal comments to comment. Part of #956 #978

[in-progress]Fix/change drupal comments to comment. Part of #956 #978

Conversation

ghost commented Nov 10, 2016 • edited by ghost Loading

jywarren commented Nov 14, 2016

jywarren commented Nov 22, 2016

ghost commented Nov 23, 2016

ghost commented Nov 23, 2016

jywarren commented Nov 23, 2016

jywarren Nov 23, 2016

Choose a reason for hiding this comment

jywarren commented Nov 23, 2016

ghost commented Nov 23, 2016

jywarren commented Nov 23, 2016

jywarren commented Nov 23, 2016

jywarren left a comment

Choose a reason for hiding this comment

jywarren Nov 23, 2016

Choose a reason for hiding this comment

jywarren Nov 23, 2016

Choose a reason for hiding this comment

jywarren Nov 23, 2016

Choose a reason for hiding this comment

jywarren Nov 23, 2016

Choose a reason for hiding this comment

jywarren Nov 29, 2016

Choose a reason for hiding this comment

jywarren commented Nov 23, 2016

jywarren commented Nov 28, 2016

ghost commented Nov 29, 2016

jywarren commented Nov 29, 2016

ghost commented Nov 10, 2016 •

edited by ghost

Loading