Decimal separator and decimals settings ignored when aggregating decimal fields

Created on 29 May 2016, almost 9 years ago
Updated 26 June 2023, almost 2 years ago

Problem/Motivation

Have view with a decimal field, say A.
Choosing comma as decimal separator.
Choosing 2 in Scale selector (number of digits to the right of the separator).
Result without aggregation correct. All values display with 2 decimals and comma as decimal point.

With aggregation the sum of A display with decimal point and 14 digits left of the separator.
Seams that the up-summing totally ignore the option I chose for the field A in views field selector.

Steps to reproduce

  1. Add a Decimal field, multi value, to a content type
  2. Add a node of that content type, with two values in the decimal field
  3. Create a view for that content type
  4. Enable aggregation
  5. Add the Decimal field to the view and set the Decimal marker to 'Comma' .
  6. In the Aggregation settings, set the Aggregation type to 'Sum'

Proposed resolution

Revert accidental commit of field handlers on aggregation field settings within get_aggregation_info() from this commit:
https://git.drupalcode.org/project/drupal/-/commit/684b4a036e736b16d21a1...

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

🐛 Bug report
Status

Needs work

Version

11.0 🔥

Component
Views 

Last updated 1 day ago

Created by

🇸🇪Sweden chrotto

Live updates comments and jobs are added and updated live.
  • PHP 8.1

    The issue particularly affects sites running on PHP version 8.1.0 or later.

  • Needs screenshots

    The change alters the user interface, so before and after screenshots should be added to document the UI change. Make sure to capture the relevant region only. Use a tool such as Aviary on Windows or Skitch on Mac OS X.

  • Needs tests

    The change is currently missing an automated test that fails when run with the original code, and succeeds when the bug has been fixed.

Sign in to follow issues

Merge Requests

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • 🇩🇰Denmark ressa Copenhagen

    Thanks for sharing a pragmatic Twig solution in #16 🐛 Decimal separator and decimals settings ignored when aggregating decimal fields Needs work @Ben Greenberg. Drupal 10 is now on Twig 3 , so the URL is https://twig.symfony.com/doc/3.x/filters/number_format.html, but the syntax is the same.

  • 🇨🇦Canada joelpittet Vancouver

    @akram khan Thanks for the patch, could you explain the changes you made so we are all clear as your reroll added more removals than @ramil g did.

  • 🇨🇦Canada joelpittet Vancouver

    We have various screenshots on this issue, so removing the PHP related issue tag and needs screenshots.

  • Merge request !10837test UI of field aggregation settings → (Closed) created by ramil g
  • Pipeline finished with Failed
    3 months ago
    Total: 388s
    #388963
  • 🇨🇦Canada joelpittet Vancouver
  • Pipeline finished with Success
    3 months ago
    Total: 2194s
    #389011
  • 🇨🇦Canada joelpittet Vancouver

    Thanks @ramil g. The test looks great and it's red/green on test-only/with fix. 🚀 it!

  • 🇨🇦Canada joelpittet Vancouver

    Moving priority due to the PHP error that is triggered
    https://www.drupal.org/docs/develop/issues/fields-and-other-parts-of-an-...

    Trigger a PHP error through the user interface, but only under rare circumstances or affecting only a small percentage of all users, even if there is a workaround.

  • 🇳🇿New Zealand quietone

    I tested on Drupal 11.x today, standard install and reproduce the problem. Applied the diff and the problem was fixed. Updated credit.

    There are no screenshots in the issue summary. Ah, I see they are in #44. @asad_ahmed, when adding screenshots it helps everyone working on the issue if they are available from the issue summary. This save reviewers and committers from hunting through the comments to find the correct images.

    Can someone explain why the test is not testing the scenario in the Issue Summary? The test seems to be testing a different problem. It is a problem I encountered when testing this. The problem was that I could not change the aggregate settings from 'Sum' to anything else. Settings to NW to understand the testings.

  • 🇨🇦Canada joelpittet Vancouver

    @quietone I don't believe the screenshots in #44 accurately display the root problem for why the decimal settings change between aggregation being on and off.

    I added the screenshot from #38 which identifies where this stems from to the issue summary, which displays the accidental copy/paste error in https://git.drupalcode.org/project/drupal/-/commit/684b4a036e736b16d21a1..., which manifests itself into the originally reported issue as well as other Views UI issues including the error I mentioned in #56 when I bumped the priority and which @ramil g wrote the tests against.

    Also I added a bunch more screenshots to the IS illustrating both problems.

    One thing I don't know how to change the title to indicate the original issue is resolved + issues with any numeric aggregated field? Any suggestions?

  • Pipeline finished with Canceled
    about 2 months ago
    Total: 78s
    #417098
  • Pipeline finished with Failed
    about 2 months ago
    Total: 517s
    #417099
  • 🇨🇦Canada joelpittet Vancouver

    LMK if there’s anything I can do to get this patch committed. It should be a slam dunk, but I may be missing something that’s holding it up.

  • Status changed to RTBC 19 days ago
  • 🇨🇦Canada joelpittet Vancouver

    To reiterate, this issue was introduced accidentally due to a copy/paste error, and it has been there for quite some time.
    https://git.drupalcode.org/project/drupal/-/commit/684b4a036e736b16d21a1...

    • catch committed 59d79456 on 10.4.x
      Issue #2735997 by ramil g, joelpittet, ranjith_kumar_k_u, asad_ahmed,...
    • catch committed e2461026 on 10.5.x
      Issue #2735997 by ramil g, joelpittet, ranjith_kumar_k_u, asad_ahmed,...
    • catch committed 5e30c259 on 11.1.x
      Issue #2735997 by ramil g, joelpittet, ranjith_kumar_k_u, asad_ahmed,...
    • catch committed d15e2ea5 on 11.x
      Issue #2735997 by ramil g, joelpittet, ranjith_kumar_k_u, asad_ahmed,...
  • 🇬🇧United Kingdom catch

    @joelpittet I don't think there was anything in particular holding it up, but I think it's a victim of the following:

    1. The RTBC queue has been constantly fluctuating between 75-150 for several months, it feels impossible to get it under 50, so it's easy for individual issues to slip through when the list is very long. This is despite ~150 commits to 11.x in the past month and however many other issues moved to needs work in the same period.

    2. It was RTBC and un-RTBCed a couple of times, that meant it wasn't the oldest issue in the RTBC queue until this week, even though it was more-or-less RTBC for a lot longer.

    This is the search I use when trying to approach the RTBC queue FIFO:
    https://www.drupal.org/project/issues/search/drupal?text=&assigned=&subm...

    The 'status changed' column is usually a good indicator, but it's not perfect - because it counts from the most recent status change, not the first change to that status. On the other hand, there are issues that were first RTBCed five years ago and have 100 comments since because they were never really RTBC, so 'first RTBC' would also not be accurate for different issues.

    3. The title made the issue look a lot more complex than it was, so at least for me I expected to be looking at it for at least an hour before I'd feel comfortable committing it. It can take longer to get to those issues. Obviously a quick scan of the MR would have indicated it's pretty straightforward but you have to look at them first, and that goes back to point #1 and #2.

    None of these are good reasons for an issue to get held up, but I think they probably are the reasons.

    Committed/pushed to 11.x and cherry-picked to 11.1.x, 10.5.x, and 10.4.x, thanks!

  • 🇨🇦Canada joelpittet Vancouver

    Thanks for the context, @catch—much appreciated! I was pretty sure it would get in eventually, just didn’t want it to be one of those “not really RTBC” cases.

    I’m really glad this bug is finally squashed—I kept running into it with aggregation enabled on all my migrations!

Production build 0.71.5 2024