EBernhardson has uploaded a new change for review.
https://gerrit.wikimedia.org/r/183985
Change subject: TOC bug: duplicated topics in some views
......................................................................
TOC bug: duplicated topics in some views
Not all pagination links include their current sorting option, this means
that it defaults to the per-user options. Because of this the first page
you load could be in one sort order, then the infinite scroll pulls in the
next page with a different sort order, resulting in duplicate topics.
* adjusts the code to always include the sort order in the generated
pagination links.
* adjusts the handling around offset-id/offset in the same code to be more
consistent and throw exceptions when invalid arguments are provided.
* always specify the find options, previously 'newest' was allowed to take
the defaults which works but ws not explicit enough.
Fixes T85882
Bug: T85582
Change-Id: I627e769bdf733bdea13a04ef6ed23ec590af2499
---
M includes/Block/TopicList.php
M tests/phpunit/api/ApiFlowViewTopicListTest.php
2 files changed, 40 insertions(+), 28 deletions(-)
git pull ssh://gerrit.wikimedia.org:29418/mediawiki/extensions/Flow
refs/changes/85/183985/1
diff --git a/includes/Block/TopicList.php b/includes/Block/TopicList.php
index 27096a2..a12e3c2 100644
--- a/includes/Block/TopicList.php
+++ b/includes/Block/TopicList.php
@@ -5,6 +5,7 @@
use Flow\Container;
use Flow\Data\Pager\Pager;
use Flow\Data\Pager\PagerPage;
+use Flow\Exception\FlowException;
use Flow\Formatter\TocTopicListFormatter;
use Flow\Formatter\TopicListFormatter;
use Flow\Formatter\TopicListQuery;
@@ -182,13 +183,9 @@
// @todo remove the 'api' => true, its always api
$findOptions = $this->getFindOptions( $options + array( 'api'
=> true ) );
- // sortby option
- if ( isset( $findOptions['sortby'] ) ) {
- $response['sortby'] = $findOptions['sortby'];
- // default is newest
- } else {
- $response['sortby'] = 'newest';
- }
+ // include the current sortby option. Note that when 'user' is
either
+ // submitted or defaulted to this is the resulting sort. ex:
newest
+ $response['sortby'] = $findOptions['sortby'];
if ( $this->workflow->isNew() ) {
return $response + $serializer->buildEmptyResult(
$this->workflow );
@@ -262,8 +259,41 @@
'api' => true,
'sortby' => 'user',
);
+
+ $sortByOption = '';
+ $user = $this->context->getUser();
if ( strlen( $requestOptions['sortby'] ) === 0 ) {
$requestOptions['sortby'] = 'user';
+ }
+ // the sortby option in $findOptions is not directly used for
querying,
+ // but is needed by the pager to generate appropriate
pagination links.
+ if ( $requestOptions['sortby'] === 'user' && !$user->isAnon() )
{
+ $requestOptions['sortby'] = $user->getOption(
'flow-topiclist-sortby' );
+ }
+ switch( $requestOptions['sortby'] ) {
+ case 'updated':
+ $findOptions = array(
+ 'sortby' => 'updated',
+ 'sort' => 'workflow_last_update_timestamp',
+ 'order' => 'desc',
+ ) + $findOptions;
+
+ if ( $requestOptions['offset'] ) {
+ throw new FlowException( 'The `updated` sort
order does not allow the `offset` parameter. Please use `offset-id`.' );
+ }
+ break;
+
+ case 'newest':
+ default:
+ $findOptions = array(
+ 'sortby' => 'newest',
+ 'sort' => 'topic_id',
+ 'order' => 'desc',
+ ) + $findOptions;
+
+ if ( $requestOptions['offset-id'] ) {
+ throw new FlowException( 'The `newest` sort
order does not allow the `offset-id` parameter. Please use `offset`.' );
+ }
}
if ( $requestOptions['offset-id'] ) {
@@ -286,32 +316,13 @@
$findOptions['pager-limit'] = $limit;
- $sortByOption = '';
- $user = $this->context->getUser();
- if ( $requestOptions['sortby'] === 'user' && !$user->isAnon() )
{
- $sortByOption = $user->getOption(
'flow-topiclist-sortby' );
- } elseif ( $requestOptions['sortby'] === 'updated' ) {
- $sortByOption = 'updated';
- }
-
if (
isset( $requestOptions['savesortby'] )
&& !$user->isAnon()
- && $user->getOption( 'flow-topiclist-sortby' ) !=
$sortByOption
+ && $user->getOption( 'flow-topiclist-sortby' ) !=
$findOptions['sortby']
) {
- $user->setOption( 'flow-topiclist-sortby',
$sortByOption );
+ $user->setOption( 'flow-topiclist-sortby',
$findOptions['sortby'] );
$user->saveSettings();
- }
-
- if ( $sortByOption === 'updated' ) {
- $findOptions = array(
- // TODO: Why is this only set for 'updated'
order? Is it redundant
- // to storage.topic_list.indexes?
- 'sort' => 'workflow_last_update_timestamp',
- 'order' => 'desc',
- // keep sortby so it can be used later for
building links
- 'sortby' => 'updated',
- ) + $findOptions;
}
return $findOptions;
diff --git a/tests/phpunit/api/ApiFlowViewTopicListTest.php
b/tests/phpunit/api/ApiFlowViewTopicListTest.php
index 7f8ef7a..53bb8f1 100644
--- a/tests/phpunit/api/ApiFlowViewTopicListTest.php
+++ b/tests/phpunit/api/ApiFlowViewTopicListTest.php
@@ -121,6 +121,7 @@
'topiclist_offset-dir' => 'fwd',
'topiclist_limit' => '2',
'topiclist_offset-id' => $topicData[1]['id'],
+
'topiclist_sortby' => 'newest',
) ),
'title' => 'fwd',
),
--
To view, visit https://gerrit.wikimedia.org/r/183985
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I627e769bdf733bdea13a04ef6ed23ec590af2499
Gerrit-PatchSet: 1
Gerrit-Project: mediawiki/extensions/Flow
Gerrit-Branch: master
Gerrit-Owner: EBernhardson <[email protected]>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
