Replace all uses of wiki.slug in favor of wiki.path, fixes #736 #1101

500swapnil · 2016-12-14T20:05:40Z

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-14T20:26:00Z

Aha - i see this is using some ERB templating code in the test:

Failure: Expected response to be a redirect to <"http://test.host/wiki/revisions/<%= @node.slug %>?_=1481746385"> but was a redirect to <"http://test.host/wiki/revisions/<%= @node.slug_from_path %>?_=1481746385">.

For that we ought to use normal Ruby string substitution, like: "/wiki/revisions/" + @node.slug and "/wiki/revisions/" + @node.slug_from_path -- so both in the test and also in the redirect itself:

https://github.com/publiclab/plots2/pull/1101/files#diff-6d0eec853a873b030e87f5a21aecdf63L150

and

https://github.com/publiclab/plots2/blob/master/test/functional/admin_controller_test.rb#L324

It looks like that code was already wrong, actually! It was not running the ERB replacements, just spitting out the whole thing as a string, see? So it's great that we caught that in our revisions here.

500swapnil · 2016-12-14T20:27:28Z

Yeah that is great! I shall make the changes now!

500swapnil · 2016-12-14T20:35:46Z

Done! So the @node.slug_from_path function cannot be accessed from a Ruby string and needs to be added separately. That was the problem?

jywarren · 2016-12-14T20:44:42Z

Ah, looks like perhaps it should be @revision.node.slug_from_path since in that action we haven't defined @node. Fingers crossed that that'll get tests to pass!

500swapnil · 2016-12-14T20:49:12Z

I defined the function slug_from_path in drupal_node.rb. So shouldnt it be @DrupalNode.slug_from_path?

jywarren · 2016-12-14T20:54:07Z

DrupalNode is the classname -- so you can use it to make a new node like DrupalNode.new. But once you make one, like: node = DrupalNode.new then you refer to it by the variable you've stored the instance in. In this case, we didn't make a `node` or `@node` variable, but we can access the node we want because `@revision` has a `.node` method which fetches it.

…

On Wed, Dec 14, 2016 at 3:49 PM, 500swapnil ***@***.***> wrote: I defined the function slug_from_path in drupal_node.rb. So shouldnt it be @DrupalNode.slug_from_path? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#1101 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABfJ4MME1_Sd-baONcKn3pfaoLXBOVIks5rIFZIgaJpZM4LNWvs> .

500swapnil · 2016-12-14T20:57:27Z

Okay

jywarren · 2016-12-14T21:09:57Z

Ah - sorry, looks like we need @revision.drupal_node not .node -- apologies!

500swapnil · 2016-12-14T21:10:25Z

No problem. I had that doubt

500swapnil · 2016-12-14T21:28:14Z

Same error
Any ideas?

jywarren · 2016-12-14T21:54:01Z

Checking now! Also, you can always ping @publiclab/reviewers for help if i'm not available -- no guarantee, but sometimes if I'm busy with other work and you really want to make progress, it's worth it. Thanks!

jywarren · 2016-12-14T22:18:46Z

test/functional/admin_controller_test.rb

@@ -321,7 +321,7 @@ def teardown
    assert_equal 0, revision.status
    assert_equal 0, revision.author.status
    assert_not_equal node(:spam_targeted_page).latest.vid, revision.vid
-    assert_redirected_to "/wiki/revisions/<%= @node.slug %>" + '?_=' + Time.now.to_i.to_s
+    assert_redirected_to "/wiki/revisions/" + @revision.drupal_node.slug_from_path + '?_=' + Time.now.to_i.to_s


OK -- while in the controller, we used an instance variable (starting with @) in the test, we are just using a local variable, so no @ required. Check each changed line to ensure we're using what's available there -- either @node or @revision or node or revision -- and match the usage in that context.

jywarren · 2016-12-14T22:19:21Z

app/views/wiki/show.html.erb

@@ -77,7 +76,7 @@
      <% if @node.power_tags('tabbed').include?("notes") %>
        <div class="tab-pane" id="tab-notes">
          <%= render :partial => "notes/notes" %>
-          <p><a href="/tag/<%= @node.slug %>"><%= raw t('wiki.show.more_research', :tag => @node.slug) %> &raquo;</a></p>
+          <p><a href="/tag/<%= @revision.drupal_node.slug_from_path %>"><%= raw t('wiki.show.more_research', :tag => @revision.drupal_node.slug_from_path) %> &raquo;</a></p>


Yes -- see how here, we can indeed use @node -- it was only in some contexts where @revision was required, not all. Thanks!

I went through the errors. @node is fine for all except admin_controller.rb and admin_controller_test.rb.
What should I use in admin_controller.rb and admin_controller_test.rb??

Is there any way of testing a part of code and not the whole code locally? Some command instead of rake test:all .

On this line in the admin controller test, revision.drupal_node:

https://github.com/publiclab/plots2/blob/master/test/functional/admin_controller_test.rb#L324

And on this line, @revision.drupal_node:

https://github.com/publiclab/plots2/blob/master/app/controllers/admin_controller.rb#L150

Yes, you can run just functional tests with rake test:functionals, units with rake test:units, integration with rake test:integration, and you can even run just one test file with:

ruby -I test test/functional/notes_controller_test.rb (for example)

I know it's time consuming to run them all!

Okay I will work on it now!

500swapnil · 2016-12-15T21:54:47Z

Fine now?

jywarren · 2016-12-15T22:26:51Z

I'll take a look but I have to head out now -- later tonight I hope! Thanks!

jywarren · 2016-12-16T17:36:45Z

Gemfile.lock

@@ -339,3 +339,9 @@ DEPENDENCIES
  uglifier (>= 1.0.3)
  will_paginate (>= 3.0.6)
  will_paginate-bootstrap (>= 1.0.1)
+
+RUBY VERSION


I'd like to not include Gemfile or Gemfile.lock changes if not essential, but I'll leave these in this time. Thanks!

jywarren · 2016-12-16T17:38:45Z

Looks great! merging now. Thanks for your careful work on this one!

500swapnil · 2016-12-16T17:39:33Z

Thank you! Any idea on which issue I should try next?

jywarren · 2016-12-16T17:52:17Z

Yes -- there are some sub-issues to this one that'd be great to have some assistance on! #397

For example, the checks on the edit and update actions, and the corresponding tests!

Replace slug by path

997fd8b

500swapnil mentioned this pull request Dec 14, 2016

Replace all uses of wiki.slug in favor of wiki.path #736

Closed

Fixed test for node.slug_from_path

ea8df9c

Added @revision

29b0bdb

add drupal_node

6d5aa2d

jywarren reviewed Dec 14, 2016

View reviewed changes

Update on removing slugs

59e0872

jywarren reviewed Dec 16, 2016

View reviewed changes

jywarren merged commit 7984631 into publiclab:master Dec 16, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace all uses of wiki.slug in favor of wiki.path, fixes #736 #1101

Replace all uses of wiki.slug in favor of wiki.path, fixes #736 #1101

500swapnil commented Dec 14, 2016 •

edited by jywarren

Loading

jywarren commented Dec 14, 2016

500swapnil commented Dec 14, 2016

500swapnil commented Dec 14, 2016

jywarren commented Dec 14, 2016

500swapnil commented Dec 14, 2016

jywarren commented Dec 14, 2016 via email

500swapnil commented Dec 14, 2016

jywarren commented Dec 14, 2016

500swapnil commented Dec 14, 2016

500swapnil commented Dec 14, 2016 •

edited

Loading

jywarren commented Dec 14, 2016

jywarren Dec 14, 2016

jywarren Dec 14, 2016

500swapnil Dec 15, 2016

500swapnil Dec 15, 2016

jywarren Dec 15, 2016

500swapnil Dec 15, 2016

500swapnil commented Dec 15, 2016

jywarren commented Dec 15, 2016

jywarren Dec 16, 2016

jywarren commented Dec 16, 2016

500swapnil commented Dec 16, 2016

jywarren commented Dec 16, 2016

Replace all uses of wiki.slug in favor of wiki.path, fixes #736 #1101

Replace all uses of wiki.slug in favor of wiki.path, fixes #736 #1101

Conversation

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

jywarren commented Dec 14, 2016

500swapnil commented Dec 14, 2016

500swapnil commented Dec 14, 2016

jywarren commented Dec 14, 2016

500swapnil commented Dec 14, 2016

jywarren commented Dec 14, 2016 via email

500swapnil commented Dec 14, 2016

jywarren commented Dec 14, 2016

500swapnil commented Dec 14, 2016

500swapnil commented Dec 14, 2016 • edited Loading

jywarren commented Dec 14, 2016

jywarren Dec 14, 2016

Choose a reason for hiding this comment

jywarren Dec 14, 2016

Choose a reason for hiding this comment

500swapnil Dec 15, 2016

Choose a reason for hiding this comment

500swapnil Dec 15, 2016

Choose a reason for hiding this comment

jywarren Dec 15, 2016

Choose a reason for hiding this comment

500swapnil Dec 15, 2016

Choose a reason for hiding this comment

500swapnil commented Dec 15, 2016

jywarren commented Dec 15, 2016

jywarren Dec 16, 2016

Choose a reason for hiding this comment

jywarren commented Dec 16, 2016

500swapnil commented Dec 16, 2016

jywarren commented Dec 16, 2016

500swapnil commented Dec 14, 2016 •

edited by jywarren

Loading

500swapnil commented Dec 14, 2016 •

edited

Loading