From ba3ee3444eb9bad94a8f612f4c3636d4985e57eb Mon Sep 17 00:00:00 2001 From: Robin Ward Date: Thu, 13 Aug 2020 11:33:46 -0400 Subject: [PATCH] FIX: Move queryParams to each discovery controller rather than shared (#10424) * REFACTOR: `refreshSort` doesn't cause it to sort again, it's misleading * FIX: Move queryParams to each discovery controller rather than shared This fixes issues where params previously would not reset between routes. For example if you added `max_posts=1` to /latest and then went to a category. * Add backward compatibility for (action "changeSort") for themes * FIX: refreshing was not working * Update app/assets/javascripts/discourse/app/controllers/discovery/topics.js Co-authored-by: Jarek Radosz Co-authored-by: Jarek Radosz --- .../app/controllers/discovery-sortable.js | 32 ++++++++++--- .../app/controllers/discovery/topics.js | 48 +++++++++---------- .../discourse/app/models/topic-list.js | 4 +- .../app/routes/build-category-route.js | 11 ++++- .../discourse/app/routes/build-topic-route.js | 20 ++++---- .../app/templates/discovery/topics.hbs | 4 +- 6 files changed, 70 insertions(+), 49 deletions(-) diff --git a/app/assets/javascripts/discourse/app/controllers/discovery-sortable.js b/app/assets/javascripts/discourse/app/controllers/discovery-sortable.js index fdaf34cd80f..5c17cdfb8c7 100644 --- a/app/assets/javascripts/discourse/app/controllers/discovery-sortable.js +++ b/app/assets/javascripts/discourse/app/controllers/discovery-sortable.js @@ -1,11 +1,10 @@ -import { alias } from "@ember/object/computed"; import { inject } from "@ember/controller"; import Controller from "@ember/controller"; // Just add query params here to have them automatically passed to topic list filters. export const queryParams = { order: { replace: true, refreshModel: true }, - ascending: { replace: true, refreshModel: true }, + ascending: { replace: true, refreshModel: true, default: false }, status: { replace: true, refreshModel: true }, state: { replace: true, refreshModel: true }, search: { replace: true, refreshModel: true }, @@ -23,17 +22,36 @@ const controllerOpts = { queryParams: Object.keys(queryParams) }; -// Aliases for the values -controllerOpts.queryParams.forEach( - p => (controllerOpts[p] = alias(`discoveryTopics.${p}`)) -); +// Default to `null` +controllerOpts.queryParams.forEach(p => { + controllerOpts[p] = queryParams[p].default; +}); + +export function changeSort(sortBy) { + let { controller } = this; + let model = this.controllerFor("discovery.topics").model; + if (sortBy === controller.order) { + controller.toggleProperty("ascending"); + model.updateSortParams(sortBy, controller.ascending); + } else { + controller.setProperties({ order: sortBy, ascending: false }); + model.updateSortParams(sortBy, false); + } +} + +export function resetParams() { + let { controller } = this; + controllerOpts.queryParams.forEach(p => { + controller.set(p, queryParams[p].default); + }); +} const SortableController = Controller.extend(controllerOpts); export const addDiscoveryQueryParam = function(p, opts) { queryParams[p] = opts; const cOpts = {}; - cOpts[p] = alias(`discoveryTopics.${p}`); + cOpts[p] = null; cOpts["queryParams"] = Object.keys(queryParams); SortableController.reopen(cOpts); }; diff --git a/app/assets/javascripts/discourse/app/controllers/discovery/topics.js b/app/assets/javascripts/discourse/app/controllers/discovery/topics.js index 3ea86d344a8..2b55556c671 100644 --- a/app/assets/javascripts/discourse/app/controllers/discovery/topics.js +++ b/app/assets/javascripts/discourse/app/controllers/discovery/topics.js @@ -1,19 +1,30 @@ import I18n from "I18n"; import discourseComputed from "discourse-common/utils/decorators"; -import { alias, not, gt, empty, notEmpty, equal } from "@ember/object/computed"; +import { + alias, + not, + gt, + empty, + notEmpty, + equal, + readOnly +} from "@ember/object/computed"; import { inject as controller } from "@ember/controller"; import DiscoveryController from "discourse/controllers/discovery"; -import { queryParams } from "discourse/controllers/discovery-sortable"; import BulkTopicSelection from "discourse/mixins/bulk-topic-selection"; import { endWith } from "discourse/lib/computed"; import showModal from "discourse/lib/show-modal"; import { userPath } from "discourse/lib/url"; import TopicList from "discourse/models/topic-list"; import Topic from "discourse/models/topic"; +import { routeAction } from "discourse/helpers/route-action"; +import { inject as service } from "@ember/service"; +import deprecated from "discourse-common/lib/deprecated"; const controllerOpts = { discovery: controller(), discoveryTopics: controller("discovery/topics"), + router: service(), period: null, @@ -21,27 +32,19 @@ const controllerOpts = { showTopicPostBadges: not("discoveryTopics.new"), redirectedReason: alias("currentUser.redirected_to_top.reason"), - order: null, - ascending: false, expandGloballyPinned: false, expandAllPinned: false, - resetParams() { - Object.keys(this.get("model.params") || {}).forEach(key => { - // controllerOpts contains the default values for parameters, so use them. They might be null. - this.set(key, controllerOpts[key]); - }); - }, + order: readOnly("model.params.order"), + ascending: readOnly("model.params.ascending"), actions: { - changeSort(sortBy) { - if (sortBy === this.order) { - this.toggleProperty("ascending"); - this.model.refreshSort(sortBy, this.ascending); - } else { - this.setProperties({ order: sortBy, ascending: false }); - this.model.refreshSort(sortBy, false); - } + changeSort() { + deprecated( + "changeSort has been changed from an (action) to a (route-action)", + { since: "2.6.0", dropFrom: "2.7.0" } + ); + return routeAction("changeSort", this.router._router, ...arguments)(); }, // Show newly inserted topics @@ -56,7 +59,7 @@ const controllerOpts = { refresh() { const filter = this.get("model.filter"); - this.resetParams(); + this.send("resetParams"); // Don't refresh if we're still loading if (this.get("discovery.loading")) { @@ -175,11 +178,4 @@ const controllerOpts = { } }; -Object.keys(queryParams).forEach(function(p) { - // If we don't have a default value, initialize it to null - if (typeof controllerOpts[p] === "undefined") { - controllerOpts[p] = null; - } -}); - export default DiscoveryController.extend(controllerOpts, BulkTopicSelection); diff --git a/app/assets/javascripts/discourse/app/models/topic-list.js b/app/assets/javascripts/discourse/app/models/topic-list.js index 16b2b67c6b1..c5239d43040 100644 --- a/app/assets/javascripts/discourse/app/models/topic-list.js +++ b/app/assets/javascripts/discourse/app/models/topic-list.js @@ -55,8 +55,8 @@ const TopicList = RestModel.extend({ }); }, - refreshSort(order, ascending) { - let params = this.params || {}; + updateSortParams(order, ascending) { + let params = Object.assign({}, this.params || {}); if (params.q) { // search is unique, nothing else allowed with it diff --git a/app/assets/javascripts/discourse/app/routes/build-category-route.js b/app/assets/javascripts/discourse/app/routes/build-category-route.js index 78c08dd2163..b4a5475d9b5 100644 --- a/app/assets/javascripts/discourse/app/routes/build-category-route.js +++ b/app/assets/javascripts/discourse/app/routes/build-category-route.js @@ -4,7 +4,11 @@ import { filterQueryParams, findTopicList } from "discourse/routes/build-topic-route"; -import { queryParams } from "discourse/controllers/discovery-sortable"; +import { + changeSort, + resetParams, + queryParams +} from "discourse/controllers/discovery-sortable"; import TopicList from "discourse/models/topic-list"; import PermissionType from "discourse/models/permission-type"; import CategoryList from "discourse/models/category-list"; @@ -253,7 +257,10 @@ export default (filterArg, params) => { triggerRefresh() { this.refresh(); - } + }, + + changeSort, + resetParams } }); }; diff --git a/app/assets/javascripts/discourse/app/routes/build-topic-route.js b/app/assets/javascripts/discourse/app/routes/build-topic-route.js index 6b6b8dadc28..e05fdcc7f5c 100644 --- a/app/assets/javascripts/discourse/app/routes/build-topic-route.js +++ b/app/assets/javascripts/discourse/app/routes/build-topic-route.js @@ -1,7 +1,11 @@ import { isEmpty } from "@ember/utils"; import I18n from "I18n"; import DiscourseRoute from "discourse/routes/discourse"; -import { queryParams } from "discourse/controllers/discovery-sortable"; +import { + changeSort, + resetParams, + queryParams +} from "discourse/controllers/discovery-sortable"; import { defaultHomepage } from "discourse/lib/utilities"; import Session from "discourse/models/session"; import { Promise } from "rsvp"; @@ -131,15 +135,6 @@ export default function(filter, extras) { expandGloballyPinned: true }; - const params = model.get("params"); - if (params && Object.keys(params).length) { - if (params.order !== undefined) { - topicOpts.order = params.order; - } - if (params.ascending !== undefined) { - topicOpts.ascending = params.ascending; - } - } this.controllerFor("discovery/topics").setProperties(topicOpts); this.controllerFor("navigation/default").set( "canCreateTopic", @@ -153,6 +148,11 @@ export default function(filter, extras) { controller: "discovery/topics", outlet: "list-container" }); + }, + + actions: { + changeSort, + resetParams } }, extras diff --git a/app/assets/javascripts/discourse/app/templates/discovery/topics.hbs b/app/assets/javascripts/discourse/app/templates/discovery/topics.hbs index 6c5b479624b..c8ae5159dff 100644 --- a/app/assets/javascripts/discourse/app/templates/discovery/topics.hbs +++ b/app/assets/javascripts/discourse/app/templates/discovery/topics.hbs @@ -59,7 +59,7 @@ showTopicPostBadges=showTopicPostBadges showPosters=true canBulkSelect=canBulkSelect - changeSort=(action "changeSort") + changeSort=(route-action "changeSort") toggleBulkSelect=(action "toggleBulkSelect") hideCategory=model.hideCategory order=order @@ -110,4 +110,4 @@ {{/footer-message}} {{/if}} - \ No newline at end of file +