Moodle
  1. Moodle
  2. MDL-38440

WebService get_contents doesn't return the full text of labels

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.4.3
    • Fix Version/s: 2.4.4
    • Component/s: Web Services
    • Labels:
    • Rank:
      48377

      Description

      It seems that get_contents relay in get_fastmodinfo functions for getting info about the modules.

      This function doesn't return the full text of a label, it only returns the first 50 characters in the field "module name".

      I think that get_contents WS should handle a special case for labels and returning the full label text inside the field description

      See MOBILE-258 for more info

        Issue Links

          Activity

          Hide
          Jérôme Mouneyrac added a comment - - edited

          What I did:

          • label also return the label description
          • the activity description is formatted as it should be in external function (bug fix)
          • I added a datagenerator for label
          • I added a label to the PHPunit (get_course_content doesn't test any fields it should be testing, if you want you can create an issue for that)
          Show
          Jérôme Mouneyrac added a comment - - edited What I did: label also return the label description the activity description is formatted as it should be in external function (bug fix) I added a datagenerator for label I added a label to the PHPunit (get_course_content doesn't test any fields it should be testing, if you want you can create an issue for that)
          Hide
          Jérôme Mouneyrac added a comment -

          Sending to Juan for quick peer-review.

          Show
          Jérôme Mouneyrac added a comment - Sending to Juan for quick peer-review.
          Hide
          Jérôme Mouneyrac added a comment -

          I pushed for integration, then I remember it's a bug fix so it can be sent after code freeze. You can reassign to peer-review. Thanks.

          Show
          Jérôme Mouneyrac added a comment - I pushed for integration, then I remember it's a bug fix so it can be sent after code freeze. You can reassign to peer-review. Thanks.
          Hide
          Damyon Wiese added a comment -

          Reopening so this issue can have a peer review.

          Show
          Damyon Wiese added a comment - Reopening so this issue can have a peer review.
          Hide
          Damyon Wiese added a comment -

          (Sorry for the noise - I forgot to reopen from the in review state for ci counters).

          Show
          Damyon Wiese added a comment - (Sorry for the noise - I forgot to reopen from the in review state for ci counters).
          Hide
          Jérôme Mouneyrac added a comment -

          Thanks for reopening. I noticed a issue btw, the third param of external_format_text should not be 'label' all the time. Taking back from peer-review status.

          Show
          Jérôme Mouneyrac added a comment - Thanks for reopening. I noticed a issue btw, the third param of external_format_text should not be 'label' all the time. Taking back from peer-review status.
          Hide
          Jérôme Mouneyrac added a comment -

          Sending to peer-review

          Show
          Jérôme Mouneyrac added a comment - Sending to peer-review
          Hide
          Ankit Agarwal added a comment - - edited

          Hello Jerome,
          I noticed a few things, that you might want to consider before pushing this further:-

          1. Something I noticed straight away is that there is no "generator_tests" to tests the generator you wrote. Afaik all new generators must have associated phpunit test to test themselef . (for ex:- data/tests/generator_test.php) .
          2. Please add some testing instructions
          3. Am not sure why we are fetching course_modules and module context in test_get_course_contents() ? If it is just for validation, than shouldn't we just trust the data generators?
          4. Files are stored in 'intro' not 'description' filearea
          5. $modules['description'] is not really consistent with moodle design imho as we call it "intro" everywhere. Anyway nothing can be done about it.

          Thanks

          Show
          Ankit Agarwal added a comment - - edited Hello Jerome, I noticed a few things, that you might want to consider before pushing this further:- Something I noticed straight away is that there is no "generator_tests" to tests the generator you wrote. Afaik all new generators must have associated phpunit test to test themselef . (for ex:- data/tests/generator_test.php) . Please add some testing instructions Am not sure why we are fetching course_modules and module context in test_get_course_contents() ? If it is just for validation, than shouldn't we just trust the data generators? Files are stored in 'intro' not 'description' filearea $modules ['description'] is not really consistent with moodle design imho as we call it "intro" everywhere. Anyway nothing can be done about it. Thanks
          Hide
          Jérôme Mouneyrac added a comment -

          Thanks Ankit for the reviews, I made all the requested changes. Submitting to integration.

          Show
          Jérôme Mouneyrac added a comment - Thanks Ankit for the reviews, I made all the requested changes. Submitting to integration.
          Hide
          Dan Poltawski added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          Dan Poltawski added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Hide
          Damyon Wiese added a comment -

          Thanks Jerome, I ran the unit tests on both branches in integration, so I'll pass the test for this.

          Thanks for adding the extra unit tests.

          Show
          Damyon Wiese added a comment - Thanks Jerome, I ran the unit tests on both branches in integration, so I'll pass the test for this. Thanks for adding the extra unit tests.
          Hide
          Damyon Wiese added a comment -

          Unit tests passed for me in integration on 24 and master.

          Show
          Damyon Wiese added a comment - Unit tests passed for me in integration on 24 and master.
          Hide
          Dan Poltawski added a comment -

          Thanks! You're changes are now spread to the world through this git and our source control repositories.

          No time to rest though, we've got days to make 2.5 the best yet!

          ciao

          Show
          Dan Poltawski added a comment - Thanks! You're changes are now spread to the world through this git and our source control repositories. No time to rest though, we've got days to make 2.5 the best yet! ciao

            People

            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: