Moodle

WebService get_contents doesn't return the full text of labels

Details

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

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

will help resolve

Improvement - An enhancement to an existing Moodle feature. MOBILE-258 Content page: Labels are not fully displayed (missing html, images, content, etc...)

  • Minor - Minor loss of function where workaround is possible
  • Resolved - A resolution has been taken, and it is awaiting verification by reporter. From here issues are either reopened, or are closed.

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
Jérôme Mouneyrac added a comment -

Sending to peer-review

Show
Jérôme Mouneyrac added a comment - Sending to peer-review
Hide
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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

Vote (0)
Watch (4)

Dates

  • Created:
    Updated:
    Resolved: