fix account notifications API documentation

only works for current user - user_id field is ignored

closes #ADMIN-711

Change-Id: I6f783a19bafbcd830fbab918782b1aa3591735d9
Reviewed-on: https://gerrit.instructure.com/139875
Tested-by: Jenkins
Reviewed-by: Rob Orton <rob@instructure.com>
Product-Review: James Williams  <jamesw@instructure.com>
QA-Review: James Williams  <jamesw@instructure.com>

app/controllers/account_notifications_controller.rb

@@ -75,15 +75,16 @@
 class AccountNotificationsController < ApplicationController
   include Api::V1::AccountNotifications
   before_action :require_user
-  before_action :require_account_admin, except: [:user_index, :user_close_notification, :show]
+  before_action :require_account_admin, only: [:create, :update, :destroy]
+  before_action :check_user_param, only: [:user_index_deprecated, :user_close_notification_deprecated, :show_deprecated]
 
   # @API Index of active global notification for the user
-  # Returns a list of all global notifications in the account for this user
+  # Returns a list of all global notifications in the account for the current user
   # Any notifications that have been closed by the user will not be returned
   #
   # @example_request
   #   curl -H 'Authorization: Bearer <token>' \
-  #   https://<canvas>/api/v1/accounts/2/users/4/account_notifications
+  #   https://<canvas>/api/v1/accounts/2/users/self/account_notifications
   #
   # @returns [AccountNotification]
   def user_index
@@ -91,13 +92,17 @@ class AccountNotificationsController < ApplicationController
     render :json => account_notifications_json(notifications, @current_user, session)
   end
 
+  def user_index_deprecated
+    user_index
+  end
+
   # @API Show a global notification
-  # Returns a global notification
+  # Returns a global notification for the current user
   # A notification that has been closed by the user will not be returned
   #
   # @example_request
   #   curl -H 'Authorization: Bearer <token>' \
-  #   https://<canvas>/api/v1/accounts/2/users/4/account_notifications/4
+  #   https://<canvas>/api/v1/accounts/2/users/self/account_notifications/4
   #
   # @returns AccountNotification
   def show
@@ -110,12 +115,16 @@ class AccountNotificationsController < ApplicationController
     end
   end
 
+  def show_deprecated
+    show
+  end
+
   # @API Close notification for user
-  # If the user no long wants to see this notification it can be excused with this call
+  # If the current user no long wants to see this notification it can be excused with this call
   #
   # @example_request
   #   curl -X DELETE -H 'Authorization: Bearer <token>' \
-  #   https://<canvas>/api/v1/accounts/2/users/4/account_notifications/4
+  #   https://<canvas>/api/v1/accounts/2/users/self/account_notifications/4
   #
   # @returns AccountNotification
   def user_close_notification
@@ -124,6 +133,10 @@ class AccountNotificationsController < ApplicationController
     render :json => account_notification_json(notification, @current_user, session)
   end
 
+  def user_close_notification_deprecated
+    user_close_notification
+  end
+
   # @API Create a global notification
   # Create and return a new global notification for an account.
   #
@@ -287,6 +300,10 @@ class AccountNotificationsController < ApplicationController
   end
 
   protected
+  def check_user_param
+    raise ActiveRecord::RecordNotFound unless api_find(User, params[:user_id]) == @current_user
+  end
+
   def require_account_admin
     require_account_context
     return false unless authorized_action(@account, @current_user, :manage_alerts)

config/routes.rb

@@ -950,9 +950,12 @@ CanvasRails::Application.routes.draw do
     scope(controller: :account_notifications) do
       post 'accounts/:account_id/account_notifications', action: :create, as: 'account_notification'
       put 'accounts/:account_id/account_notifications/:id', action: :update, as: 'account_notification_update'
-      get 'accounts/:account_id/users/:user_id/account_notifications', action: :user_index, as: 'user_account_notifications'
-      get 'accounts/:account_id/users/:user_id/account_notifications/:id', action: :show, as: 'user_account_notification_show'
-      delete 'accounts/:account_id/users/:user_id/account_notifications/:id', action: :user_close_notification, as: 'user_account_notification'
+      get 'accounts/:account_id/account_notifications', action: :user_index, as: 'user_account_notifications' # to change the api docs
+      get 'accounts/:account_id/users/:user_id/account_notifications', action: :user_index_deprecated # for back compat
+      get 'accounts/:account_id/account_notifications/:id', action: :show, as: 'user_account_notification_show'
+      get 'accounts/:account_id/users/:user_id/account_notifications/:id', action: :show_deprecated
+      delete 'accounts/:account_id/account_notifications/:id', action: :user_close_notification, as: 'user_account_notification'
+      delete 'accounts/:account_id/users/:user_id/account_notifications/:id', action: :user_close_notification_deprecated
     end
 
     scope(controller: :brand_configs_api) do

spec/apis/v1/account_notifications_api_spec.rb

@@ -29,11 +29,10 @@ describe 'Account Notification API', type: :request do
   describe 'user_index' do
     before do
       account_notification(message: 'default')
-      @path = "/api/v1/accounts/#{@admin.account.id}/users/#{@admin.id}/account_notifications"
+      @path = "/api/v1/accounts/#{@admin.account.id}/account_notifications"
       @api_params = {controller: 'account_notifications',
                      action: 'user_index',
                      format: 'json',
-                     user_id: @admin.id,
                      account_id: @admin.account.id.to_s}
     end
 
@@ -41,17 +40,37 @@ describe 'Account Notification API', type: :request do
       account_notification(message: 'second')
       json = api_call(:get, @path, @api_params,)
       expect(json.length).to eq 2
+      expect(json.map{|r| r["message"]}).to match_array(%w{default second})
+    end
+
+    it "should still work on the old endpoint" do
+      json = api_call(:get, "/api/v1/accounts/#{@admin.account.id}/users/#{@admin.id}/account_notifications", {
+        controller: 'account_notifications',
+        action: 'user_index_deprecated',
+        format: 'json',
+        user_id: @admin.id.to_s,
+        account_id: @admin.account.id.to_s})
+      expect(json.map{|r| r["message"]}).to eq %w{default}
+    end
+
+    it "should catch a user_id mismatch on the old endpoint" do
+      other_user = User.create!
+      api_call(:get, "/api/v1/accounts/#{@admin.account.id}/users/#{other_user.id}/account_notifications", {
+        controller: 'account_notifications',
+        action: 'user_index_deprecated',
+        format: 'json',
+        user_id: other_user.id.to_s,
+        account_id: @admin.account.id.to_s}, {}, {:expected_status => 404})
     end
   end
 
   describe 'show' do
     before do
       @an = account_notification(message: 'default')
-      @path = "/api/v1/accounts/#{@admin.account.id}/users/#{@admin.id}/account_notifications/#{@an.id}"
+      @path = "/api/v1/accounts/#{@admin.account.id}/account_notifications/#{@an.id}"
       @api_params = {controller: 'account_notifications',
                      action: 'show',
                      format: 'json',
-                     user_id: @admin.id,
                      id: @an.id,
                      account_id: @admin.account.id.to_s}
     end
@@ -64,12 +83,11 @@ describe 'Account Notification API', type: :request do
     it "should show the notification as a non admin" do
       user = user_with_managed_pseudonym(:account => @admin.account)
 
-      @path = "/api/v1/accounts/#{user.account.id}/users/#{user.id}/account_notifications/#{@an.id}"
+      @path = "/api/v1/accounts/#{user.account.id}/account_notifications/#{@an.id}"
 
       @api_params = {controller: 'account_notifications',
                      action: 'show',
                      format: 'json',
-                     user_id: user.id,
                      id: @an.id,
                      account_id: @user.account.id.to_s}
 
@@ -81,12 +99,11 @@ describe 'Account Notification API', type: :request do
   describe 'user_close_notification' do
     before do
       @a = account_notification(message: 'default')
-      @path = "/api/v1/accounts/#{@admin.account.id}/users/#{@admin.id}/account_notifications/#{@a.id}"
+      @path = "/api/v1/accounts/#{@admin.account.id}/account_notifications/#{@a.id}"
       @api_params = {controller: 'account_notifications',
                      action: 'user_close_notification',
                      format: 'json',
                      id: @a.id.to_param,
-                     user_id: @admin.id,
                      account_id: @admin.account.id.to_s}
     end
 
@@ -95,7 +112,7 @@ describe 'Account Notification API', type: :request do
       @admin.reload
       expect(@admin.preferences[:closed_notifications]).to eq [@a.id]
 
-      json = api_call(:get, "/api/v1/accounts/#{@admin.account.id}/users/#{@admin.id}/account_notifications", @api_params.merge(action: 'user_index'),)
+      json = api_call(:get, "/api/v1/accounts/#{@admin.account.id}/account_notifications", @api_params.merge(action: 'user_index'),)
       expect(json.length).to eq 0
     end
   end