Fixes #6065 - Update TopbarSweeper to clear cache for other users than User.current #1495
Conversation
|
@GregSutcliffe you'd need to stub |
|
Ticking/unticking the admin flag works well, however I realized removing an user from an user group does not sweep the top bar as expected. |
|
@GregSutcliffe ping? |
|
@elobato I havent forgotten, been busy :) |
|
I agree with @ares about breaking this down, it'd be nice to keep the implementation a bit closer to (or inside) the classes it interacts with, especially if we can move the common code to a reusable base. Plugins might want to extend the functionality and this will make it possible. (We have a similar issue with our audit helper.) If you can look at sorting this PR next week, I'd much appreciate it as I'd like it in 1.5.2 for the following week. |
|
👍 to the functionality, fixes what the gif shows and works well. @domcleal is this close enough to the classes it interacts with? It looks good to me, just misses some tests, I expect that will be tricky to write unfortunately. |
|
@elobato @domcleal I was looking at tests on Friday. Sadly we set caching to false in I could try to do something with expects and check that the method is called at least once in the appropriate place - do you think it's worth it? |
|
I think that's good enough testing |
| @@ -373,6 +373,10 @@ def self.random_password(size = 16) | |||
| size.times.collect {|i| set[rand(set.size)] }.join | |||
| end | |||
|
|
|||
| def expire_topbar_cache | |||
| ActionController::Base.new.expire_fragment(TopbarSweeper.fragment_name(id)) | |||
There was a problem hiding this comment.
Should topbar sweeper simply be passed through these expire_topbar_cache methods so you can call expire_fragment on it, rather than instantiating a new controller here?
There was a problem hiding this comment.
We can do that if you think it's better - I don't have a preference, but as I understand it, I think it all ends up calling the same method anyway.
There was a problem hiding this comment.
Right, I think it's cleaner as we already have the sweeper object.
|
I think expecting the right calls to expire_fragment when you modify these objects is the right way to test it, there's no need to test the caching itself. Generally with the placement of expire_topbar_cache in the respective classes, it looks a lot better. |
|
@domcleal updated - passing "sweeper" around seems to work ok. I've not tested the Org/Loc sweeper explicitly, but it should be ok. Tests will prove it, hopefully shortly :) |
|
Ok, tests done, although I couldn;t think of a way to test the UserRole observer since it has no controller of it's own. Given the observers all pass through the same way, it should be fine. |
|
If you updated a user's roles through the UsersController, it would modify UserRole objects, right? |
|
@domcleal good point - my confusion was forgetting I could just expect on UserRole instead of User for that test. Updated. |
|
[test] |
| @@ -195,4 +195,14 @@ class LocationsControllerTest < ActionController::TestCase | |||
| assert_equal location.id, assigns(:taxonomy).parent_id | |||
| end | |||
|
|
|||
| test "changes should expire topbar cache" do | |||
| User.current = users :admin | |||
There was a problem hiding this comment.
Please use as_admin around the minimum block required, ditto in the other tests
|
Reviews fine for me, thanks for the update. |
| test "changes should expire topbar cache" do | ||
| user1 = FactoryGirl.create(:user, :with_mail) | ||
| user2 = FactoryGirl.create(:user, :with_mail) | ||
| usergroup = FactoryGirl.create(:usergroup, :users = [user1, user2]) |
There was a problem hiding this comment.
Here :users = is causing the test suite to fail, it should be `:users =>``
There was a problem hiding this comment.
Whoops, thanks @elobato. Updated.
… users than User.current
|
Successfully tested this, merged as 70acceb for 1.5.2, thanks @GregSutcliffe ! |
I'm unclear how to test this, does anyone have any ideas? I don't see any existing tests for TopbarSweeper...