Added test to block basic user edits, closes issue #397 #1114

500swapnil · 2016-12-16T18:41: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!

500swapnil · 2016-12-16T18:51:52Z

It failed the tests!

jywarren · 2016-12-16T22:16:19Z

All right -- that's good, but check why it failed (click "Details" on the test check box) -- was it that it didn't show the right message?

+ assert_select ".alert", "expected message"

We should put a message there as outlined in #397 --

  flash.now[:warning] = "This page is <a href='/wiki/power-tags#Locking'>locked</a>, and only <a href='/wiki/moderators'>moderators</a> can edit it."

Then, try to implement the feature itself -- as described in #397 --

Look to see if the "locked" tag exists, and if it does, redirects back to the wiki page with an error message, by adding just after this line: https://github.com/publiclab/plots2/blob/master/app/controllers/wiki_controller.rb#L74 as follows:

if @node.has_power_tag('locked') && (current_user.role != "admin" && current_user.role != "moderator")
  flash.now[:warning] = "This page is <a href='/wiki/power-tags#Locking'>locked</a>, and only <a href='/wiki/moderators'>moderators</a> can edit it."
  redirect_to @node.path
end

Then the test should pass (if it's written properly! I might've made errors...) because it'll match the right message.

You could also enclose it in a block to make sure that the total # of DrupalNodeRevisions doesn't change, like:

assert_difference 'DrupalNodeRevision.count', 0 do
  # put your POST call inside here
end

That's a nice type of test -- it'll ensure that within the block, the number of revisions cannot change, or the test will fail.

500swapnil · 2016-12-17T18:40:36Z

Still failing tests! Different error now. Need your help with this @jywarren

jywarren · 2016-12-18T19:18:23Z

test/functional/wiki_controller_test.rb

+    node(:organizers).add_tag('locked', rusers(:admin)) # lock the page with a tag
+    # then try updating it
+    assert_difference 'DrupalNodeRevision.count', 0 do
+      post :update,


Oh, note that your changes were to the :edit action, but your test is for the :update action. Both need to be changed and tested, but I think you're changing one but testing the other in this step.

Thank you! You are right! And the entire post block comes under assert_difference right?

jywarren · 2016-12-18T21:09:07Z

test/functional/wiki_controller_test.rb

+    assert_difference 'DrupalNodeRevision.count', 0 do
+      post :edit,
+          id:  node(:organizers).id,
+          uid:   rusers(:bob).id,


These two lines shouldn't be needed in the edit action.

Oh I meant the UID and the next one. The id is still required.

Yeah I removed uid and title. Error with the message now

Oh I meant the UID and the next one. The id is still required.

Also I'm not sure if I left a comment but the id needs to be the slug in this context, so you can use the new node.slug_from_path method, or just the string "organizers"!

Oh OK -- I'll try to take a look later today!

Yes you mentioned abt the ID. And Thanks in advance!

jywarren · 2016-12-18T21:10:14Z

test/functional/wiki_controller_test.rb

@@ -108,21 +108,21 @@ def teardown

  test "viewing edit wiki page" do

-    get :edit, 
+    get :edit,
         id: 'organizers'


Also look how the id for the edit action should be the page slug, not the numeric id! That should solve your last issue.

500swapnil · 2016-12-19T19:58:03Z

@ryzokuken Can you tell why I am getting this error and how to fix this?

ryzokuken · 2016-12-19T20:02:54Z

I'll check the Travis log.

ryzokuken · 2016-12-19T20:12:50Z

You failed the test you created yourself, as there were no elements in the page as .alert while the test requires one such element to be present. I suppose this is because you set flash[:warning] text but did not create the alert.

500swapnil · 2016-12-19T21:16:26Z

@ryzokuken Okay so I will remove the '.alert' test. But how do I test this?
flash.now[:warning] = "This page is locked, and only moderators can edit it."

jywarren · 2016-12-19T21:46:09Z

But wait, don't we want the test to identify the alert you've written? Why isn't it detecting the notice?

…

On Dec 19, 2016 4:16 PM, "Swapnil Gupta" ***@***.***> wrote: Okay so I will remove the '.alert' test. But how do I test this? flash.now[:warning] = "This page is locked <http:///wiki/power-tags#Locking>, and only moderators <http:///wiki/moderators> can edit it." — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1114 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABfJ8ZSQR-OxluX_r_jdG982bSfQmSFks5rJvQqgaJpZM4LPeow> .

500swapnil · 2016-12-20T03:44:20Z

I dont know why its showing an error! How do I detect a 'flash.now[:warning]'?

jywarren · 2016-12-20T14:23:12Z

app/controllers/wiki_controller.rb

@@ -72,6 +72,10 @@ def edit
    else
      @node = DrupalNode.find_wiki(params[:id])
    end
+    if @node.has_power_tag('locked') && (current_user.role != "admin" && current_user.role != "moderator")
+      flash.now[:warning] = "This page is <a href='/wiki/power-tags#Locking'>locked</a>, and only <a href='/wiki/moderators'>moderators</a> can edit it."


Ah, i think the problem is here -- a flash.now runs before the redirect, but a flash[] runs after. If you remove the "now" it should work and pass the test!

jywarren · 2016-12-20T14:24:03Z

test/functional/wiki_controller_test.rb

+          id: 'organizers'
+    end
+    assert_template "wiki/edit"
+    assert_select ".alert", "flash.now[:warning] = 'This page is <a href='/wiki/power-tags#Locking'>locked</a>, and only <a href='/wiki/moderators'>moderators</a> can edit it.'"


Also, remove flash.now[:warning] = from this -- the select will test the contents of the element, so it should match the message itself. Good!

500swapnil · 2016-12-20T20:14:59Z

The error is that there is no ".alert" message. We need to use a different method for detecting flash[:warning] any ideas? Can we use assert_equal?

jywarren · 2016-12-21T20:19:28Z

Hmm, odd. You can also test the flash like this:

https://github.com/publiclab/plots2/pull/1114/files#diff-afcc4c4b1665a46e6c88e913be20b76dR132

But please add it above the existing flash test so we test both ways. Thanks!

500swapnil · 2016-12-21T20:40:42Z

Okay so the statement would be like:

assert_equal flash[:warning] , "This page is <a href='/wiki/power-tags#Locking'>locked</a>, and only <a href='/wiki/moderators'>moderators</a> can edit it."

right?

jywarren · 2016-12-26T22:27:37Z

Yes, that's right! Sorry for the slow response, we're still in a holiday here but I'll be more responsive now. Thanks!

jywarren · 2016-12-27T19:36:39Z

Well, it's still failing. I think we have to figure out if all these conditions are actually passing in the test:

if @node.has_power_tag('locked') && (current_user.role != "admin" && current_user.role != "moderator")

Is there any way they wouldn't be? Could we echo out each one, like:

puts @node.has_power_tag('locked')
puts current_user.role != "admin"
puts current_user.role != "moderator"

in the controller code and see what we get?

jywarren · 2016-12-27T23:19:25Z

app/controllers/wiki_controller.rb

@@ -72,6 +72,10 @@ def edit
    else
      @node = DrupalNode.find_wiki(params[:id])
    end
+    if @node.has_power_tag('locked') && (current_user.role != "admin" && current_user.role != "moderator")


Ah! Here's the problem! has_power_tag looks for a tag with the given key, and any value, in the format key:value -- but the locked tag is not a key:value format tag. Here we should instead just use @node.has_tag('locked'). That should fix it!

jywarren · 2016-12-27T23:21:15Z

@500swapnil -- the change i just proposed should work. But after this PR, you really should begin using named feature branches -- see this guide for how to do this. That way your PRs won't come from your master branch, but from a branch named, for example, edit-locking.

500swapnil · 2016-12-28T13:41:45Z

I made the required change and yes, I will work using branches after this PR!

jywarren · 2016-12-28T14:10:46Z

Hmm, OK, can we assert template after a redirect? Maybe not -- I guess best remove that line.

jywarren · 2016-12-28T14:13:00Z

See, because in functional tests we can only access the most recent request. Tests with a sequence of multiple requests really belong in integration tests, which have their own abilities and features.

Thanks!!

jywarren · 2016-12-28T14:43:29Z

And perhaps similarly the asserr_select can't be done without loading the redirect.

jywarren · 2016-12-28T22:20:41Z

Hmm, it's only passing now because you removed all the assertions -- but we need to preserve some -- was the assert flash not working? Ah - i see the tests failed, but do check them by clicking the "X" link -- 961812c only failed due to a stalled test so I restarted it. You can tell this when it stalls at 1:30 instead of 7-8 minutes, and you can restart the tests manually by closing and re-opening the PR. It's a bit of a pain, but sometimes it gets stuck downloading remote dependencies.

Let's see if 961812c passes, and if not, why!

jywarren · 2016-12-28T22:58:34Z

OK -- got the test in 961812c to run -- https://travis-ci.org/publiclab/plots2/builds/187231729

Failure: expecting <"wiki/show"> but rendering with <"">
test_basic_user_blocked_from_editing_a_locked_wiki_page(WikiControllerTest)
test/functional/wiki_controller_test.rb:142:in `block in <class:WikiControllerTest>'

I believe it's catching on this line: https://github.com/publiclab/plots2/pull/1114/files#diff-afcc4c4b1665a46e6c88e913be20b76dR154

This makes sense -- and I think it means you can go back to the state you were in at 961812c -- because I believe this is showing that it's not rendering a template at that point -- it's redirecting since the basic user shouldn't be able to post an update -- the "organizers" page is locked. Does that help?

Thanks for your persistence on this -- the restarting-tests thing is annoying, but you're getting closer to completion here.

jywarren · 2016-12-29T19:56:38Z

test/functional/wiki_controller_test.rb

+          id: 'organizers'
+    end
+    assert_equal flash[:warning] , "This page is <a href='/wiki/power-tags#Locking'>locked</a>, and only <a href='/wiki/moderators'>moderators</a> can edit it."
+    assert_redirected_to node.path


Ah - should be the last thing here -- this should be node(:organizers).path since we haven't made a variable called node in this test. Fingers crossed that this is the last thing!

500swapnil · 2016-12-30T02:09:06Z

I made the required changes and yes it works now! Thank you. Can you explain in brief the syntax of this line? I am trying to understand the working of tests.
node(:organizers).add_tag('locked', rusers(:admin))

jywarren · 2016-12-31T18:12:43Z

Great! Tests stalled but I restarted them; we'll see.

Yes, node(:organizers).add_tag('locked', rusers(:admin)) is a bit obscure. Let's see:

node(:organizers) is accessing the test fixture data for nodes, in /test/fixtures/node.yml. It's fetching the "organizers" entry.

node.add_tag('locked', rusers(:admin)) is using a method from /app/models/drupal_node.rb called add_tag() which requires a tagname and a user.

rusers(:admin) is accessing the rusers.yml fixture, so getting the test "admin" user.

So that whole line is adding a tag to the test fixture data, but in such a way that it doesn't require a change to the fixture files themselves - it's just happening within the test itself. We might add a comment there to explain that line if you like, but it's not required.

OK -- waiting for those tests!

jywarren · 2016-12-31T18:22:54Z

Passed! Perfect. Merging! We actually also need similar tests and limits for the update action. They'd be virtually the same as the changes you've made here -- interested in giving it a try?

500swapnil · 2017-01-01T10:18:56Z

Yeah I will try fixing that too! Thanks!

Added test to block basic user edits

7ea8e07

500swapnil mentioned this pull request Dec 16, 2016

Wiki page locking #397

Closed

8 tasks

Swapnil Gupta and others added 3 commits December 17, 2016 10:16

Added feature

e5a2696

upate wiki_controller_test.rb

1cc7ae7

Update wiki_controller_test.rb

a19f298

jywarren reviewed Dec 18, 2016

View reviewed changes

Update wiki_controller_test.rb

05c27b9

jywarren requested changes Dec 18, 2016

View reviewed changes

500swapnil added 3 commits December 19, 2016 03:47

Update wiki_controller_test.rb

ff97cb5

Update wiki_controller_test.rb

f63007f

Update wiki_controller_test.rb

59539cd

jywarren reviewed Dec 20, 2016

View reviewed changes

Update on flash[]

af7da24

Added flash test

5200796

jywarren reviewed Dec 27, 2016

View reviewed changes

update wiki_controller.rb

475161b

update

5d3b82f

Swapnil Gupta added 3 commits December 28, 2016 21:10

Removed assert_select

961812c

removed assert flash

148a678

removed assert_redirected_to test

8035299

Update

f0345ca

Back to 961812c state

52d6eb2

jywarren reviewed Dec 29, 2016

View reviewed changes

Update wiki_controller_test.rb

858beec

jywarren approved these changes Dec 31, 2016

View reviewed changes

jywarren merged commit 940286e into publiclab:master Dec 31, 2016

Added test to block basic user edits, closes issue #397 #1114

Added test to block basic user edits, closes issue #397 #1114

Conversation

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

500swapnil commented Dec 16, 2016

jywarren commented Dec 16, 2016

500swapnil commented Dec 17, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

500swapnil Dec 18, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

500swapnil commented Dec 19, 2016

ryzokuken commented Dec 19, 2016

ryzokuken commented Dec 19, 2016

500swapnil commented Dec 19, 2016 • edited Loading

jywarren commented Dec 19, 2016 via email

500swapnil commented Dec 20, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

500swapnil commented Dec 20, 2016

jywarren commented Dec 21, 2016

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

jywarren commented Dec 26, 2016

jywarren commented Dec 27, 2016

Choose a reason for hiding this comment

jywarren commented Dec 27, 2016

500swapnil commented Dec 28, 2016

jywarren commented Dec 28, 2016

jywarren commented Dec 28, 2016

jywarren commented Dec 28, 2016

jywarren commented Dec 28, 2016

jywarren commented Dec 28, 2016

Choose a reason for hiding this comment

500swapnil commented Dec 30, 2016

jywarren commented Dec 31, 2016

jywarren commented Dec 31, 2016

500swapnil commented Jan 1, 2017

500swapnil commented Dec 16, 2016 •

edited by jywarren

Loading

500swapnil commented Dec 17, 2016 •

edited

Loading

500swapnil Dec 18, 2016 •

edited

Loading

500swapnil commented Dec 19, 2016 •

edited

Loading

500swapnil commented Dec 21, 2016 •

edited by jywarren

Loading