[MDL-38440] WebService get_contents doesn't return the full text of labels - Moodle Tracker

Details

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

Testing Instructions:

Hide

phpunit course/tests/externallib_test.php
phpunit mod/label/tests/generator_test.php

Show
phpunit course/tests/externallib_test.php phpunit mod/label/tests/generator_test.php
Affected Branches:
MOODLE_24_STABLE
Fixed Branches:
MOODLE_24_STABLE
Pull from Repository:
git://github.com/mouneyrac/moodle.git
Pull Master Branch:
~~MDL-38440~~-master
Pull Master Diff URL:
https://github.com/mouneyrac/moodle/compare/moodle:master...MDL-38440-master

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

Gliffy Diagrams

Issue Links

will help resolve

MOBILE-258 Content page: Labels are not fully displayed (missing html, images, content, etc...)

Resolved

Activity

Ascending order - Click to sort in descending order

Hide

Permalink

Jérôme Mouneyrac added a comment - 20/Mar/13 4:54 AM - 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 - 20/Mar/13 4:54 AM - 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 - 20/Mar/13 5:00 AM

Sending to Juan for quick peer-review.

Show

Jérôme Mouneyrac added a comment - 20/Mar/13 5:00 AM Sending to Juan for quick peer-review.

Hide

Permalink

Jérôme Mouneyrac added a comment - 27/Mar/13 1:19 AM

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 - 27/Mar/13 1:19 AM 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 - 27/Mar/13 1:54 AM

Reopening so this issue can have a peer review.

Show

Damyon Wiese added a comment - 27/Mar/13 1:54 AM Reopening so this issue can have a peer review.

Hide

Permalink

Damyon Wiese added a comment - 27/Mar/13 1:56 AM

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

Show

Damyon Wiese added a comment - 27/Mar/13 1:56 AM (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 - 27/Mar/13 1:59 AM

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 - 27/Mar/13 1:59 AM 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 - 02/Apr/13 7:13 AM

Sending to peer-review

Show

Jérôme Mouneyrac added a comment - 02/Apr/13 7:13 AM Sending to peer-review

Hide

Permalink

Ankit Agarwal added a comment - 24/Apr/13 9:33 AM - 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

Show

Ankit Agarwal added a comment - 24/Apr/13 9:33 AM - 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 - 26/Apr/13 7:23 AM

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

Show

Jérôme Mouneyrac added a comment - 26/Apr/13 7:23 AM Thanks Ankit for the reviews, I made all the requested changes. Submitting to integration.

Hide

Permalink

Dan Poltawski added a comment - 26/Apr/13 10:20 AM

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 - 26/Apr/13 10:20 AM 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 - 29/Apr/13 3:31 AM

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 - 29/Apr/13 3:31 AM 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 - 29/Apr/13 3:32 AM

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

Show

Damyon Wiese added a comment - 29/Apr/13 3:32 AM Unit tests passed for me in integration on 24 and master.

Hide

Permalink

Dan Poltawski added a comment - 02/May/13 2:28 PM

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 - 02/May/13 2:28 PM 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

Assignee:

Jérôme Mouneyrac

Reporter:

Juan Leyva

Peer reviewer:

Ankit Agarwal

Integrator:

Damyon Wiese

Tester:

Damyon Wiese

Participants:

Ankit Agarwal, Damyon Wiese, Dan Poltawski, Jérôme Mouneyrac, Juan Leyva

Votes:

0 Vote for this issue

Watchers:

4 Start watching this issue

Dates

Created:

12/Mar/13 9:07 AM

Updated:

02/May/13 2:28 PM

Resolved:

02/May/13 2:28 PM

Agile

View on Board