bounced email search admin tool

test plan: - simulate bounced emails in the Rails console. do this for a handful of users, putting different details in each: cc = User.find(x).communication_channel cc.bounce_count = 1 cc.last_bounce_at = 10.days.ago cc.last_bounce_details = {'bouncedRecipients' => [{'diagnosticCode' => '550 some error'}]} - ensure the "bounced emails" admin tool appears if the feature ("Bounced Emails Admin Tool") is enabled and the user has the requisite permissions, which include both :read_roster ("Users - view list") and either :view_notifications on the root account level ("Notifications - view"), which requires the "Admins can view notifications" account setting to be enabled, or :read_messages ("View notifications sent to users") on the site admin level - click the button without entering a search term and the most recent addresses appear (let's be real: they all appear, because you don't have 1000 of them) - enter a search term and click the button, and matching addresses appear - the date filtering should work flag = bounced_emails_admin_tool closes LS-1587 Change-Id: I38174d5422f6c4417d7c5e6856256061a8bfc2be Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/251572 Tested-by: Service Cloud Jenkins <svc.cloudjenkins@instructure.com> Reviewed-by: Ed Schiebel <eschiebel@instructure.com> QA-Review: Robin Kuss <rkuss@instructure.com> Product-Review: Peyton Craighill <pcraighill@instructure.com>
2020-10-30 14:44:52 -06:00 · 2020-10-30 14:44:52 -06:00 · 42deabaed7
parent 07db7ec34a
commit 42deabaed7
13 changed files with 353 additions and 37 deletions
--- a/app/coffeescripts/views/accounts/admin_tools/AdminToolsView.js
+++ b/app/coffeescripts/views/accounts/admin_tools/AdminToolsView.js
@ -34,6 +34,7 @@ export default class AdminToolsView extends Backbone.View {
    this.child('restoreContentPaneView', '#restoreContentPane')
    this.child('messageContentPaneView', '#commMessagesPane')
    this.child('loggingContentPaneView', '#loggingPane')
+    this.child('bouncedEmailsPaneView', '#bouncedEmailsPane')
    this.optionProperty('tabs')

    this.prototype.template = template
@ -52,6 +53,7 @@ export default class AdminToolsView extends Backbone.View {
    json.courseRestore = this.tabs.courseRestore
    json.viewMessages = this.tabs.viewMessages
    json.logging = this.tabs.logging
+    json.bouncedEmails = this.tabs.bouncedEmails
    return json
  }
 }
--- a/app/controllers/accounts_controller.rb
+++ b/app/controllers/accounts_controller.rb
@ -1131,7 +1131,10 @@ class AccountsController < ApplicationController
                      Account.site_admin.grants_right?(@current_user, :read_messages),
       logging: logging
      }
-    js_env enhanced_grade_change_query: Auditors::read_from_postgres? && Account.site_admin.feature_enabled?(:enhanced_grade_change_query)
+    js_env enhanced_grade_change_query: Auditors::read_from_postgres? &&
+      Account.site_admin.feature_enabled?(:enhanced_grade_change_query)
+    js_env bounced_emails_admin_tool: @account.feature_enabled?(:bounced_emails_admin_tool) &&
+      @account.grants_right?(@current_user, session, :view_bounced_emails)
  end

  def confirm_delete_user
--- a/app/controllers/communication_channels_controller.rb
+++ b/app/controllers/communication_channels_controller.rb
@ -580,16 +580,25 @@ class CommunicationChannelsController < ApplicationController
  end

  protected
+
+  def account
+    @account ||= params[:account_id] == 'self' ? @domain_root_account : Account.find(params[:account_id])
+  end
+
  def bulk_action_args
-    account = params[:account_id] == 'self' ? @domain_root_account : Account.find(params[:account_id])
-    args = params.permit(:after, :before, :pattern, :with_invalid_paths, :path_type).to_unsafe_h.symbolize_keys
+    args = params.permit(:after, :before, :pattern, :with_invalid_paths, :path_type, :order).to_unsafe_h.symbolize_keys
    args.merge!({account: account})
  end

  def generate_bulk_report
-    if authorized_action(Account.site_admin, @current_user, :read_messages)
+    if account.grants_right?(@current_user, session, :view_bounced_emails)
      action = yield
-      send_data(action.report, type: 'text/csv')
+      respond_to do |format|
+        format.csv { send_data(action.csv_report, type: 'text/csv') }
+        format.json { send_data(action.json_report, type: 'application/json') }
+      end
+    else
+      render_unauthorized_action
    end
  end

--- a/app/jsx/bounced_emails/BouncedEmailsView.js
+++ b/app/jsx/bounced_emails/BouncedEmailsView.js
@ -0,0 +1,189 @@
+/*
+ * Copyright (C) 2020 - present Instructure, Inc.
+ *
+ * This file is part of Canvas.
+ *
+ * Canvas is free software: you can redistribute it and/or modify it under
+ * the terms of the GNU Affero General Public License as published by the Free
+ * Software Foundation, version 3 of the License.
+ *
+ * Canvas is distributed in the hope that it will be useful, but WITHOUT ANY
+ * WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR
+ * A PARTICULAR PURPOSE. See the GNU Affero General Public License for more
+ * details.
+ *
+ * You should have received a copy of the GNU Affero General Public License along
+ * with this program. If not, see <http://www.gnu.org/licenses/>.
+ */
+
+import React, {useState, useCallback} from 'react'
+import {Table} from '@instructure/ui-table'
+import {Spinner} from '@instructure/ui-spinner'
+import {Text} from '@instructure/ui-text'
+import {TextInput} from '@instructure/ui-text-input'
+import {Button} from '@instructure/ui-buttons'
+import {View} from '@instructure/ui-view'
+import {Responsive} from '@instructure/ui-responsive'
+import {string} from 'prop-types'
+import I18n from 'i18n!bounced_emails'
+import doFetchApi from 'jsx/shared/effects/doFetchApi'
+import CanvasDateInput from 'jsx/shared/components/CanvasDateInput'
+import FriendlyDatetime from 'jsx/shared/FriendlyDatetime'
+import tz from 'timezone'
+
+BouncedEmailsView.propTypes = {
+  accountId: string.isRequired
+}
+
+export default function BouncedEmailsView({accountId}) {
+  const [loading, setLoading] = useState(false)
+  const [data, setData] = useState()
+  const [searchTerm, setSearchTerm] = useState('')
+  const [after, setAfter] = useState()
+  const [before, setBefore] = useState()
+  const [fetchError, setFetchError] = useState('')
+
+  const formatDate = date => {
+    return tz.format(date, 'date.formats.medium')
+  }
+
+  // so, uh, the report localizes column names, and we're just identifying them by position
+  // that's maybe a little brittle :(
+  const renderTableHeader = header => {
+    return (
+      <Table.Head>
+        <Table.Row>
+          <Table.ColHeader id="name">{header[1] /* Name */}</Table.ColHeader>
+          <Table.ColHeader id="path">{header[4] /* Path */}</Table.ColHeader>
+          <Table.ColHeader id="date" width="9rem">
+            {header[5] /* Date of most recent bounce */}
+          </Table.ColHeader>
+          <Table.ColHeader id="reason" width="50%">
+            {header[6] /* Bounce reason */}
+          </Table.ColHeader>
+        </Table.Row>
+      </Table.Head>
+    )
+  }
+
+  const renderTableRows = useCallback(body_data => {
+    return body_data.map(row => (
+      <Table.Row key={row[2] /* communication channel id */}>
+        <Table.Cell>
+          <a href={`/about/${row[0]}`}>{row[1]}</a>
+        </Table.Cell>
+        <Table.Cell>
+          <a href={`mailto:${row[4]}`}>{row[4]}</a>
+        </Table.Cell>
+        <Table.Cell>
+          <FriendlyDatetime dateTime={row[5]} format={I18n.t('#date.formats.medium')} />
+        </Table.Cell>
+        <Table.Cell>
+          <Text wrap="break-word">{row[6]}</Text>
+        </Table.Cell>
+      </Table.Row>
+    ))
+  }, [])
+
+  const renderTableBody = useCallback(
+    body_data => {
+      return <Table.Body>{renderTableRows(body_data)}</Table.Body>
+    },
+    [renderTableRows]
+  )
+
+  const onFetch = useCallback(
+    ({json}) => {
+      setData(json)
+      setFetchError('')
+      setLoading(false)
+    },
+    [setLoading, setData]
+  )
+
+  const onError = useCallback(() => {
+    setLoading(false)
+    setFetchError(I18n.t('Failed to perform search'))
+  }, [setLoading, setFetchError])
+
+  const performSearch = useCallback(() => {
+    const path = `/api/v1/accounts/${accountId}/bounced_communication_channels/`
+    const params = {order: 'desc'}
+    if (searchTerm) {
+      params.pattern = searchTerm
+    }
+    if (before) {
+      params.before = before.toISOString()
+    }
+    if (after) {
+      params.after = after.toISOString()
+    }
+    setLoading(true)
+    doFetchApi({path, params})
+      .then(onFetch)
+      .catch(onError)
+  }, [accountId, searchTerm, onFetch, onError, before, after])
+
+  const renderTable = useCallback(
+    table_data => {
+      if (loading) {
+        return <Spinner renderTitle={I18n.t('Loading')} margin="large auto 0 auto" />
+      }
+      if (!table_data) {
+        return null
+      }
+      if (table_data.length <= 1) {
+        // the only returned row is the table header
+        return <Text color="secondary">{I18n.t('No results')}</Text>
+      }
+      return (
+        <Responsive
+          query={{small: {maxWidth: '1000px'}, large: {minWidth: '1000px'}}}
+          props={{small: {layout: 'stacked'}, large: {layout: 'fixed'}}}
+        >
+          {props => (
+            <Table caption={I18n.t('Bounced Emails')} {...props}>
+              {renderTableHeader(table_data[0])}
+              {renderTableBody(table_data.slice(1))}
+            </Table>
+          )}
+        </Responsive>
+      )
+    },
+    [loading, renderTableHeader, renderTableBody]
+  )
+
+  return (
+    <>
+      <View as="div" margin="0 0 small 0">
+        <TextInput
+          renderLabel={I18n.t('Address (use * as wildcard)')}
+          placeholder={I18n.t('mfoster@*')}
+          value={searchTerm}
+          onChange={(_event, value) => {
+            setSearchTerm(value)
+          }}
+        />
+      </View>
+      <View as="div" margin="0 0 small 0">
+        <CanvasDateInput
+          renderLabel={I18n.t('Last bounced after')}
+          formatDate={formatDate}
+          onSelectedDateChange={setAfter}
+        />
+        &emsp;
+        <CanvasDateInput
+          renderLabel={I18n.t('Last bounced before')}
+          formatDate={formatDate}
+          onSelectedDateChange={setBefore}
+        />
+      </View>
+      <View as="div" margin="0 0 small 0">
+        <Button color="primary" margin="small 0 0 0" onClick={performSearch}>
+          {I18n.t('Search')}
+        </Button>
+      </View>
+      {fetchError ? <Text color="danger">{fetchError}</Text> : renderTable(data)}
+    </>
+  )
+}
--- a/app/jsx/bundles/account_admin_tools.js
+++ b/app/jsx/bundles/account_admin_tools.js
@ -32,6 +32,9 @@ import UserDateRangeSearchFormView from 'compiled/views/accounts/admin_tools/Use
 import CommMessageItemView from 'compiled/views/accounts/admin_tools/CommMessageItemView'
 import messagesSearchResultsTemplate from 'jst/accounts/admin_tools/commMessagesSearchResults'
 import usersTemplate from 'jst/accounts/usersList'
+import React from 'react'
+import ReactDOM from 'react-dom'
+import BouncedEmailsView from '../bounced_emails/BouncedEmailsView'
 import ready from '@instructure/ready'

 // This is used by admin tools to display search results
@ -70,7 +73,8 @@ ready(() => {
    tabs: {
      courseRestore: ENV.PERMISSIONS.restore_course,
      viewMessages: ENV.PERMISSIONS.view_messages,
-      logging: !!ENV.PERMISSIONS.logging
+      logging: !!ENV.PERMISSIONS.logging,
+      bouncedEmails: ENV.bounced_emails_admin_tool
    },
    restoreContentPaneView: new RestoreContentPaneView({
      courseSearchFormView: new CourseSearchFormView({model: restoreModel}),
@ -84,4 +88,9 @@ ready(() => {
  })

  app.render()
+
+  const bouncedEmailsMountPoint = document.getElementById('bouncedEmailsPane')
+  if (bouncedEmailsMountPoint) {
+    ReactDOM.render(<BouncedEmailsView accountId={ENV.ACCOUNT_ID} />, bouncedEmailsMountPoint)
+  }
 })
--- a/app/models/account.rb
+++ b/app/models/account.rb
@ -1220,6 +1220,12 @@ class Account < ActiveRecord::Base

    given { |user| !self.site_admin? && self.root_account? && self.grants_right?(user, :manage_site_settings) }
    can :manage_privacy_settings
+
+    given do |user|
+      self.root_account? && self.grants_right?(user, :read_roster) &&
+        (self.grants_right?(user, :view_notifications) || Account.site_admin.grants_right?(user, :read_messages))
+    end
+    can :view_bounced_emails
  end

  alias_method :destroy_permanently!, :destroy
--- a/app/models/communication_channel/bulk_actions.rb
+++ b/app/models/communication_channel/bulk_actions.rb
@ -24,12 +24,13 @@ class CommunicationChannel
    # bulk_limit will be used to limit the number of results returned.
    REPORT_LIMIT = 10_000

-    attr_reader :account, :after, :before, :pattern, :path_type
+    attr_reader :account, :after, :before, :order, :pattern, :path_type

-    def initialize(account:, after: nil, before: nil, pattern: nil, path_type: nil, with_invalid_paths: false)
-      @account, @pattern, @path_type, @with_invalid_paths = account, pattern, path_type, with_invalid_paths
+    def initialize(account:, after: nil, before: nil, order: nil, pattern: nil, path_type: nil, with_invalid_paths: false)
+      @account, @pattern, @path_type, @with_invalid_paths= account, pattern, path_type, with_invalid_paths
      @after = Time.zone.parse(after) if after
      @before = Time.zone.parse(before) if before
+      @order = order&.downcase == 'desc' ? :desc : :asc
    end

    def matching_channels(for_report: false)
@ -52,32 +53,47 @@ class CommunicationChannel
      end
    end

-    def report
+    def column_headers
+      [
+        I18n.t('User ID'),
+        I18n.t('Name'),
+        I18n.t('Communication channel ID'),
+        I18n.t('Type'),
+        I18n.t('Path')
+      ] + self.class.report_columns.keys
+    end
+
+    def column_data(cc)
+      [
+        cc.user.id,
+        cc.user.name,
+        cc.id,
+        cc.path_type,
+        cc.path_description
+      ] + self.class.report_columns.values.map { |value_generator| value_generator.to_proc.call(cc) }
+    end
+
+    def csv_report
      GuardRail.activate(:secondary) do
        CSV.generate do |csv|
-          columns = self.class.report_columns
-
-          csv << [
-            I18n.t('User ID'),
-            I18n.t('Name'),
-            I18n.t('Communication channel ID'),
-            I18n.t('Type'),
-            I18n.t('Path')
-          ] + columns.keys
-
+          csv << column_headers
          matching_channels(for_report: true).preload(:user).each do |cc|
-            csv << [
-              cc.user.id,
-              cc.user.name,
-              cc.id,
-              cc.path_type,
-              cc.path_description
-            ] + columns.values.map { |value_generator| value_generator.to_proc.call(cc) }
+            csv << column_data(cc)
          end
        end
      end
    end

+    def json_report
+      GuardRail.activate(:secondary) do
+        data = [column_headers]
+        matching_channels(for_report: true).preload(:user).each do |cc|
+          data << column_data(cc).map { |col| col&.to_s } # stringify ids but leave nulls alone
+        end
+        data.to_json
+      end
+    end
+
    class ResetBounceCounts < BulkActions
      def self.bulk_limit
        1000
@ -91,7 +107,7 @@ class CommunicationChannel
      end

      def filter(ccs)
-        ccs = ccs.where('bounce_count > 0').order(:last_bounce_at)
+        ccs = ccs.where('bounce_count > 0').order(last_bounce_at: order)
        ccs = ccs.where('last_bounce_at < ?', before) if before
        ccs = ccs.where('last_bounce_at > ?', after) if after
        ccs
@ -120,7 +136,7 @@ class CommunicationChannel
      end

      def filter(ccs)
-        ccs = ccs.where(workflow_state: 'unconfirmed').order(:created_at)
+        ccs = ccs.where(workflow_state: 'unconfirmed').order(created_at: order)
        ccs = ccs.where('created_at < ?', before) if before
        ccs = ccs.where('created_at > ?', after) if after
        ccs
--- a/app/views/jst/accounts/admin_tools/AdminTools.handlebars
+++ b/app/views/jst/accounts/admin_tools/AdminTools.handlebars
@ -9,6 +9,9 @@
    {{#if logging}}
    <li class="logging"><a href="#loggingPane">{{#t "tab_labels.admin_tools_logging"}}Logging{{/t}}</a></li>
    {{/if}}
+    {{#if bouncedEmails}}
+    <li class="bouncedEmails"><a href="#bouncedEmailsPane">{{#t}}Bounced Emails{{/t}}</a></li>
+    {{/if}}
  </ul>
  <div id="adminToolsTabPanes">
    {{#if courseRestore}}
@ -20,5 +23,8 @@
    {{#if logging}}
    <div id="loggingPane"></div>
    {{/if}}
+    {{#if bouncedEmails}}
+    <div id="bouncedEmailsPane"></div>
+    {{/if}}
  </div>
 </div>
--- a/config/feature_flags/learning_activities_release_flags.yml
+++ b/config/feature_flags/learning_activities_release_flags.yml
@ -102,3 +102,12 @@ inline_math_everywhere:
    This feature reequires that the "Updated math equation handling"
    flag is also on.
  applies_to: RootAccount
+bounced_emails_admin_tool:
+  state: hidden
+  display_name: Bounced Emails Admin Tool
+  description: |-
+    Adds a new Bounced Emails tab to Account Admin Tools, which
+    allows you to search email addresses where Canvas notifications
+    have bounced
+  applies_to: RootAccount
+
--- a/config/routes.rb
+++ b/config/routes.rb
@ -1554,9 +1554,11 @@ CanvasRails::Application.routes.draw do
      get 'users/:user_id/communication_channels', action: :index, as: 'communication_channels'
      post 'users/:user_id/communication_channels', action: :create
      post 'users/:user_id/communication_channels/:id', action: :reset_bounce_count, as: 'reset_bounce_count'
-      get 'accounts/:account_id/bounced_communication_channels.csv', action: :bouncing_channel_report
+      get 'accounts/:account_id/bounced_communication_channels.csv', action: :bouncing_channel_report, defaults: { format: :csv }
+      get 'accounts/:account_id/bounced_communication_channels', action: :bouncing_channel_report
      post 'accounts/:account_id/bounced_communication_channels/reset', action: :bulk_reset_bounce_counts
-      get 'accounts/:account_id/unconfirmed_communication_channels.csv', action: :unconfirmed_channel_report
+      get 'accounts/:account_id/unconfirmed_communication_channels.csv', action: :unconfirmed_channel_report, defaults: { format: :csv }
+      get 'accounts/:account_id/unconfirmed_communication_channels', action: :unconfirmed_channel_report
      post 'accounts/:account_id/unconfirmed_communication_channels/confirm', action: :bulk_confirm
      delete 'users/self/communication_channels/push', action: :delete_push_token
      delete 'users/:user_id/communication_channels/:id', action: :destroy
--- a/spec/controllers/communication_channels_controller_spec.rb
+++ b/spec/controllers/communication_channels_controller_spec.rb
@ -833,10 +833,16 @@ describe CommunicationChannelsController do
        ]
      end

-      context 'as a site admin' do
-        before do
-          account_admin_user(account: Account.site_admin)
-          user_session(@user)
+      context 'as an account admin' do
+        before :once do
+          @account = Account.default
+          @account.settings[:admins_can_view_notifications] = true
+          @account.save!
+          account_admin_user_with_role_changes(:account => @account, :role_changes => {view_notifications: true})
+        end
+
+        before :each do
+          user_session(@admin)
        end

        it 'fetches communication channels in this account and orders by date' do
@ -871,6 +877,16 @@ describe CommunicationChannelsController do
            channel_csv(c1),
            channel_csv(c3)
          ]
+
+          # also test JSON format
+          get 'bouncing_channel_report', params: {account_id: Account.default.id, format: :json}
+          json = JSON.parse(response.body)
+          expect(json).to eq [
+            ['User ID', 'Name', 'Communication channel ID', 'Type', 'Path', 'Date of most recent bounce', 'Bounce reason'],
+            channel_csv(c2),
+            channel_csv(c1),
+            channel_csv(c3)
+          ]
        end

        it 'ignores communication channels in other accounts' do
@ -914,8 +930,11 @@ describe CommunicationChannelsController do

        it 'uses the requested account' do
          a = account_model
-          user_with_pseudonym(account: a)
+          account_admin_user_with_role_changes(user: @admin, account: a, role_changes: {view_notifications: true})
+          a.settings[:admins_can_view_notifications] = true
+          a.save!

+          user_with_pseudonym(account: a)
          c = @user.communication_channels.create!(path: 'one@example.com', path_type: 'email') do |cc|
            cc.workflow_state = 'active'
            cc.bounce_count = 1
--- a/spec/models/account_spec.rb
+++ b/spec/models/account_spec.rb
@ -641,6 +641,7 @@ describe Account do

    limited_access = [ :read, :read_as_admin, :manage, :update, :delete, :read_outcomes, :read_terms ]
    conditional_access = RoleOverride.permissions.select { |_, v| v[:account_allows] }.map(&:first)
+    conditional_access += [:view_bounced_emails] # since this depends on :view_notifications
    disabled_by_default = RoleOverride.permissions.select { |_, v| v[:true_for].empty? }.map(&:first)
    full_access = RoleOverride.permissions.keys +
                  limited_access - disabled_by_default - conditional_access +
@ -659,7 +660,6 @@ describe Account do
      admin_privileges += [:manage_privacy_settings] if k == :root

      user_privileges = limited_access + common_siteadmin_privileges
-
      expect(account.check_policy(hash[:site_admin][:admin]) - conditional_access).to match_array admin_privileges
      expect(account.check_policy(hash[:site_admin][:user]) - conditional_access).to match_array user_privileges
    end
--- a/spec/selenium/admin_tools_spec.rb
+++ b/spec/selenium/admin_tools_spec.rb
@ -582,4 +582,50 @@ describe "admin_tools" do
      expect(fj('.ui-dialog dl dd:last').text).to eq @course.name
    end
  end
+
+  context "bounced emails search" do
+    before do
+      u1 = user_with_pseudonym
+      u2 = user_with_pseudonym
+      u1.communication_channels.create!(path: 'one@example.com', path_type: 'email') do |cc|
+        cc.workflow_state = 'active'
+        cc.bounce_count = 1
+        cc.last_bounce_at = 2.days.ago
+      end
+      u1.communication_channels.create!(path: 'two@example.com', path_type: 'email') do |cc|
+        cc.workflow_state = 'active'
+        cc.bounce_count = 2
+        cc.last_bounce_at = 4.days.ago
+      end
+      u2.communication_channels.create!(path: 'three@example.com', path_type: 'email') do |cc|
+        cc.workflow_state = 'active'
+        cc.bounce_count = 3
+        cc.last_bounce_at = 6.days.ago
+        cc.last_bounce_details = {'bouncedRecipients' => [{'diagnosticCode' => '550 what a luser'}]}
+      end
+      @account.enable_feature!(:bounced_emails_admin_tool)
+      @user = @account_admin
+    end
+
+    it "does not appear if the user lacks permission" do
+      load_admin_tools_page
+      expect(f('#adminToolsTabNav')).not_to contain_css('a[href="#bouncedEmailsPane"]')
+    end
+
+    it "performs searches" do
+      @account.settings[:admins_can_view_notifications] = true
+      @account.save!
+      load_admin_tools_page
+      f('a[href="#bouncedEmailsPane"]').click
+      replace_content fj('label:contains("Address") input'), '*@example.com'
+      replace_content fj('label:contains("Last bounced after") input'), 5.days.ago.iso8601
+      replace_content fj('label:contains("Last bounced before") input'), 3.days.ago.iso8601
+      fj('button:contains("Search")').click
+      wait_for_ajaximations
+      data = ff('#bouncedEmailsPane table td').map(&:text)
+      expect(data).not_to include 'one@example.com'
+      expect(data).to include 'two@example.com'
+      expect(data).not_to include 'three@example.com'
+    end
+  end
 end