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

group API, check and update DocBlock

    Details

    • Type: Task
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.2, 2.2.1
    • Fix Version/s: 2.3
    • Component/s: Documentation, Groups
    • Labels:

      Description

      Check and update documentation, so that it should comply with moodle coding guidelines.
      Following needs to be updated/checked for group api

      1. DocBlock for page and functions.
      2. All the files should be checked/updated.

      Note: You can create sub-tasks, so as to avoid bulk integration.

        Gliffy Diagrams

        1. smurf_MDL-30997.xml
          3 kB
          Eloy Lafuente (stronk7)

          Issue Links

            Activity

            Hide
            dougiamas Martin Dougiamas added a comment - - edited

            OK, quite a few issues here and with http://docs.moodle.org/dev/Groups_API

            1. The main API for plugins (actually only activity modules) is lib/grouplib.php. That's why it's separate from the /group directory.

            eg
            function groups_group_exists($groupid)
            function groups_get_group_name($groupid)
            function groups_get_grouping_name($groupingid)
            function groups_get_group_by_name($courseid, $name)
            function groups_get_grouping_by_name($courseid, $name)
            function groups_get_group($groupid, $fields='*', $strictness=IGNORE_MISSING)
            function groups_get_grouping($groupingid, $fields='*', $strictness=IGNORE_MISSING)
            function groups_get_all_groups($courseid, $userid=0, $groupingid=0, $fields='g.*')
            function groups_get_user_groups($courseid, $userid=0)
            function groups_get_all_groupings($courseid)
            function groups_is_member($groupid, $userid=null)
            function groups_has_membership($cm, $userid=null)
            function groups_get_members($groupid, $fields='u.*', $sort='lastname ASC')
            function groups_get_grouping_members($groupingid, $fields='u.*', $sort='lastname ASC')
            function groups_get_course_groupmode($course)
            function groups_get_activity_groupmode($cm, $course=null)
            function groups_print_course_menu($course, $urlroot, $return=false)
            function groups_print_activity_menu($cm, $urlroot, $return=false, $hideallparticipants=false)
            function groups_get_course_group($course, $update=false, $allowedgroups=null)
            function groups_get_activity_group($cm, $update=false, $allowedgroups=null)
            function groups_get_activity_allowed_groups($cm,$userid=0)
            function groups_course_module_visible($cm, $userid=null)

            Grep forum code for group_ to see how they are used in the forum module. That's all the API document needs to talk about. (creating/managing groups is done elsewhere by other code)

            So only those need "@category group" (Note, not "groups")

            2. The package for all the group files in /group and /lib/grouplib.php is "core_group". Not "group_group".

            http://docs.moodle.org/dev/Frankenstyle#Core_subsystems

            Speak up if you need any help.

            Show
            dougiamas Martin Dougiamas added a comment - - edited OK, quite a few issues here and with http://docs.moodle.org/dev/Groups_API 1. The main API for plugins (actually only activity modules) is lib/grouplib.php. That's why it's separate from the /group directory. eg function groups_group_exists($groupid) function groups_get_group_name($groupid) function groups_get_grouping_name($groupingid) function groups_get_group_by_name($courseid, $name) function groups_get_grouping_by_name($courseid, $name) function groups_get_group($groupid, $fields='*', $strictness=IGNORE_MISSING) function groups_get_grouping($groupingid, $fields='*', $strictness=IGNORE_MISSING) function groups_get_all_groups($courseid, $userid=0, $groupingid=0, $fields='g.*') function groups_get_user_groups($courseid, $userid=0) function groups_get_all_groupings($courseid) function groups_is_member($groupid, $userid=null) function groups_has_membership($cm, $userid=null) function groups_get_members($groupid, $fields='u.*', $sort='lastname ASC') function groups_get_grouping_members($groupingid, $fields='u.*', $sort='lastname ASC') function groups_get_course_groupmode($course) function groups_get_activity_groupmode($cm, $course=null) function groups_print_course_menu($course, $urlroot, $return=false) function groups_print_activity_menu($cm, $urlroot, $return=false, $hideallparticipants=false) function groups_get_course_group($course, $update=false, $allowedgroups=null) function groups_get_activity_group($cm, $update=false, $allowedgroups=null) function groups_get_activity_allowed_groups($cm,$userid=0) function groups_course_module_visible($cm, $userid=null) Grep forum code for group_ to see how they are used in the forum module. That's all the API document needs to talk about. (creating/managing groups is done elsewhere by other code) So only those need "@category group" (Note, not "groups") 2. The package for all the group files in /group and /lib/grouplib.php is "core_group". Not "group_group". http://docs.moodle.org/dev/Frankenstyle#Core_subsystems Speak up if you need any help.
            Hide
            timhunt Tim Hunt added a comment -

            I wouldn't just look at forum. There were some recent, tricky, group-related bug-fixes in the quiz reports. Have a look at MDL-27183 / https://github.com/moodle/moodle/commit/e4977ba5c699e642cb53207b5f84fe6522245e57 as one example of the kind of things you need to help people understand. (I am not sure I fully understand it yet).

            Show
            timhunt Tim Hunt added a comment - I wouldn't just look at forum. There were some recent, tricky, group-related bug-fixes in the quiz reports. Have a look at MDL-27183 / https://github.com/moodle/moodle/commit/e4977ba5c699e642cb53207b5f84fe6522245e57 as one example of the kind of things you need to help people understand. (I am not sure I fully understand it yet).
            Hide
            abgreeve Adrian Greeve added a comment -

            I've made the alteration suggested to the example code and grouped the functions. Though I'm still not entirely happy with the way that looks at the moment. Do we need the function section at all? Could it be removed?

            Show
            abgreeve Adrian Greeve added a comment - I've made the alteration suggested to the example code and grouped the functions. Though I'm still not entirely happy with the way that looks at the moment. Do we need the function section at all? Could it be removed?
            Hide
            andyjdavis Andrew Davis added a comment -

            Some very minor points:

            group/members.php
            "@copyright © 2006 The Open University and others, N.D.Freear AT open.ac.uk, J.White AT open.ac.uk and others"
            Probably want to remove the ©

            group/tabs.php
            Expand the file description a little to say what these tabs are for. "Prints navigation tabs" or something.

            Show
            andyjdavis Andrew Davis added a comment - Some very minor points: group/members.php "@copyright © 2006 The Open University and others, N.D.Freear AT open.ac.uk, J.White AT open.ac.uk and others" Probably want to remove the © group/tabs.php Expand the file description a little to say what these tabs are for. "Prints navigation tabs" or something.
            Hide
            abgreeve Adrian Greeve added a comment -

            Thanks for picking that up Andrew I've now fixed that up. I found a few files that had © in the copyright section.

            Show
            abgreeve Adrian Greeve added a comment - Thanks for picking that up Andrew I've now fixed that up. I found a few files that had © in the copyright section.
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Hi Adrian,

            Sending this back so that a couple of things can be fixed up:

            1. groups_get_group $strictness is an int not a string, return is a stdClass not an object (same goes for several other function)
            2. groups_has_membership $cm is using object, likely it is an instance of cm_info
            3. groups_get_grouping_members $fields is a string, as is $sort
            4. groups_get_activity_groupmode likely $cm is a cm_info object (or stdClass) and $course is stdClass
            5. Please check other uses of cm while looking at that
            6. typo in description of file docblock for group/autogroup.php
            7. autogroup_form needs proper docblock as does its definition method
            8. Incorrect @package used in clientlib.js (we arn't docing JS as part of this task from what I understand however we should tidy that up seeing as this is going back anyway)
            9. phpdoc block for class core_group_external missing copyright and license tags
            10. incorrect return for core_group_external::create_groups_returns
            11. same as above for get_groups_returns and other return functions (all return functions perhaps?)
            12. moodle_group_external missing license and copyright
            13. group_form class needs proper docblock (definition needs proper docblock too)
            14. Actually all of that classes methods need proper doc blocks
            15. import_form.php needs phpdocs for class + methods

            On a general note @param object $whatever is nearly always incorrect.
            We deprecated the object class in favour of stdClass in Moodle 2.0 (check lib/setuplib.php).
            In most cases it is a stdClass. Sometimes it is something else, $context is usually a context object, $rs normally is a moodle_recordset. etc

            Cheers
            Sam

            Show
            samhemelryk Sam Hemelryk added a comment - Hi Adrian, Sending this back so that a couple of things can be fixed up: groups_get_group $strictness is an int not a string, return is a stdClass not an object (same goes for several other function) groups_has_membership $cm is using object, likely it is an instance of cm_info groups_get_grouping_members $fields is a string, as is $sort groups_get_activity_groupmode likely $cm is a cm_info object (or stdClass) and $course is stdClass Please check other uses of cm while looking at that typo in description of file docblock for group/autogroup.php autogroup_form needs proper docblock as does its definition method Incorrect @package used in clientlib.js (we arn't docing JS as part of this task from what I understand however we should tidy that up seeing as this is going back anyway) phpdoc block for class core_group_external missing copyright and license tags incorrect return for core_group_external::create_groups_returns same as above for get_groups_returns and other return functions (all return functions perhaps?) moodle_group_external missing license and copyright group_form class needs proper docblock (definition needs proper docblock too) Actually all of that classes methods need proper doc blocks import_form.php needs phpdocs for class + methods On a general note @param object $whatever is nearly always incorrect. We deprecated the object class in favour of stdClass in Moodle 2.0 (check lib/setuplib.php). In most cases it is a stdClass. Sometimes it is something else, $context is usually a context object, $rs normally is a moodle_recordset. etc Cheers Sam
            Hide
            abgreeve Adrian Greeve added a comment -

            Thanks Sam for the feedback.
            I went through and made those changes. It should now be closer to what is expected.

            Show
            abgreeve Adrian Greeve added a comment - Thanks Sam for the feedback. I went through and made those changes. It should now be closer to what is expected.
            Hide
            stronk7 Eloy Lafuente (stronk7) 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
            stronk7 Eloy Lafuente (stronk7) 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
            stronk7 Eloy Lafuente (stronk7) added a comment -

            This issue is missing the repository url.

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - This issue is missing the repository url.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            ping!

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - ping!
            Hide
            abgreeve Adrian Greeve added a comment -

            Sorry. I don't know how I missed your last message. I've added the git repository

            Show
            abgreeve Adrian Greeve added a comment - Sorry. I don't know how I missed your last message. I've added the git repository
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Hi Adrian,

            I'm reopening this because some problems were found in the patch (smurf xml file attached for details):

            • whitespace
            • indent
            • alignment of asterisks
            • one function missing some param in phpdocs.

            Should be easy to fix, send it again to integration once ready, thank!

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Hi Adrian, I'm reopening this because some problems were found in the patch (smurf xml file attached for details): whitespace indent alignment of asterisks one function missing some param in phpdocs. Should be easy to fix, send it again to integration once ready, thank!
            Hide
            abgreeve Adrian Greeve added a comment -

            Thanks for the smurf file. I've fixed those problems. Submitting for integration.

            Show
            abgreeve Adrian Greeve added a comment - Thanks for the smurf file. I've fixed those problems. Submitting for integration.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Integrated, thanks (master only).

            I've added one extra commit fixing trailing whitespace and the indent of some comments in group/externallib.php

            Ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Integrated, thanks (master only). I've added one extra commit fixing trailing whitespace and the indent of some comments in group/externallib.php Ciao
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Nobody tested this

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Nobody tested this
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Some changes to Moodle should be milestones in the project by themselves.

            This is not the case and your fix is not so important, but your collaboration is highly appreciated, thanks!

            Closing as fixed, ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Some changes to Moodle should be milestones in the project by themselves. This is not the case and your fix is not so important, but your collaboration is highly appreciated, thanks! Closing as fixed, ciao

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  25/Jun/12