[openstreetmap/openstreetmap-website] Revoking administrator role on current user should fail (#1701)

[Tom Hughes]

tomhughes requested changes on this pull request.



> @@ -8,6 +8,7 @@ class UserRolesController < ApplicationController
   before_action :require_valid_role
   before_action :not_in_role, :only => [:grant]
   before_action :in_role, :only => [:revoke]
+  before_action :not_revoke_admin_current_user

This should only be applied to the `revoke` method but in fact this is so specific that I don't think there's any point making it a filter - the point of filters is that they can be shared across multiple methods but these even has the method name in it's name so clearly it only ever makes sense to apply it to revoke.

As such I would just put the logic in the `revoke` method and get rid of the filter.

> @@ -134,5 +134,9 @@ def test_revoke
     end
     assert_redirected_to user_path(target_user.display_name)
     assert_equal "The string `no_such_role' is not a valid role.", flash[:error]
+
+    # Revoking administrator role from current user should fail
+    post :revoke, :params => { :display_name => administrator_user.display_name, :role => "administrator" }
+    assert_redirected_to user_path(administrator_user.display_name)

Can we assert that the flash message is shown here... I think there some examples of doing it in other tests.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/openstreetmap/openstreetmap-website/pull/1701#pullrequestreview-84269604
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstreetmap.org/pipermail/rails-dev/attachments/20171218/fa01b57b/attachment.html>