Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-73941

Review reportbuilder and ensure that all floats are using current lang decsep

    XMLWordPrintable

Details

    • Bug
    • Status: Closed
    • Major
    • Resolution: Fixed
    • 4.0
    • 4.0
    • Report builder
    • MOODLE_400_STABLE
    • MOODLE_400_STABLE
    • Hide

      Covered by automated tests

      To test manually:

      1. Install/switch to French (fr) language pack
      2. Create new report from Users report source, containing following columns
        • Lastname (Nom)
        • Confirmed (Confirmé)
      3. Change aggregation method of the Confirmed column to
        • Average (Moyenne)
        • Percent (Pourcentage)
      4. Confirm column values are formatted using localised format (comma decimal separator) for each
      Show
      Covered by automated tests To test manually: Install/switch to French (fr) language pack Create new report from Users report source, containing following columns Lastname ( Nom ) Confirmed ( Confirmé ) Change aggregation method of the Confirmed column to Average ( Moyenne ) Percent ( Pourcentage ) Confirm column values are formatted using localised format (comma decimal separator) for each

    Description

      While working on MDL-73824 it has been detected that reportbuilder not always observe current separators (decimal, thousands...) when printing floats (specifically percentages, because there was a behat test failing with decsep set to comma and, still, the test was printing dots (.).

      If you want too see how to bulk-upgrade all behat tests to run using another decsep, take a look to MDL-73824. Alternatively, it can be set for an scenario with:

      And the following "language customisations" exist:
            | component       | stringid | value |
            | core_langconfig | decsep   | ,     |
      

      The failing scenario when running all tests with comma decsep is:

      003 Example: | Percentage  | 66,7%  |                      # /Users/stronk7/git_moodle/moodle/reportbuilder/tests/behat/columnaggregationeditor.feature:95
            And I should see "66,7%" in the "Richie" "table_row" # /Users/stronk7/git_moodle/moodle/reportbuilder/tests/behat/columnaggregationeditor.feature:86
              "66,7%" text was not found in the "Richie" element (Behat\Mink\Exception\ExpectationException)
      

      And, instead of the expected 66,7% (comma), a 66.7% (dot) is output.

      In this exact case the fix is easy and I got it passing with this easy change:

      index 8a0a2971764..360d8dec4a9 100644
      --- a/reportbuilder/classes/local/helpers/format.php
      +++ b/reportbuilder/classes/local/helpers/format.php
      @@ -61,6 +61,6 @@ class format {
            * @return string
            */
           public static function percent($value): string {
      -        return sprintf('%.1f%%', (float) $value);
      +        return format_float($value) . '%';
           }
       }
      

      But, instead of fixing that as part of MDL-73824 , I think this case deserves a must-fix issue per se, so it can be reviewed if there are other places where the localised floats are not working (I've seen, at very least, another suspicious sprintf() in the code.

      So this is about:

      • Review all the places where reportbuilder can be using floats for user input or output (not for exporting files, it seems to be a consensus about always using English dots there). Fix them using PARAM_LOCALISEDFLOAT or unformat_float() for inputs and format_float() for outputs.
      • If possible, add some extra examples, also changing the decimal separator (to comma, to hash...) and verify that all the places working with floats, continue working ok.

      Ciao

      Attachments

        Issue Links

          Activity

            People

              pholden Paul Holden
              stronk7 Eloy Lafuente (stronk7)
              Carlos Escobedo Carlos Escobedo
              Eloy Lafuente (stronk7) Eloy Lafuente (stronk7)
              CiBoT CiBoT
              David Carrillo, Paul Holden
              Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

                Created:
                Updated:
                Resolved:
                19/Apr/22

                Time Tracking

                  Estimated:
                  Original Estimate - Not Specified
                  Not Specified
                  Remaining:
                  Remaining Estimate - 0 minutes
                  0m
                  Logged:
                  Time Spent - 2 hours, 45 minutes
                  2h 45m