Add specs for NotificationsController [#311] #319

tcdowney · 2016-10-09T21:58:58Z

Issue: #311

This includes some minor refactoring of the NotificationsController.

tcdowney · 2016-10-09T22:00:44Z

app/controllers/notifications_controller.rb

-    # Use callbacks to share common setup or constraints between actions.
-    def set_notification
-      begin
-        @notification = Notification.find(params[:id])


This used to let users destroy notifications belonging to other users provided that they knew (or guessed) a valid Notification ID. I got rid of this before_action since it was only used in one place and was duplicating logic and used a more specific query in in #destroy.

tcdowney · 2016-10-09T22:02:06Z

app/controllers/notifications_controller.rb

@@ -26,33 +31,14 @@ def fetch_notifications
  end

  def signed_in
-    if !user_signed_in?
-      signed_in = -1


This code wasn't reachable due to the if_not_signed_in before action. If the intent was for this endpoint to actually return -1 to un-authenticated users, I can restore this code and prevent the before action from acting on this action.

So this code is being accessed in notifications.js:197. This was created to figure out whether the user is signed in from notifications.js.

Your refactor should be fine 👍

tcdowney · 2016-10-09T22:08:01Z

app/controllers/notifications_controller.rb

    respond_to do |format|
-        format.html { redirect_to :back }
+      format.html { redirect_to :back }
      format.json { head :no_content }
    end
  end

  def clear


clear_notifications GET /notifications/clear(.:format) notifications#clear fetch_notifications_notifications GET /notifications/fetch_notifications(.:format) notifications#fetch_notifications signed_in_notifications GET /notifications/signed_in(.:format) notifications#signed_in notifications GET /notifications(.:format) notifications#index notification DELETE /notifications/:id(.:format) notifications#destroy

I don't think the route for clear should be using HTTP GET since destroying all notifications for a user is a very stateful request. It's also susceptible to CSRF attacks since a malicious user could give someone a relatively benign looking link like http://if-me.org/notifications/clear and the act of clicking on it (provided they were logged in) would delete all of their notifications whether they liked it or not.

http://programmers.stackexchange.com/questions/188860/why-shouldnt-a-get-request-change-data-on-the-server

Mind if I create an issue for switching this over?

Edit: logged #320

Thanks for pointing this out! Definitely fix this 💯

julianguyen · 2016-10-10T01:28:41Z

Looks like @esBeee also made a PR related to the notifications controller (#316). Can you two sync on this? Also @esBeee, please email us at join.ifme@gmail.com to be added to our Slack.

julianguyen · 2016-10-10T02:00:39Z

app/controllers/notifications_controller.rb

@@ -1,20 +1,25 @@
 class NotificationsController < ApplicationController
  before_filter :if_not_signed_in
-  before_action :set_notification, only: [:destroy]


I like what @esBeee did to secure notifications https://github.com/julianguyen/ifme/pull/316/files#diff-e7d2878d7ccecd38f6e3a83b5cf85053R73

Cool, glad someone else noticed it earlier! I'll sync up and work my changes around theirs. Thanks for the heads up.

julianguyen · 2016-10-10T02:20:52Z

spec/controllers/notifications_controller_spec.rb

+  describe '#fetch_notifications' do
+    let(:user) { FactoryGirl.create(:user1) }
+    let(:other_user) { FactoryGirl.create(:user2) }
+    let!(:other_user_notification) do


Is adding ! necessary?

It's necessary for this one because I wanted to make sure that we were testing that fetch_notifications only returned the current user's notifications. Since this let is never explicitly invoked anywhere in the test, this second notification won't be created without the !.

julianguyen · 2016-10-24T03:05:05Z

What's the status on this @tcdowney?

julianguyen · 2016-10-30T23:48:24Z

@tcdowney Can you rebase and let me know when it's ready to merge? Thanks :)

- includes some minor refactoring of the notifications controller

tcdowney · 2016-11-13T07:36:29Z

@julianguyen

I rebased this branch with what's on master and all tests are passing locally. Tests are failing on CI, though, but the reason why doesn't seem related to the code that I've changed here -- have you see those failures before? 😢

julianguyen · 2016-11-13T07:48:20Z

Not sure, I'm going to rebuild your PR on CI.

julianguyen · 2016-11-13T07:51:14Z

@tcdowney

Devise.secret_key was not set. Please add the following to your Devise initializer:

EDIT: Hmm I'm wondering if anything needs to be set up with CircleCI from your end...

julianguyen · 2016-11-13T08:02:03Z

@tcdowney Hmm maybe you need to add yourself to the if me CircleCI org?
Thoughts @brunoocasali?

brunoocasali · 2016-11-14T02:00:12Z

@julianguyen, @tcdowney I don't know what this happens andela-oesho for example, have made a "passed" build... Having an account or not, should not matters to build pass or not, I will rebuild to see.

julianguyen · 2016-11-15T22:05:46Z

@brunoocasali Are things working now?

brunoocasali · 2016-11-16T00:14:49Z

I can't understand what happened on this PR, but it already merged, we need to merge the #339 to get the new features of continuous integration (by the way there are the changes about C.I.) I suppose when he goes to be merged we have no more problems with it :D

tcdowney commented Oct 9, 2016

View reviewed changes

tcdowney added hacktoberfest and removed hacktoberfest labels Oct 9, 2016

tcdowney mentioned this pull request Oct 9, 2016

Use HTTP DELETE instead of GET for destructive NotificationsController#clear action #320

Closed

julianguyen reviewed Oct 10, 2016

View reviewed changes

tcdowney mentioned this pull request Oct 11, 2016

Use DELETE instead of GET for Notifications#clear [#320] #327

Merged

the-bass mentioned this pull request Oct 11, 2016

Secure notifications controller #316

Closed

tcdowney force-pushed the pr/311-notifications_controller_spec branch from 94c0ccb to 3538e8e Compare November 13, 2016 07:20

Add specs for NotificationsController [ifmeorg#311]

7e2057a

- includes some minor refactoring of the notifications controller

tcdowney force-pushed the pr/311-notifications_controller_spec branch from 3538e8e to 7e2057a Compare November 13, 2016 07:31

julianguyen merged commit 89b01d3 into ifmeorg:master Nov 13, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add specs for NotificationsController [#311] #319

Add specs for NotificationsController [#311] #319

tcdowney commented Oct 9, 2016

tcdowney Oct 9, 2016

tcdowney Oct 9, 2016

julianguyen Oct 10, 2016

julianguyen Oct 10, 2016

tcdowney Oct 9, 2016 •

edited

Loading

julianguyen Oct 10, 2016

julianguyen commented Oct 10, 2016

julianguyen Oct 10, 2016

tcdowney Oct 10, 2016

julianguyen Oct 10, 2016

tcdowney Oct 10, 2016

julianguyen commented Oct 24, 2016

julianguyen commented Oct 30, 2016

tcdowney commented Nov 13, 2016

julianguyen commented Nov 13, 2016

julianguyen commented Nov 13, 2016 •

edited

Loading

julianguyen commented Nov 13, 2016 •

edited

Loading

brunoocasali commented Nov 14, 2016

julianguyen commented Nov 15, 2016

brunoocasali commented Nov 16, 2016

Add specs for NotificationsController [#311] #319

Add specs for NotificationsController [#311] #319

Conversation

tcdowney commented Oct 9, 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

tcdowney Oct 9, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julianguyen commented Oct 10, 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

julianguyen commented Oct 24, 2016

julianguyen commented Oct 30, 2016

tcdowney commented Nov 13, 2016

julianguyen commented Nov 13, 2016

julianguyen commented Nov 13, 2016 • edited Loading

julianguyen commented Nov 13, 2016 • edited Loading

brunoocasali commented Nov 14, 2016

julianguyen commented Nov 15, 2016

brunoocasali commented Nov 16, 2016

tcdowney Oct 9, 2016 •

edited

Loading

julianguyen commented Nov 13, 2016 •

edited

Loading

julianguyen commented Nov 13, 2016 •

edited

Loading