[1339] hide announcement #1350

alex-kogan · 2015-11-10T01:00:16Z

Resolves #1339 on master

orenyk · 2015-11-10T01:05:28Z

Here are some notes on testing cookies in controller specs. You'd basically want to use the Devise helpers for spoofing user login in the tests - to see how we've done it in the past you can look at spec/controllers/reservations_controller_spec.rb.

alex-kogan · 2015-12-01T02:50:44Z

@AlanLiu96 - Ready for your code review

alex-kogan · 2015-12-04T21:13:25Z

@orenyk - While doing he's code review for this issue Alan raised a valid question, should a banned user be able to hide the announcements as well?

AlanLiu96 · 2015-12-04T21:28:01Z

It looks good mostly! The only issues that came up were when checkout users and banned users were not able to hide the announcement. I also found that if you hid the announcements and quickly changed the page, the announcements were still there, but I think that's a separate issue (or not one we have to worry much about anyway). Once the checkout users and banned users are addressed, it should be set to merge in!

orenyk · 2015-12-06T03:32:08Z

Thanks @AlanLiu96! Regarding the refresh issue - did the "hidden" announcements stay visible permanently or only for a single refresh? Either way it's probably ok, but we should still understand what's going on.

orenyk · 2015-12-14T19:04:15Z

A second note - once we squash these commits we should cherry pick that squashed commit onto release-v5.5 to patch that version as well.

AlanLiu96 · 2015-12-20T02:54:44Z

I'm not sure if it's fixed now, but the hidden announcements stayed there permanently when I last checked. I think that it was probably just from changing the page too quickly before it fully completed the request made to hide the announcement.

orenyk · 2015-12-21T04:26:30Z

@alex-kogan please comment when @AlanLiu96's comments re: checkout persons and banned users have been addressed, thanks!

orenyk · 2015-12-22T03:24:08Z

@alex-kogan I just tested locally and confirmed that Checkout Persons and Banned users can't dismiss announcements. Let's get it fixed and make sure that the tests are actually working.

alex-kogan · 2015-12-23T00:45:16Z

@AlanLiu96 & @orenyk - Added the needed abilities please recheck.

orenyk · 2015-12-23T01:49:17Z

Looks good, although I'm confused as to why your tests didn't pick it up. I'll do a code review now and see if we can figure it out.

orenyk · 2015-12-23T01:50:36Z

app/models/ability.rb

@@ -31,6 +31,7 @@ def initialize(user) # rubocop:disable all
        can :override, :reservation_errors if AppConfig.get(:override_on_create)
        can :override, :checkout_errors if AppConfig.get(:override_at_checkout)
        can :view_detailed, EquipmentModel
+        can :hide, Announcement


This line is unnecessary given line 47 below.

I noticed the redundancy but for some reason without that line checkout-person can't hide the announcement

Huh, that's odd. I'll mess around and see if I can figure out why.

For the record, this is a separate issue and we'll deal with it in #1391

alex-kogan · 2015-12-23T01:55:51Z

@orenyk - my hypothesis about the tests:

it only check the js
it uses the ability to decide for what type of user to test for the hide ability

orenyk · 2015-12-23T02:03:46Z

spec/controllers/announcements_controller_spec.rb

@@ -129,5 +149,8 @@
      end
      it_behaves_like 'access denied'
    end
+    context 'GET hide' do


Note that this test only checks for patrons (see line 123 above), not all user roles. That's why it didn't catch the others. We should probably write tests for all roles... as this is a common problem I'm going to open a new issue to write an RSpec helper to deal with it.

Now I remember that when we discussed it last time that was the decision made :)
I'll wait for the issue to open

Gotcha. For this issue let's include separate tests for all roles and we'll refactor in #1390. I'm still looking into the ability.rb thing, it might actually be fundamentally broken 😛.

orenyk · 2015-12-23T02:41:19Z

Ok, ability.rb is kinda broken but at the moment it isn't a big deal (see #1391 for more details). Let's leave this working and we'll refactor there, although let's add the appropriate specs to cover all user roles. Make sense?

orenyk · 2015-12-23T02:49:07Z

spec/controllers/announcements_controller_spec.rb

+      verifier = ActiveSupport::MessageVerifier.new(
+        Reservations::Application.config.secret_key_base)
+      name = 'hidden_announcement_ids'
+      @request.cookies[name] = verifier.generate([@announcement[:id].to_s])


Why are we using @request instead of just request? Changing it doesn't appear to break the spec.

Ok, the major issue is here. You're basically setting the relevant cookie in the request and therefore it's still set in the response, regardless of the controller behavior. If we look at this SO answer, we see how we would test for a signed cookie, effectively deleting line 29 (this line) and adding the following after line 30:

jar.signed[name] = [@announcement[:id].to_s]

This gives us an equivalent cookie to test against, and then we check to make sure the response has the right cookie value. I made sure it worked by commenting out the relevant line in ability.rb and the test failed as expected.

Let's get this text fixed and duplicate it to check all user roles. Let me know if you have any questions!

orenyk · 2015-12-23T03:02:33Z

Ok, more problematically, the spec doesn't actually appear to test anything (in the sense that when I remove the "fix" in ability.rb the test still passes. This is one of the primary motivators for the Red-Green-Refactor / TDD system - if we don't have an initially failing test, we don't actually know that our code changes make it pass. Given the complexity of this test I'm going to take a stab at making it work, if that's all right with you.

orenyk · 2015-12-23T03:08:08Z

Ok, figured it out - I'll comment inline.

orenyk · 2015-12-23T03:12:36Z

spec/controllers/announcements_controller_spec.rb

+        Reservations::Application.config.secret_key_base)
+      name = 'hidden_announcement_ids'
+      @request.cookies[name] = verifier.generate([@announcement[:id].to_s])
+      jar = @request.cookie_jar


Also, let's remove the @ here (so it would be jar = request.cookie_jar) for consistency.

alex-kogan · 2015-12-26T12:18:57Z

@orenyk - Fixed the actual spec to test the hiding of the announcements, also added specs for each type of user. Ready for re-review.

orenyk · 2015-12-27T00:05:12Z

spec/controllers/announcements_controller_spec.rb

@@ -130,4 +145,43 @@
      it_behaves_like 'access denied'
    end
  end
+  context 'check hide' do
+    before { @announcement = FactoryGirl.create(:announcement) }


This isn't necessary given line 20 above, @announcement already exists.

orenyk · 2015-12-27T00:53:00Z

Ok, this is now working as expected, well done 😄. Just a few inline comments, mostly regarding the naming of test blocks and the organization of the spec file, and I think this will be good to squash. Feel free to squash after making the changes and we'll give this a final quick review before cherry-picking for v5.5.1 and merging both PR's. Thanks!

alex-kogan · 2015-12-27T10:58:22Z

Fixed all the comments, I don't really know how to squash...
If you can give me some instructions that would be appreciated.

orenyk · 2015-12-27T17:54:40Z

Great, looks good. Here's our guide to squashing commits from the contributing guide - you can replace step 1 by just looking at the pull request (i.e. it says "9 Commits" at the top), but everything else should work fine. Give it a shot; as long as you don't push things to GitHub you can always delete your local branch and try again if you mess something up. Let me know if you run into any specific issues!

alex-kogan · 2015-12-30T09:34:21Z

I squashed the commits, now what?

orenyk · 2015-12-30T18:08:19Z

Now you'd force-push to this branch:

git push -f origin 1339_hide_announcement

If you're worried that you might have messed something up you can always create a new branch at the new commit and push that instead, just to verify that the diff is identical (although it should be) without overwriting any history. Does that make sense?

alex-kogan · 2016-01-01T14:32:07Z

Done!

orenyk · 2016-01-01T21:27:15Z

Looks good. The last thing is you need to update the commit message to clean up all the duplication from the various commit messages (everything after - Add shared examples for specs) and fix the typo in the third line (Cahnge --> Change). You can do that using git commit --amend from within the branch to edit the commit message and force-push the branch again. Once we've merged this in to master we'll cherry-pick the final commit onto release-v5.5 as well. Nice work!

orenyk · 2016-01-05T07:01:08Z

@alex-kogan I don't know if you missed this but I'd like to get this onto release-v5.5 ASAP so we can tag/release/deploy a new release candidate. Let me know when you've had a chance to update this, thanks!

alex-kogan · 2016-01-05T07:06:17Z

Haven't missed it will do it asap. Thanks!

‫ב-5 בינו׳ 2016, בשעה 9:01, ‏‏Oren Kanner ‏[email protected] כתב/ה:‬

@alex-kogan I don't know if you missed this but I'd like to get this onto release-v5.5 ASAP so we can tag/release/deploy a new release candidate. Let me know when you've had a chance to update this, thanks!

—
Reply to this email directly or view it on GitHub.

orenyk · 2016-01-05T07:15:28Z

Thank you!

Resolves #1339 - Change the ability file - Add a cookie testing spec - Add shared examples for specs

alex-kogan · 2016-01-05T08:59:33Z

Done - ready for realse

orenyk · 2016-01-05T18:14:25Z

Perfect, merging. I'll cherry-pick onto release-v5.5 and update the CHANGELOG's.

[1339] hide announcement

AlanLiu96 added the 1 - working label Dec 4, 2015

orenyk reviewed Dec 23, 2015
View reviewed changes

orenyk mentioned this pull request Dec 23, 2015

ability.rb is fundamentally (kinda) broken #1391

Closed

orenyk reviewed Dec 23, 2015
View reviewed changes

orenyk reviewed Dec 27, 2015
View reviewed changes

alex-kogan closed this Dec 27, 2015

alex-kogan reopened this Dec 27, 2015

alex-kogan force-pushed the 1339_hide_announcement branch from d4f5021 to 081e261 Compare January 1, 2016 14:21

Adding the ability to hide the announcements for all users

3742415

Resolves #1339 - Change the ability file - Add a cookie testing spec - Add shared examples for specs

alex-kogan force-pushed the 1339_hide_announcement branch from 081e261 to 3742415 Compare January 5, 2016 08:58

orenyk added a commit that referenced this pull request Jan 5, 2016

Merge pull request #1350 from YaleSTC/1339_hide_announcement

02ec988

[1339] hide announcement

orenyk merged commit 02ec988 into master Jan 5, 2016

orenyk removed the 1 - working label Jan 5, 2016

orenyk mentioned this pull request Jan 5, 2016

[1339] Adding the ability to hide the announcements for all users [v5.5] #1408

Merged

[1339] hide announcement #1350

[1339] hide announcement #1350

Conversation

alex-kogan commented Nov 10, 2015

orenyk commented Nov 10, 2015

alex-kogan commented Dec 1, 2015

alex-kogan commented Dec 4, 2015

AlanLiu96 commented Dec 4, 2015

orenyk commented Dec 6, 2015

orenyk commented Dec 14, 2015

AlanLiu96 commented Dec 20, 2015

orenyk commented Dec 21, 2015

orenyk commented Dec 22, 2015

alex-kogan commented Dec 23, 2015

orenyk commented Dec 23, 2015

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

alex-kogan commented Dec 23, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

orenyk commented Dec 23, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

orenyk commented Dec 23, 2015

orenyk commented Dec 23, 2015

Choose a reason for hiding this comment

alex-kogan commented Dec 26, 2015

Choose a reason for hiding this comment

orenyk commented Dec 27, 2015

alex-kogan commented Dec 27, 2015

orenyk commented Dec 27, 2015

alex-kogan commented Dec 30, 2015

orenyk commented Dec 30, 2015

alex-kogan commented Jan 1, 2016

orenyk commented Jan 1, 2016

orenyk commented Jan 5, 2016

alex-kogan commented Jan 5, 2016

orenyk commented Jan 5, 2016 via email

alex-kogan commented Jan 5, 2016

orenyk commented Jan 5, 2016