Moodle
  1. Moodle
  2. MDL-30997

group API, check and update DocBlock

    Details

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

      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.

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

        Issue Links

          Activity

          Hide
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          Eloy Lafuente (stronk7) added a comment -

          This issue is missing the repository url.

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

          ping!

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

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

          Show
          Adrian Greeve added a comment - Sorry. I don't know how I missed your last message. I've added the git repository
          Hide
          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
          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
          Adrian Greeve added a comment -

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

          Show
          Adrian Greeve added a comment - Thanks for the smurf file. I've fixed those problems. Submitting for integration.
          Hide
          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
          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
          Eloy Lafuente (stronk7) added a comment -

          Nobody tested this

          Show
          Eloy Lafuente (stronk7) added a comment - Nobody tested this
          Hide
          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
          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: