"Browse other activities" next to "replicate this" prompt, fixes #754 #1090

500swapnil · 2016-12-12T19:40:52Z

Make sure these boxes are checked before your pull request is ready to be reviewed and merged. Thanks!

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-12-12T20:02:21Z

app/views/notes/_responses.html.erb

+    <p id="<%= response_type %>s"><a class='btn btn-primary btn-lg' href="/post?tags=<%= response_type %>:<%= @node.id %><% if response_type == 'response' %>,hidden:response<% end %><%= ',' + (@tagnames.uniq.delete_if{|x| x.match(":") }).join(',') if @tagnames && @tagnames.length > 0 %>"><%= t('notes._responses.post_' + response_type) %></a>&nbsp;
+    <% if @node.power_tag('activity') %>
+    	<% if DrupalTag.where(name: @node.power_tag('activity')) %>
+    		<% @responses = DrupalTag.where(name: @node.power_tag('activity')).tag.nodes.first %><% end %><% end %></p>


OK, - closer, great -- here, I don't think we need an instance variable for @responses -- but we could say:

<% if @node.power_tag('activity') %> <% if DrupalTag.where(name: @node.power_tag('activity')).first.nodes %> <%= DrupalTag.where(name: @node.power_tag('activity')).first.nodes.first.path %> <% end %> <% end %>

Please also put end statements and close </p> tags on their own line, with clean indentations (two spaces per indent) -- that makes it a lot easier to read!

See how this is a lot of logic to put into a template? If we imagined we'd use this logic more than just once, we might want to move some of it into the DrupalTag model, to simplify the code here. Then we could also unit test the code to ensure it doesn't break in the future. But it's your choice -- if it's just going to be used here, we're OK just doing a little bit of logic in the templates, as long as its readable and efficient.

And see how we ask @node.power_tag('activity') three times? We could make that a bit more efficient too -- although it will probably cache the database call that fetches that info, so it's not too bad.

Okay it isnt used that much so we will see about adding it to the DrupalTag model. I will make these changes. And don't we need all this in 'Browse other activities' link and '' tags?

jywarren · 2016-12-12T22:23:41Z

OK - great - see how the tests ran, and some failed? This highlights exactly the IF checks we need -- i guess we forgot at least one. So the first test failure is:

Error: test_users_are_logged_out_and_alerted_when_banned,_and_notes_are_not_accessible(ModerateAndBanTest): ActionView::Template::Error: undefined method `nodes' for nil:NilClass

And note that these failed on the following line: app/views/notes/_responses.html.erb:10

So, we're calling .nodes on a nil -- that nil was the result of:

DrupalTag.where(name: @node.power_tag('activity')).first

So i guess we have to add another check that the above call doesn't return nil.

You can also write shorter checks by using this neat .try() method in ActiveSupport. That can perhaps keep your code a bit more compact, if it's helpful.

Thanks!!!

jywarren · 2016-12-12T22:24:20Z

This is a great example of how testing can help catch problems before we merge code!

500swapnil · 2016-12-12T23:01:11Z

don't we need all this in 'Browse other activities' link and '' tags?

jywarren · 2016-12-12T23:18:15Z

app/views/notes/_responses.html.erb

+      <% if @node.power_tag('activity') %>
+        <% if DrupalTag.where(name: @node.power_tag('activity')).first %>
+          <% if DrupalTag.where(name: @node.power_tag('activity')).first.nodes %>
+            <%= DrupalTag.where(name: @node.power_tag('activity')).first.nodes.first.path %>


Not sure if this answers your question, but here, we now can make this appear in a link, like:

<a href="<%= DrupalTag.where(name: @node.power_tag('activity')).first.nodes.first.path %>">Browse other activities for "<%= @node.power_tag('activity') %>"</a>

Make sense?

Got it! Yes it clears it up. Thanks!

500swapnil · 2016-12-12T23:27:34Z

Now do we need to add a test for this link in **** ?

jywarren · 2016-12-12T23:33:27Z

I think we could... You could add an extra assert_select to an existing test on a node tagged with "activity:...." -- there should be one in /test/functional/notes_controller_test.rb, though you might have to add an id= property to the <a> tag so you can correctly select it. I can help with this tomorrow!

…

On Dec 12, 2016 6:27 PM, "500swapnil" ***@***.***> wrote: Now do we need to add a test for this link in **** ? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#1090 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABfJxw_UPXH7S5vfatg_UzdRvkmS-02ks5rHdhmgaJpZM4LK8Ot> .

500swapnil · 2016-12-12T23:44:07Z

So I guess it would be
assert_select "a.other-activites" , true
where the id of '' is 'other-activites'. Ok let me know where I can add this tomorrow. Thanks!

jywarren · 2016-12-13T00:10:36Z

Yes, but # instead of . for ids -- periods are for class= I can provide more input tomorrow!

…

On Dec 12, 2016 6:44 PM, "500swapnil" ***@***.***> wrote: So I guess it would be assert_select "a.other-activites" , true where the id of '' is 'other-activites'. Ok let me know where I can add this tomorrow. Thanks! — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#1090 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABfJ31JaN3xVyArmiTQkq73uknOxTqYks5rHdxJgaJpZM4LK8Ot> .

jywarren · 2016-12-13T02:26:16Z

So, if it's an id, you do assert_select "a#other-activities" (true is implied with only one param), and the test could go among these similar ones:

https://github.com/publiclab/plots2/blob/master/test/integration/node_insert_extras_test.rb#L47

500swapnil · 2016-12-13T12:52:34Z

Is this ok?

jywarren · 2016-12-13T13:15:42Z

Hi -- this looks perfect; merging now. If you're looking for a new challenge, please take a look at our help-wanted list: https://github.com/publiclab/plots2/labels/help-wanted

Thanks!

500swapnil · 2016-12-13T14:41:10Z

Thanks!

ebarry · 2016-12-21T16:13:55Z

Thanks so much @500swapnil !!!!! This is a really useful addition to helping everyone find the projects they are interested in.

500swapnil · 2016-12-21T16:55:47Z

Happy to help! @ebarry

Add link for browsing other activities

0cfdce6

This was referenced Dec 12, 2016

"Browse other activities" next to "replicate this" prompt #754

Closed

"Browse other activities" next to "replicate this" prompt (2) , fixes #754 #1083

Closed

jywarren reviewed Dec 12, 2016

View reviewed changes

Update _responses.html.erb

256b4de

Update _responses.html.erb

1209f89

jywarren reviewed Dec 12, 2016

View reviewed changes

Update _responses.html.erb

326754a

Swapnil Gupta and others added 2 commits December 13, 2016 13:23

Added test for link

039b616

Update node_insert_extras_test.rb

51ce02d

jywarren merged commit 3cbe709 into publiclab:master Dec 13, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

"Browse other activities" next to "replicate this" prompt, fixes #754 #1090

"Browse other activities" next to "replicate this" prompt, fixes #754 #1090

500swapnil commented Dec 12, 2016 •

edited by jywarren

Loading

jywarren Dec 12, 2016

jywarren Dec 12, 2016

500swapnil Dec 12, 2016

jywarren commented Dec 12, 2016

jywarren commented Dec 12, 2016

500swapnil commented Dec 12, 2016

jywarren Dec 12, 2016

500swapnil Dec 12, 2016

500swapnil commented Dec 12, 2016

jywarren commented Dec 12, 2016 via email

500swapnil commented Dec 12, 2016

jywarren commented Dec 13, 2016 via email

jywarren commented Dec 13, 2016

500swapnil commented Dec 13, 2016

jywarren commented Dec 13, 2016

500swapnil commented Dec 13, 2016

ebarry commented Dec 21, 2016

500swapnil commented Dec 21, 2016

"Browse other activities" next to "replicate this" prompt, fixes #754 #1090

"Browse other activities" next to "replicate this" prompt, fixes #754 #1090

Conversation

500swapnil commented Dec 12, 2016 • edited by jywarren Loading

jywarren Dec 12, 2016

Choose a reason for hiding this comment

jywarren Dec 12, 2016

Choose a reason for hiding this comment

500swapnil Dec 12, 2016

Choose a reason for hiding this comment

jywarren commented Dec 12, 2016

jywarren commented Dec 12, 2016

500swapnil commented Dec 12, 2016

jywarren Dec 12, 2016

Choose a reason for hiding this comment

500swapnil Dec 12, 2016

Choose a reason for hiding this comment

500swapnil commented Dec 12, 2016

jywarren commented Dec 12, 2016 via email

500swapnil commented Dec 12, 2016

jywarren commented Dec 13, 2016 via email

jywarren commented Dec 13, 2016

500swapnil commented Dec 13, 2016

jywarren commented Dec 13, 2016

500swapnil commented Dec 13, 2016

ebarry commented Dec 21, 2016

500swapnil commented Dec 21, 2016

500swapnil commented Dec 12, 2016 •

edited by jywarren

Loading