From 292a67aede0eacc514b426f184f8ecd6b6b65569 Mon Sep 17 00:00:00 2001 From: Shamim Rezaie Date: Tue, 9 Jun 2020 21:34:44 +1000 Subject: [PATCH] MDL-68991 core: Prevent popup blockers blocking feedback window Some browsers like Firefox are very inflexible with window.open() and block it if it is not instantly invoked after the user click. Also according to https://stackoverflow.com/a/6807615 it is best practice to replace self:: with static:: --- lib/amd/build/userfeedback.min.js | 2 +- lib/amd/build/userfeedback.min.js.map | 2 +- lib/amd/src/userfeedback.js | 28 ++++++--------------------- lib/classes/userfeedback.php | 10 +++++----- lib/tests/behat/userfeedback.feature | 14 ++++++++++++++ 5 files changed, 27 insertions(+), 29 deletions(-) diff --git a/lib/amd/build/userfeedback.min.js b/lib/amd/build/userfeedback.min.js index 3eee54bf623..03f8faf3389 100644 --- a/lib/amd/build/userfeedback.min.js +++ b/lib/amd/build/userfeedback.min.js @@ -1,2 +1,2 @@ -define ("core/userfeedback",["exports","core/ajax","core/notification"],function(a,b,c){"use strict";Object.defineProperty(a,"__esModule",{value:!0});a.registerEventListeners=void 0;b=d(b);c=d(c);function d(a){return a&&a.__esModule?a:{default:a}}var f={regions:{root:"[data-region=\"core/userfeedback\"]"},actions:{}};f.actions.give="".concat(f.regions.root," [data-action=\"give\"]");f.actions.remind="".concat(f.regions.root," [data-action=\"remind\"]");a.registerEventListeners=function registerEventListeners(){document.addEventListener("click",function(a){var b=a.target.closest(f.actions.give);if(b){a.preventDefault();g().then(function(){return i(b)}).then(h).catch(c.default.exception)}var d=a.target.closest(f.actions.remind);if(d){a.preventDefault();Promise.resolve(d).then(i).then(h).catch(c.default.exception)}})};var g=function(){return b.default.call([{methodname:"core_get_userfeedback_url",args:{contextid:M.cfg.contextid}}])[0].then(function(a){if(!window.open(a)){throw new Error("Unable to open popup")}})},h=function(a){if(a.dataset.record){return b.default.call([{methodname:"core_create_userfeedback_action_record",args:{action:a.dataset.action,contextid:M.cfg.contextid}}])[0]}return Promise.resolve()},i=function(a){if(a.dataset.hide){a.closest(f.regions.root).remove()}return a}}); +define ("core/userfeedback",["exports","core/ajax","core/notification"],function(a,b,c){"use strict";Object.defineProperty(a,"__esModule",{value:!0});a.registerEventListeners=void 0;b=d(b);c=d(c);function d(a){return a&&a.__esModule?a:{default:a}}var f={regions:{root:"[data-region=\"core/userfeedback\"]"},actions:{}};f.actions.give="".concat(f.regions.root," [data-action=\"give\"]");f.actions.remind="".concat(f.regions.root," [data-action=\"remind\"]");a.registerEventListeners=function registerEventListeners(){document.addEventListener("click",function(a){var b=a.target.closest(f.actions.give);if(b){a.preventDefault();if(!window.open(b.href)){throw new Error("Unable to open popup")}Promise.resolve(b).then(h).then(g).catch(c.default.exception)}var d=a.target.closest(f.actions.remind);if(d){a.preventDefault();Promise.resolve(d).then(h).then(g).catch(c.default.exception)}})};var g=function(a){if(a.dataset.record){return b.default.call([{methodname:"core_create_userfeedback_action_record",args:{action:a.dataset.action,contextid:M.cfg.contextid}}])[0]}return Promise.resolve()},h=function(a){if(a.dataset.hide){a.closest(f.regions.root).remove()}return a}}); //# sourceMappingURL=userfeedback.min.js.map diff --git a/lib/amd/build/userfeedback.min.js.map b/lib/amd/build/userfeedback.min.js.map index 2a4225f3097..71fb5b65041 100644 --- a/lib/amd/build/userfeedback.min.js.map +++ b/lib/amd/build/userfeedback.min.js.map @@ -1 +1 @@ -{"version":3,"sources":["../src/userfeedback.js"],"names":["Selectors","regions","root","actions","give","remind","registerEventListeners","document","addEventListener","e","giveAction","target","closest","preventDefault","giveFeedback","then","hideRoot","recordAction","catch","Notification","exception","remindAction","Promise","resolve","Ajax","call","methodname","args","contextid","M","cfg","url","window","open","Error","clickedItem","dataset","record","action","hide","remove"],"mappings":"sLAuBA,OACA,O,mDAEA,GAAMA,CAAAA,CAAS,CAAG,CACdC,OAAO,CAAE,CACLC,IAAI,CAAE,qCADD,CADK,CAIdC,OAAO,CAAE,EAJK,CAAlB,CAMAH,CAAS,CAACG,OAAV,CAAkBC,IAAlB,WAA4BJ,CAAS,CAACC,OAAV,CAAkBC,IAA9C,4BACAF,CAAS,CAACG,OAAV,CAAkBE,MAAlB,WAA8BL,CAAS,CAACC,OAAV,CAAkBC,IAAhD,8B,yBAKsC,QAAzBI,CAAAA,sBAAyB,EAAM,CACxCC,QAAQ,CAACC,gBAAT,CAA0B,OAA1B,CAAmC,SAAAC,CAAC,CAAI,CACpC,GAAMC,CAAAA,CAAU,CAAGD,CAAC,CAACE,MAAF,CAASC,OAAT,CAAiBZ,CAAS,CAACG,OAAV,CAAkBC,IAAnC,CAAnB,CACA,GAAIM,CAAJ,CAAgB,CACZD,CAAC,CAACI,cAAF,GAEAC,CAAY,GACPC,IADL,CACU,iBAAMC,CAAAA,CAAQ,CAACN,CAAD,CAAd,CADV,EAEKK,IAFL,CAEUE,CAFV,EAGKC,KAHL,CAGWC,UAAaC,SAHxB,CAIH,CAED,GAAMC,CAAAA,CAAY,CAAGZ,CAAC,CAACE,MAAF,CAASC,OAAT,CAAiBZ,CAAS,CAACG,OAAV,CAAkBE,MAAnC,CAArB,CACA,GAAIgB,CAAJ,CAAkB,CACdZ,CAAC,CAACI,cAAF,GAEAS,OAAO,CAACC,OAAR,CAAgBF,CAAhB,EACKN,IADL,CACUC,CADV,EAEKD,IAFL,CAEUE,CAFV,EAGKC,KAHL,CAGWC,UAAaC,SAHxB,CAIH,CACJ,CApBD,CAqBH,C,IAOKN,CAAAA,CAAY,CAAG,UAAM,CACvB,MAAOU,WAAKC,IAAL,CAAU,CAAC,CACdC,UAAU,CAAE,2BADE,CAEdC,IAAI,CAAE,CACFC,SAAS,CAAEC,CAAC,CAACC,GAAF,CAAMF,SADf,CAFQ,CAAD,CAAV,EAKH,CALG,EAMFb,IANE,CAMG,SAAAgB,CAAG,CAAI,CACT,GAAI,CAACC,MAAM,CAACC,IAAP,CAAYF,CAAZ,CAAL,CAAuB,CACnB,KAAM,IAAIG,CAAAA,KAAJ,CAAU,sBAAV,CACT,CAEJ,CAXE,CAYV,C,CAQKjB,CAAY,CAAG,SAAAkB,CAAW,CAAI,CAChC,GAAIA,CAAW,CAACC,OAAZ,CAAoBC,MAAxB,CAAgC,CAC5B,MAAOb,WAAKC,IAAL,CAAU,CAAC,CACdC,UAAU,CAAE,wCADE,CAEdC,IAAI,CAAE,CACFW,MAAM,CAAEH,CAAW,CAACC,OAAZ,CAAoBE,MAD1B,CAEFV,SAAS,CAAEC,CAAC,CAACC,GAAF,CAAMF,SAFf,CAFQ,CAAD,CAAV,EAMH,CANG,CAOV,CAED,MAAON,CAAAA,OAAO,CAACC,OAAR,EACV,C,CAQKP,CAAQ,CAAG,SAAAmB,CAAW,CAAI,CAC5B,GAAIA,CAAW,CAACC,OAAZ,CAAoBG,IAAxB,CAA8B,CAC1BJ,CAAW,CAACvB,OAAZ,CAAoBZ,CAAS,CAACC,OAAV,CAAkBC,IAAtC,EAA4CsC,MAA5C,EACH,CAED,MAAOL,CAAAA,CACV,C","sourcesContent":["// This file is part of Moodle - http://moodle.org/\n//\n// Moodle is free software: you can redistribute it and/or modify\n// it under the terms of the GNU General Public License as published by\n// the Free Software Foundation, either version 3 of the License, or\n// (at your option) any later version.\n//\n// Moodle is distributed in the hope that it will be useful,\n// but WITHOUT ANY WARRANTY; without even the implied warranty of\n// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the\n// GNU General Public License for more details.\n//\n// You should have received a copy of the GNU General Public License\n// along with Moodle. If not, see .\n\n/**\n * Handle clicking on action links of the feedback alert.\n *\n * @module core/cta_feedback\n * @copyright 2020 Shamim Rezaie \n * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later\n */\n\nimport Ajax from 'core/ajax';\nimport Notification from 'core/notification';\n\nconst Selectors = {\n regions: {\n root: '[data-region=\"core/userfeedback\"]',\n },\n actions: {},\n};\nSelectors.actions.give = `${Selectors.regions.root} [data-action=\"give\"]`;\nSelectors.actions.remind = `${Selectors.regions.root} [data-action=\"remind\"]`;\n\n/**\n * Attach the necessary event handlers to the action links\n */\nexport const registerEventListeners = () => {\n document.addEventListener('click', e => {\n const giveAction = e.target.closest(Selectors.actions.give);\n if (giveAction) {\n e.preventDefault();\n\n giveFeedback()\n .then(() => hideRoot(giveAction))\n .then(recordAction)\n .catch(Notification.exception);\n }\n\n const remindAction = e.target.closest(Selectors.actions.remind);\n if (remindAction) {\n e.preventDefault();\n\n Promise.resolve(remindAction)\n .then(hideRoot)\n .then(recordAction)\n .catch(Notification.exception);\n }\n });\n};\n\n/**\n * The action function that is called when users choose to give feedback.\n *\n * @returns {Promise}\n */\nconst giveFeedback = () => {\n return Ajax.call([{\n methodname: 'core_get_userfeedback_url',\n args: {\n contextid: M.cfg.contextid,\n }\n }])[0]\n .then(url => {\n if (!window.open(url)) {\n throw new Error('Unable to open popup');\n }\n return;\n });\n};\n\n/**\n * Record the action that the user took.\n *\n * @param {HTMLElement} clickedItem The action element that the user chose.\n * @returns {Promise}\n */\nconst recordAction = clickedItem => {\n if (clickedItem.dataset.record) {\n return Ajax.call([{\n methodname: 'core_create_userfeedback_action_record',\n args: {\n action: clickedItem.dataset.action,\n contextid: M.cfg.contextid,\n }\n }])[0];\n }\n\n return Promise.resolve();\n};\n\n/**\n * Hide the root node of the CTA notification.\n *\n * @param {HTMLElement} clickedItem The action element that the user chose.\n * @returns {HTMLElement}\n */\nconst hideRoot = clickedItem => {\n if (clickedItem.dataset.hide) {\n clickedItem.closest(Selectors.regions.root).remove();\n }\n\n return clickedItem;\n};\n"],"file":"userfeedback.min.js"} \ No newline at end of file +{"version":3,"sources":["../src/userfeedback.js"],"names":["Selectors","regions","root","actions","give","remind","registerEventListeners","document","addEventListener","e","giveAction","target","closest","preventDefault","window","open","href","Error","Promise","resolve","then","hideRoot","recordAction","catch","Notification","exception","remindAction","clickedItem","dataset","record","Ajax","call","methodname","args","action","contextid","M","cfg","hide","remove"],"mappings":"sLAuBA,OACA,O,mDAEA,GAAMA,CAAAA,CAAS,CAAG,CACdC,OAAO,CAAE,CACLC,IAAI,CAAE,qCADD,CADK,CAIdC,OAAO,CAAE,EAJK,CAAlB,CAMAH,CAAS,CAACG,OAAV,CAAkBC,IAAlB,WAA4BJ,CAAS,CAACC,OAAV,CAAkBC,IAA9C,4BACAF,CAAS,CAACG,OAAV,CAAkBE,MAAlB,WAA8BL,CAAS,CAACC,OAAV,CAAkBC,IAAhD,8B,yBAKsC,QAAzBI,CAAAA,sBAAyB,EAAM,CACxCC,QAAQ,CAACC,gBAAT,CAA0B,OAA1B,CAAmC,SAAAC,CAAC,CAAI,CACpC,GAAMC,CAAAA,CAAU,CAAGD,CAAC,CAACE,MAAF,CAASC,OAAT,CAAiBZ,CAAS,CAACG,OAAV,CAAkBC,IAAnC,CAAnB,CACA,GAAIM,CAAJ,CAAgB,CACZD,CAAC,CAACI,cAAF,GAEA,GAAI,CAACC,MAAM,CAACC,IAAP,CAAYL,CAAU,CAACM,IAAvB,CAAL,CAAmC,CAC/B,KAAM,IAAIC,CAAAA,KAAJ,CAAU,sBAAV,CACT,CAEDC,OAAO,CAACC,OAAR,CAAgBT,CAAhB,EACKU,IADL,CACUC,CADV,EAEKD,IAFL,CAEUE,CAFV,EAGKC,KAHL,CAGWC,UAAaC,SAHxB,CAIH,CAED,GAAMC,CAAAA,CAAY,CAAGjB,CAAC,CAACE,MAAF,CAASC,OAAT,CAAiBZ,CAAS,CAACG,OAAV,CAAkBE,MAAnC,CAArB,CACA,GAAIqB,CAAJ,CAAkB,CACdjB,CAAC,CAACI,cAAF,GAEAK,OAAO,CAACC,OAAR,CAAgBO,CAAhB,EACKN,IADL,CACUC,CADV,EAEKD,IAFL,CAEUE,CAFV,EAGKC,KAHL,CAGWC,UAAaC,SAHxB,CAIH,CACJ,CAxBD,CAyBH,C,IAQKH,CAAAA,CAAY,CAAG,SAAAK,CAAW,CAAI,CAChC,GAAIA,CAAW,CAACC,OAAZ,CAAoBC,MAAxB,CAAgC,CAC5B,MAAOC,WAAKC,IAAL,CAAU,CAAC,CACdC,UAAU,CAAE,wCADE,CAEdC,IAAI,CAAE,CACFC,MAAM,CAAEP,CAAW,CAACC,OAAZ,CAAoBM,MAD1B,CAEFC,SAAS,CAAEC,CAAC,CAACC,GAAF,CAAMF,SAFf,CAFQ,CAAD,CAAV,EAMH,CANG,CAOV,CAED,MAAOjB,CAAAA,OAAO,CAACC,OAAR,EACV,C,CAQKE,CAAQ,CAAG,SAAAM,CAAW,CAAI,CAC5B,GAAIA,CAAW,CAACC,OAAZ,CAAoBU,IAAxB,CAA8B,CAC1BX,CAAW,CAACf,OAAZ,CAAoBZ,CAAS,CAACC,OAAV,CAAkBC,IAAtC,EAA4CqC,MAA5C,EACH,CAED,MAAOZ,CAAAA,CACV,C","sourcesContent":["// This file is part of Moodle - http://moodle.org/\n//\n// Moodle is free software: you can redistribute it and/or modify\n// it under the terms of the GNU General Public License as published by\n// the Free Software Foundation, either version 3 of the License, or\n// (at your option) any later version.\n//\n// Moodle is distributed in the hope that it will be useful,\n// but WITHOUT ANY WARRANTY; without even the implied warranty of\n// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the\n// GNU General Public License for more details.\n//\n// You should have received a copy of the GNU General Public License\n// along with Moodle. If not, see .\n\n/**\n * Handle clicking on action links of the feedback alert.\n *\n * @module core/cta_feedback\n * @copyright 2020 Shamim Rezaie \n * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later\n */\n\nimport Ajax from 'core/ajax';\nimport Notification from 'core/notification';\n\nconst Selectors = {\n regions: {\n root: '[data-region=\"core/userfeedback\"]',\n },\n actions: {},\n};\nSelectors.actions.give = `${Selectors.regions.root} [data-action=\"give\"]`;\nSelectors.actions.remind = `${Selectors.regions.root} [data-action=\"remind\"]`;\n\n/**\n * Attach the necessary event handlers to the action links\n */\nexport const registerEventListeners = () => {\n document.addEventListener('click', e => {\n const giveAction = e.target.closest(Selectors.actions.give);\n if (giveAction) {\n e.preventDefault();\n\n if (!window.open(giveAction.href)) {\n throw new Error('Unable to open popup');\n }\n\n Promise.resolve(giveAction)\n .then(hideRoot)\n .then(recordAction)\n .catch(Notification.exception);\n }\n\n const remindAction = e.target.closest(Selectors.actions.remind);\n if (remindAction) {\n e.preventDefault();\n\n Promise.resolve(remindAction)\n .then(hideRoot)\n .then(recordAction)\n .catch(Notification.exception);\n }\n });\n};\n\n/**\n * Record the action that the user took.\n *\n * @param {HTMLElement} clickedItem The action element that the user chose.\n * @returns {Promise}\n */\nconst recordAction = clickedItem => {\n if (clickedItem.dataset.record) {\n return Ajax.call([{\n methodname: 'core_create_userfeedback_action_record',\n args: {\n action: clickedItem.dataset.action,\n contextid: M.cfg.contextid,\n }\n }])[0];\n }\n\n return Promise.resolve();\n};\n\n/**\n * Hide the root node of the CTA notification.\n *\n * @param {HTMLElement} clickedItem The action element that the user chose.\n * @returns {HTMLElement}\n */\nconst hideRoot = clickedItem => {\n if (clickedItem.dataset.hide) {\n clickedItem.closest(Selectors.regions.root).remove();\n }\n\n return clickedItem;\n};\n"],"file":"userfeedback.min.js"} \ No newline at end of file diff --git a/lib/amd/src/userfeedback.js b/lib/amd/src/userfeedback.js index c1b1a7f56e4..28a77eddcb3 100644 --- a/lib/amd/src/userfeedback.js +++ b/lib/amd/src/userfeedback.js @@ -42,8 +42,12 @@ export const registerEventListeners = () => { if (giveAction) { e.preventDefault(); - giveFeedback() - .then(() => hideRoot(giveAction)) + if (!window.open(giveAction.href)) { + throw new Error('Unable to open popup'); + } + + Promise.resolve(giveAction) + .then(hideRoot) .then(recordAction) .catch(Notification.exception); } @@ -60,26 +64,6 @@ export const registerEventListeners = () => { }); }; -/** - * The action function that is called when users choose to give feedback. - * - * @returns {Promise} - */ -const giveFeedback = () => { - return Ajax.call([{ - methodname: 'core_get_userfeedback_url', - args: { - contextid: M.cfg.contextid, - } - }])[0] - .then(url => { - if (!window.open(url)) { - throw new Error('Unable to open popup'); - } - return; - }); -}; - /** * Record the action that the user took. * diff --git a/lib/classes/userfeedback.php b/lib/classes/userfeedback.php index 8128336cdf0..7d315ee6a6f 100644 --- a/lib/classes/userfeedback.php +++ b/lib/classes/userfeedback.php @@ -57,9 +57,9 @@ class core_userfeedback { $actions = [ [ 'title' => get_string('calltofeedback_give'), - 'url' => '#', + 'url' => static::make_link()->out(false), 'data' => [ - 'action' => 'give', + 'action' => 'give', 'record' => 1, 'hide' => 1, ], @@ -103,13 +103,13 @@ class core_userfeedback { $lastactiontime = max($give ?: 0, $remind ?: 0); switch ($CFG->userfeedback_nextreminder) { - case self::REMIND_AFTER_UPGRADE: - $lastupgrade = self::last_major_upgrade_time(); + case static::REMIND_AFTER_UPGRADE: + $lastupgrade = static::last_major_upgrade_time(); if ($lastupgrade >= $lastactiontime) { return $lastupgrade + ($CFG->userfeedback_remindafter * DAYSECS) < time(); } break; - case self::REMIND_PERIODICALLY: + case static::REMIND_PERIODICALLY: return $lastactiontime + ($CFG->userfeedback_remindafter * DAYSECS) < time(); break; } diff --git a/lib/tests/behat/userfeedback.feature b/lib/tests/behat/userfeedback.feature index b090f807f67..6cf33d6a21e 100644 --- a/lib/tests/behat/userfeedback.feature +++ b/lib/tests/behat/userfeedback.feature @@ -34,3 +34,17 @@ Feature: Gathering user feedback And I reload the page Then I should not see "Give feedback" in the "region-main" "region" And I should not see "Remind me later" in the "region-main" "region" + + @javascript + Scenario: Users should not see the notification after they click on the give feedback link + Given the following config values are set as admin: + | enableuserfeedback | 1 | + | userfeedback_nextreminder | 2 | + | userfeedback_remindafter | 90 | + When I log in as "admin" + And I follow "Dashboard" in the user menu + And I click on "Give feedback" "link" + And I close all opened windows + And I reload the page + Then I should not see "Give feedback" in the "region-main" "region" + And I should not see "Remind me later" in the "region-main" "region"