"Some" Pager Plugin and [view:total-rows] Regression Issue

Created on 16 October 2024, 11 months ago

Problem/Motivation

The solution to ๐Ÿ› [view:total-rows] problem in Display a 'Specified number of items' pager Postponed: needs info introduced the following regression related issues:

  1. ๐Ÿ› More link is missing in pager when using the "Some" pager and there are more records than shown Needs work
  2. ๐Ÿ› More link disappears when time-based views cache is enabled Needs work

Before these issues can be fixed, I believe a decision needs to be made as to what exactly the [view:total-rows] token should return based on the pager selected and other display configurations, as this will affect how these issues are solved.

The description of the [view:total-rows] token is The total amount of results returned from the view. The current display will be used..

๐Ÿ› [view:total-rows] problem in Display a 'Specified number of items' pager Postponed: needs info interpreted that to be the total number of results returned from the query which can be limited by the pager. So if I am using the Some pager and only displaying 5 results when there are say 20, [view:total-rows] should only return 5.

But what if I have a "More link" enabled and I want to add text to the View header that says "Displaying 5 of 20". I believe this is a valid need but can no longer be done because of the changes in ๐Ÿ› [view:total-rows] problem in Display a 'Specified number of items' pager Postponed: needs info .

So if a "More link" is enabled should [view:total-rows] return the total number of results ignoring any limits applied by the "Some" pager, which in the above example be 20? Or should it always the number of results of the "Some" pager limited query, which is the current functionality?

Proposed resolution

I propose the [view:total-rows] token return the total number of results ignoring any limits applied by the "Some" pager if the "More link" is enabled.

I believe this would result in an easier fix to the related regression issues, and possibly make more sense given what is being displayed by the View. If this is the approach pursued, then I think the [view:total-rows] token description needs to be updated to provide more information on what if can be affected by.

Remaining tasks

  1. Determine how the value returned from [view:total-rows] token should be affected by the "Some" pager and More link.
  2. Based on the decision of #1 determine other remaining tasks i.e. token description update, tests, new token.
๐ŸŒฑ Plan
Status

Active

Version

11.0 ๐Ÿ”ฅ

Component

views.module

Created by

๐Ÿ‡จ๐Ÿ‡ฆCanada tame4tex

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @tame4tex
  • ๐Ÿ‡จ๐Ÿ‡ฆCanada tame4tex
  • ๐Ÿ‡จ๐Ÿ‡ฆCanada tame4tex

    Given there wasn't a lot of work involved, and to hopefully speed things along, I have created a MR with the proposed resolution.

    Even though this should fix the related issues, it doesn't automatically close them as testing should be added for the specific bug experienced, ie visibility of the More link and how caching effects this visibility.

  • Pipeline finished with Success
    11 months ago
    Total: 1469s
    #311977
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States smustgrave

    Believe there is also BC concerns to be had here as well. As this could immediately impact existing sites.

  • ๐Ÿ‡จ๐Ÿ‡ฆCanada tame4tex

    BC is definitely a consideration but I would consider it less so compared to the regression issues resulting from the change.

    It would only impact sites that started using the [view:total-rows] token from version 10.1.0 because prior to that version it returned the total number of items before the pager limit was applied.

    Similar to the token issue, we have a site where the usage of Displaying @current_record_count of @total on a block using the "Some" pager is now broken due to this change.

    This is what it looked like from D8 all the way to D10.1.0

    This is how it looks now

    On further thought, on reading the issue summary of ๐Ÿ› [view:total-rows] problem in Display a 'Specified number of items' pager Postponed: needs info again, using @current_record_count in their header rather than [view:total-rows] would have solved their problem and there would have been no need for the code change.

    So on further thought maybe the changes in ๐Ÿ› [view:total-rows] problem in Display a 'Specified number of items' pager Postponed: needs info should be rolled back and we instead update the description of [view:total-rows] token to explain it is the total number of items before any limiting by the page is applied.

    Right now I am just proposing options, hoping to get feedback on preferred approach.

  • ๐Ÿ‡จ๐Ÿ‡ฆCanada tame4tex

    I have updated the Issue summary with the additional regression issue and the two possible resolutions I have thought of so far.

    I have also added a MR for Option #1 which is now my preferred.

  • Pipeline finished with Success
    11 months ago
    Total: 2026s
    #314256
  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia KumudB Ahmedabad

    I have verified your changes, and its working as expected on Drupal version 11.x, here I attached the screen recording and screen shot.
    Earlier it was not displaying "more" link when specified items are displaying.

    Now it is working as expected.

    Description also Updated

  • ๐Ÿ‡จ๐Ÿ‡ฆCanada tame4tex

    Updated title to also include Result Summary @total token.

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States smustgrave

    Do not have an answer for the remaining tasks but the MR appears to need a manual rebase.

  • +1 for Option 1. I concur that the original fix in ๐Ÿ› [view:total-rows] problem in Display a 'Specified number of items' pager Postponed: needs info was flawed and seems to be tailored to a pretty rare use-case, where a Result Summary with @current_record_count could have simply been used. That change broke all the views on our site that use the pattern described in comment #6 โ†’ with a summary "displaying X of X" and a link to view all results. So while this change would break some backwards compatibility, it's also fixing backwards compatibility...

  • Status changed to Needs work 8 months ago
  • ๐Ÿ‡ณ๐Ÿ‡ฑNetherlands johnv
  • ๐Ÿ‡จ๐Ÿ‡ฆCanada harika gujjula

    harika gujjula โ†’ made their first commit to this issueโ€™s fork.

  • Pipeline finished with Failed
    7 months ago
    Total: 419s
    #434322
  • Pipeline finished with Success
    about 1 month ago
    Total: 594s
    #574053
  • Pipeline finished with Success
    about 1 month ago
    #574069
  • Status changed to Needs review about 1 month ago
  • ๐Ÿ‡จ๐Ÿ‡ฆCanada tame4tex

    Both proposed solution MR have been rebased and conflicts addressed. Green and back to NR.

  • Pipeline finished with Success
    about 1 month ago
    Total: 920s
    #574091
Production build 0.71.5 2024