Details

    • Rank:
      38258

      Description

      Create web service core_grade_get_definitions. This web service will return rubric grading definitions. It accepts the following parameters:

      • An array of course module ids (required parameter)
      • An areaname such as 'submissions' (required parameter)
      • Optionally set parameter activeonly to true to return only the active method

        Issue Links

          Activity

          Hide
          Jérôme Mouneyrac added a comment -

          I added Marina, it could be good to have your opinion about the web service function specification. i.e. optional parameters, missing parameters, suggested returned values, ..., it's all about opening Moodle to external application, the subject here being getting rubric grading definition.

          Show
          Jérôme Mouneyrac added a comment - I added Marina, it could be good to have your opinion about the web service function specification. i.e. optional parameters, missing parameters, suggested returned values, ..., it's all about opening Moodle to external application, the subject here being getting rubric grading definition.
          Hide
          Jérôme Mouneyrac added a comment - - edited

          After a first talk, it looks like it would be called: gradingform_rubric_export_definitions or gradingform_rubric_get_definitions

          gradingform => component name
          rubric => plugin name
          export/get_definitions => method name

          Show
          Jérôme Mouneyrac added a comment - - edited After a first talk, it looks like it would be called: gradingform_rubric_export_definitions or gradingform_rubric_get_definitions gradingform => component name rubric => plugin name export/get_definitions => method name
          Hide
          Marina Glancy added a comment -

          Also added David as a watcher.

          First thing that I don't like is the ending definitionS - I'd suggest that a function returns only one definition.

          Second, I wonder how external application can know areaid. In this case there should be another function that returns areaid.

          What we can accept as parameters:
          Module id + Graded area name (i.e. for assignments it will be "submission")
          or
          Context id + Graded area name
          or
          Area id

          In this case we can say that each advanced grading method MUST declare a function
          gradingform_XXXX_export_definition($areaid) (or ..._get_definition)

          Actually we can have another function that would return all definitions used in course/module, for all grading methods used. Hopefully in the future there will be other grading methods and other modules will support advanced grading.

          this would be

          core_gradingform_get_definitions_in_course($courseid)
          and/or
          core_gradingform_get_definitions_in_module($moduleid)

          and those functions would search for available definitions and invoke gradingform_XXXX_get_definition from appropriate plugin

          Show
          Marina Glancy added a comment - Also added David as a watcher. First thing that I don't like is the ending definitionS - I'd suggest that a function returns only one definition. Second, I wonder how external application can know areaid. In this case there should be another function that returns areaid. What we can accept as parameters: Module id + Graded area name (i.e. for assignments it will be "submission") or Context id + Graded area name or Area id In this case we can say that each advanced grading method MUST declare a function gradingform_XXXX_export_definition($areaid) (or ..._get_definition) Actually we can have another function that would return all definitions used in course/module, for all grading methods used. Hopefully in the future there will be other grading methods and other modules will support advanced grading. this would be core_gradingform_get_definitions_in_course($courseid) and/or core_gradingform_get_definitions_in_module($moduleid) and those functions would search for available definitions and invoke gradingform_XXXX_get_definition from appropriate plugin
          Hide
          Jérôme Mouneyrac added a comment -

          After talking to Marina, we thought we could implement one generic bulk function core_gradingform_get_definitions with parameters 'list of course id' and 'list of module id'. This function will basically call the plugin export functions (they don't exist yet).

          Paul would it suit your need?

          Can you also explain how you planned to use core_grade_get_rubric_grading_definitions into your client?

          Show
          Jérôme Mouneyrac added a comment - After talking to Marina, we thought we could implement one generic bulk function core_gradingform_get_definitions with parameters 'list of course id' and 'list of module id'. This function will basically call the plugin export functions (they don't exist yet). Paul would it suit your need? Can you also explain how you planned to use core_grade_get_rubric_grading_definitions into your client?
          Hide
          Paul Charsley added a comment -

          Hi Jerome, Marina,

          The Lightwork client will need to use this web service as follows:

          • Lightwork will have a list of assignments/assignment data (these are assignments that the Lightwork user is marking/managing)

          On demand Lightwork performs a synchronization with Moodle and as part of this synchronization needs to refresh the advanced grading definition information for its assignments.

          Using the generic bulk function that you propose with a list of course ids and a list of module ids would mean that we would be returned grading definitions for all the assignments of a course. This extra information would adversely impact Lightwork's synchronization.

          Would it be possible to?:

          Use a list of course_module ids instead (list of ids from table course_modules)
          OR
          Add an additional parameter which is an optional list of instance ids (allowing us to get definitions just for the assignments we are interested in)

          Note 1 - We are also adding a new grading method (the marking guide) and so I am imagining that its definitions will also be returned by this web service. See MDL-31731
          Note 2 - Our idea when adding this function was that it will return the grading form definitions. We will also need to add another web service (I'll create another sub task for this) that will return all the instances of a grading definition, ie the marks and feedback for each student/item

          Show
          Paul Charsley added a comment - Hi Jerome, Marina, The Lightwork client will need to use this web service as follows: Lightwork will have a list of assignments/assignment data (these are assignments that the Lightwork user is marking/managing) On demand Lightwork performs a synchronization with Moodle and as part of this synchronization needs to refresh the advanced grading definition information for its assignments. Using the generic bulk function that you propose with a list of course ids and a list of module ids would mean that we would be returned grading definitions for all the assignments of a course. This extra information would adversely impact Lightwork's synchronization. Would it be possible to?: Use a list of course_module ids instead (list of ids from table course_modules) OR Add an additional parameter which is an optional list of instance ids (allowing us to get definitions just for the assignments we are interested in) Note 1 - We are also adding a new grading method (the marking guide) and so I am imagining that its definitions will also be returned by this web service. See MDL-31731 Note 2 - Our idea when adding this function was that it will return the grading form definitions. We will also need to add another web service (I'll create another sub task for this) that will return all the instances of a grading definition, ie the marks and feedback for each student/item
          Hide
          Marina Glancy added a comment -

          Paul, let's say we have a function
          core_gradingform_get_definitions($courseids, $moduleids = null, $contextids = null)
          where each parameter can be array of integers, or empty
          in your case you can specify
          core_gradingform_get_definitions(null, array(123))
          it will return you all definitions used in assignment where module id=123

          I suppose that's about what Jerome had in mind. Is this ok with you?

          I'm very glad by the way that you are creating a new advanced grading method. If you have any suggestions to the API let me know, I'll be happy to discuss.

          Show
          Marina Glancy added a comment - Paul, let's say we have a function core_gradingform_get_definitions($courseids, $moduleids = null, $contextids = null) where each parameter can be array of integers, or empty in your case you can specify core_gradingform_get_definitions(null, array(123)) it will return you all definitions used in assignment where module id=123 I suppose that's about what Jerome had in mind. Is this ok with you? I'm very glad by the way that you are creating a new advanced grading method. If you have any suggestions to the API let me know, I'll be happy to discuss.
          Hide
          David Mudrak added a comment -

          Hm. That $moduleids smells. The advanced grading forms were built on the concept of contexts and areas (like files, comments or ratings in their new APIs), not course modules. Also, the suggested API does not take multiple areas per context into account.
          So, considering grading forms as belonging to a "module" or "course" is OK from user perspective. But IMHO not a good approach in terms of API.

          Show
          David Mudrak added a comment - Hm. That $moduleids smells. The advanced grading forms were built on the concept of contexts and areas (like files, comments or ratings in their new APIs), not course modules. Also, the suggested API does not take multiple areas per context into account. So, considering grading forms as belonging to a "module" or "course" is OK from user perspective. But IMHO not a good approach in terms of API.
          Hide
          Marina Glancy added a comment -

          I agree with David, contexts are more convenient and would be more correct way.
          I am just not sure that external interface knows the context ids. And if they don't they would need to make additional request to find them out. And we try to minimize the number of WS requests to increase the performance.

          Show
          Marina Glancy added a comment - I agree with David, contexts are more convenient and would be more correct way. I am just not sure that external interface knows the context ids. And if they don't they would need to make additional request to find them out. And we try to minimize the number of WS requests to increase the performance.
          Hide
          Paul Charsley added a comment -

          Hi Marina, David, Jerome,

          Having a list of context ids as a parameter would be fine for us. Prior to calling core_gradingform_get_definitions we will be invoking another web service defined in MDL-31683. This web service will have given us a list of assignments and we can include the context id for each assignment in this list.

          Show
          Paul Charsley added a comment - Hi Marina, David, Jerome, Having a list of context ids as a parameter would be fine for us. Prior to calling core_gradingform_get_definitions we will be invoking another web service defined in MDL-31683 . This web service will have given us a list of assignments and we can include the context id for each assignment in this list.
          Hide
          Jérôme Mouneyrac added a comment -

          Hi,
          till now the external functions decided what is the context type (COURSE, SYSTEM, USER...) and retrieve the context linked to the course/user/module/other id passed as parameter.

          If we go with sending contextid we need to:

          • implement a way for the client to get it - I suggest get_courses_content could send contextid of each course and module, get_courses could send contextid for each course, get_users for each user...
          • have more documentation about this way - I suggest every functions with contextid parameter should mention the function that return the contextid in its description.
          Show
          Jérôme Mouneyrac added a comment - Hi, till now the external functions decided what is the context type (COURSE, SYSTEM, USER...) and retrieve the context linked to the course/user/module/other id passed as parameter. If we go with sending contextid we need to: implement a way for the client to get it - I suggest get_courses_content could send contextid of each course and module, get_courses could send contextid for each course, get_users for each user... have more documentation about this way - I suggest every functions with contextid parameter should mention the function that return the contextid in its description.
          Hide
          Jérôme Mouneyrac added a comment - - edited

          To summarise my last comment, I'm ok with contextids as parameter.

          Show
          Jérôme Mouneyrac added a comment - - edited To summarise my last comment, I'm ok with contextids as parameter.
          Hide
          David Mudrak added a comment -

          OK. With regards to multiple areas, the call core_gradingform_get_definitions() should actually get an array of grading areas and each of these areas should either provide just the one active definition for that area, or even all present definitions with the information which of them is currently the active one.

          For the current assignment, the returned structure would contain just one element as the assignment declares just one grading area. You can refer to how backup and restore of grading forms is implemented as it addresses the same issue.

          Show
          David Mudrak added a comment - OK. With regards to multiple areas, the call core_gradingform_get_definitions() should actually get an array of grading areas and each of these areas should either provide just the one active definition for that area, or even all present definitions with the information which of them is currently the active one. For the current assignment, the returned structure would contain just one element as the assignment declares just one grading area. You can refer to how backup and restore of grading forms is implemented as it addresses the same issue.
          Hide
          Jérôme Mouneyrac added a comment -

          is that what you mean:
          core_gradingform_get_definitions

          parameters:

          • array of grading aera id
          • active aera only

          returned value:
          array of grading definitions

          Show
          Jérôme Mouneyrac added a comment - is that what you mean: core_gradingform_get_definitions parameters : array of grading aera id active aera only returned value : array of grading definitions
          Hide
          Paul Charsley added a comment -

          Hi Jerome,

          I think it should be as follows:
          core_gradingform_get_definitions

          parameters:

          • array of context ids
          • boolean active only flag

          return value:

          • array of grading areas with their definitions
          Show
          Paul Charsley added a comment - Hi Jerome, I think it should be as follows: core_gradingform_get_definitions parameters: array of context ids boolean active only flag return value: array of grading areas with their definitions
          Hide
          David Mudrak added a comment -

          Yup, +1 for Paul's suggestion

          Show
          David Mudrak added a comment - Yup, +1 for Paul's suggestion
          Hide
          Jérôme Mouneyrac added a comment -

          Cool, moving it to the Roadmap

          Show
          Jérôme Mouneyrac added a comment - Cool, moving it to the Roadmap
          Hide
          Marina Glancy added a comment - - edited

          not sure about the return. Don't we want to include information about courses/assignments where those definitions were found?

          I would suggest return the array where each element has the following fields:

          {
          courseid: 13,
          coursecontextid: 15,
          moduleid: 123,
          modulecontextid: 456,
          moduletype: 'assignment',
          areaid: 3,
          areaname: 'submission',
          gradingmethod: 'rubric',
          definition: {...}
          }
          

          another question is what we want to see in definition?
          There are two functions in grading methods API:
          1. gradingform_controller::get_definition()
          2. gradingform_controller::get_definition_copy(gradingform_controller $target)

          They both already exist and plugins overwrite them to include definitions in their own format.
          First one is great but the result contains real ids of definition, criteria and levels (in case of rubric)
          The second one prepares the definition to be used in function gradingform_controller::update_definition() but it requires target controller (which we actually can make up before calling)

          Not sure how you want to use the result.

          Show
          Marina Glancy added a comment - - edited not sure about the return. Don't we want to include information about courses/assignments where those definitions were found? I would suggest return the array where each element has the following fields: { courseid: 13, coursecontextid: 15, moduleid: 123, modulecontextid: 456, moduletype: 'assignment', areaid: 3, areaname: 'submission', gradingmethod: 'rubric', definition: {...} } another question is what we want to see in definition? There are two functions in grading methods API: 1. gradingform_controller::get_definition() 2. gradingform_controller::get_definition_copy(gradingform_controller $target) They both already exist and plugins overwrite them to include definitions in their own format. First one is great but the result contains real ids of definition, criteria and levels (in case of rubric) The second one prepares the definition to be used in function gradingform_controller::update_definition() but it requires target controller (which we actually can make up before calling) Not sure how you want to use the result.
          Hide
          Marina Glancy added a comment -

          forgot the flag whether grading method is active. And also noticed that some fields are always present in definition, no need to repeat them.

          So, another option for returned elements:

          {
          courseid: 13,
          coursecontextid: 15,
          moduleid: 123,
          contextid: 456,
          component: 'mod_assignment',
          areaname: 'submission',
          isactive: true,
          definition: {
            areaid: 3,
            method: 'rubric',
            status: 20,
            name: 'My first rubric',
            description: '...',
            ...
          }
          }
          
          Show
          Marina Glancy added a comment - forgot the flag whether grading method is active. And also noticed that some fields are always present in definition, no need to repeat them. So, another option for returned elements: { courseid: 13, coursecontextid: 15, moduleid: 123, contextid: 456, component: 'mod_assignment', areaname: 'submission', isactive: true , definition: { areaid: 3, method: 'rubric', status: 20, name: 'My first rubric', description: '...', ... } }
          Hide
          Paul Charsley added a comment -

          Hi Marina,

          A couple of points:

          • I added timemodified which is useful when checking if the definition has changed since we last got the data
          • I added criteria and levels which complete the rubric definition
          • It might be better to have the area id at the top level with its area data
          • I added our new marking guide advanced grading method
          • Also, courseid,coursecontextid,moduleid. Do we need this extra data? Isn't contextid enough?

          (
          id: 3,
          courseid: 13, ?
          coursecontextid: 15, ?
          moduleid: 123, ?
          contextid: 456,
          component: 'mod_assignment',
          areaname: 'submission',
          activemethod: rubric,
          rubricdefinition (
          method: 'rubric',
          status: 20,
          name: 'My first rubric',
          description: 'my description',
          timemodified: 13242
          criteria: - rubric criteria array containing levels array for each criterion
          )
          markingguidedefinition (
          method: 'markingguide',
          status: 20,
          name: 'My first marking guide',
          description: 'my description',
          timemodified: 13242
          criteria: - marking guide criteria ....
          )
          )

          What do you think?

          Show
          Paul Charsley added a comment - Hi Marina, A couple of points: I added timemodified which is useful when checking if the definition has changed since we last got the data I added criteria and levels which complete the rubric definition It might be better to have the area id at the top level with its area data I added our new marking guide advanced grading method Also, courseid,coursecontextid,moduleid. Do we need this extra data? Isn't contextid enough? ( id: 3, courseid: 13, ? coursecontextid: 15, ? moduleid: 123, ? contextid: 456, component: 'mod_assignment', areaname: 'submission', activemethod: rubric, rubricdefinition ( method: 'rubric', status: 20, name: 'My first rubric', description: 'my description', timemodified: 13242 criteria: - rubric criteria array containing levels array for each criterion ) markingguidedefinition ( method: 'markingguide', status: 20, name: 'My first marking guide', description: 'my description', timemodified: 13242 criteria: - marking guide criteria .... ) ) What do you think?
          Hide
          Marina Glancy added a comment - - edited

          Paul,
          timemodified and criteria and actually all other fields within definition are returned by plugin (as returned by gradingform_controller::get_definition)

          in your example areaid will be different for those two definitions. And it is also part of definition by the way

          regarding courseid,coursecontextid,moduleid - it's completely up to you, if you need it

          I do not like the keys 'rubricdefinition' and 'markingguidedefinition'. In case you want to receive one module context in one element, I would suggest the following structure:

          (
            courseid: 13, ?
            coursecontextid: 15, ?
            moduleid: 123, ?
            contextid: 456,
            component: 'mod_assignment',
            areas: (
              submission: (
                areaname: 'submission',
                activemethod: rubric,
                definitions: (
                  rubric: (
                    areaid: 3,
                    method: 'rubric',
                    status: 20,
                    name: 'My first rubric',
                    description: 'my description',
                    timemodified: 13242,
                    ... - whatever additional information this grading method provides (criteria)
                  ),
                  markingguide: (
                    areaid: 4,
                    method: 'markingguide',
                    status: 20,
                    name: 'My first marking guide',
                    description: 'my description',
                    timemodified: 13242,
                    ... - whatever additional information this grading method provides
                  )
                )
              ),
              // here might be another grading area but at the moment 'submission' is the only one
            )
          )
          
          Show
          Marina Glancy added a comment - - edited Paul, timemodified and criteria and actually all other fields within definition are returned by plugin (as returned by gradingform_controller::get_definition) in your example areaid will be different for those two definitions. And it is also part of definition by the way regarding courseid,coursecontextid,moduleid - it's completely up to you, if you need it I do not like the keys 'rubricdefinition' and 'markingguidedefinition'. In case you want to receive one module context in one element, I would suggest the following structure: ( courseid: 13, ? coursecontextid: 15, ? moduleid: 123, ? contextid: 456, component: 'mod_assignment', areas: ( submission: ( areaname: 'submission', activemethod: rubric, definitions: ( rubric: ( areaid: 3, method: 'rubric', status: 20, name: 'My first rubric', description: 'my description', timemodified: 13242, ... - whatever additional information this grading method provides (criteria) ), markingguide: ( areaid: 4, method: 'markingguide', status: 20, name: 'My first marking guide', description: 'my description', timemodified: 13242, ... - whatever additional information this grading method provides ) ) ), // here might be another grading area but at the moment 'submission' is the only one ) )
          Hide
          Paul Charsley added a comment -

          Hi Marina,

          Thanks for your feedback. We'll begin development based on your latest suggestion. I'm sure more questions will come up before we've finished!

          Show
          Paul Charsley added a comment - Hi Marina, Thanks for your feedback. We'll begin development based on your latest suggestion. I'm sure more questions will come up before we've finished!
          Hide
          Martin Dougiamas added a comment -

          My -1000 for contextids in these grade-related web service functions, and probably for ANY web service functions.

          Context IDs are an internal system for Moodle development, which were added to make different things like categories/courses/activities appear more consistent for capability checking.

          However, the main ID for COURSES is courseid, and for ACTIVITIES it's the moduleid. It makes no sense at all to have grading forms for categories or blocks (or even courses probably).

          The web services should stick to these conventions. It's clearer and more logical for an external API, and it's easy for internal code to derive the contextid from them if required.

          Show
          Martin Dougiamas added a comment - My -1000 for contextids in these grade-related web service functions, and probably for ANY web service functions. Context IDs are an internal system for Moodle development, which were added to make different things like categories/courses/activities appear more consistent for capability checking. However, the main ID for COURSES is courseid, and for ACTIVITIES it's the moduleid. It makes no sense at all to have grading forms for categories or blocks (or even courses probably). The web services should stick to these conventions. It's clearer and more logical for an external API, and it's easy for internal code to derive the contextid from them if required.
          Hide
          Wirianto Djunaidi added a comment -

          I agree with Martin on contextids. I'm looking into webservices API for uploading/downloading multiple files and am thinking about the same thing. It does put the burden on the webservices code to do all the appropriate lookup of the contextids when needed.

          Show
          Wirianto Djunaidi added a comment - I agree with Martin on contextids. I'm looking into webservices API for uploading/downloading multiple files and am thinking about the same thing. It does put the burden on the webservices code to do all the appropriate lookup of the contextids when needed.
          Hide
          David Mudrak added a comment -

          internal system for Moodle development, which were added to make different things like categories/courses/activities appear more consistent for capability checking.

          Well, contexts definitely serve in more areas than just capability checks these days. For example, Questions, Comments, Tags or Ratings live in them and it is substantial to use the contextid when referring to their location.

          It makes no sense at all to have grading forms for categories or blocks (or even courses probably).

          Not really. When grading forms API was being designed, this aspect was considered. Finally it was decided that the grading forms should not be coupled with activities modules exclusively. As an example, grading forms could be shared per course_category (like questions do) in the future. Or, if someone wants to use a rubric directly in the gradebook (for manually created grade items), then the context of the rubric would be the course.

          The web services should stick to these conventions

          But, what if one day we are going to implement a WS to handle Comments or Ratings? IMHO we can't hide context from the outer world, it's just too essential for correct and unique addressing.

          So, if we drop context from the WS discussed here, we will offer the access to just a subset of all available grading forms. We would give access only to those that are already linked with a course_module. That means that for example shared rubric templates are not available via WS...

          Show
          David Mudrak added a comment - internal system for Moodle development, which were added to make different things like categories/courses/activities appear more consistent for capability checking. Well, contexts definitely serve in more areas than just capability checks these days. For example, Questions, Comments, Tags or Ratings live in them and it is substantial to use the contextid when referring to their location. It makes no sense at all to have grading forms for categories or blocks (or even courses probably). Not really. When grading forms API was being designed, this aspect was considered. Finally it was decided that the grading forms should not be coupled with activities modules exclusively. As an example, grading forms could be shared per course_category (like questions do) in the future. Or, if someone wants to use a rubric directly in the gradebook (for manually created grade items), then the context of the rubric would be the course. The web services should stick to these conventions But, what if one day we are going to implement a WS to handle Comments or Ratings? IMHO we can't hide context from the outer world, it's just too essential for correct and unique addressing. So, if we drop context from the WS discussed here, we will offer the access to just a subset of all available grading forms. We would give access only to those that are already linked with a course_module. That means that for example shared rubric templates are not available via WS...
          Hide
          Jérôme Mouneyrac added a comment - - edited

          As we didn't come up to an agreement in yesterday chat, I indicated that for the moment we are dealing without them, see the Moodledocs. I'll update this information once we are sure how to deal with client dev "element" id.

          My suggestion:
          I think the client developer just want one identifier per "element". I don't think the external dev cares how it is called in Moodle. He probably expect something called 'id'.
          I think the web service documentation (automatically generated from the external descriptions) should explains it clearly. I see two solutions:
          a) a parameter called 'id' with for description "This unique identifier is in fact the contextid of the grading dorm/comment/rating in Moodle"
          b) a parameter called 'contextid' with for description "This is the unique identifier of this gradring form/comment/rating..."

          Finally I think we would need some kind of table to explain to web service developer what are the unique identifier by element in Moodle core:

          course => course id
          user => user id
          comment => context id
          gradind form => context id
          ...

          Show
          Jérôme Mouneyrac added a comment - - edited As we didn't come up to an agreement in yesterday chat, I indicated that for the moment we are dealing without them, see the Moodledocs . I'll update this information once we are sure how to deal with client dev "element" id. My suggestion: I think the client developer just want one identifier per "element". I don't think the external dev cares how it is called in Moodle. He probably expect something called 'id'. I think the web service documentation (automatically generated from the external descriptions) should explains it clearly. I see two solutions: a) a parameter called 'id' with for description "This unique identifier is in fact the contextid of the grading dorm/comment/rating in Moodle" b) a parameter called 'contextid' with for description "This is the unique identifier of this gradring form/comment/rating..." Finally I think we would need some kind of table to explain to web service developer what are the unique identifier by element in Moodle core: course => course id user => user id comment => context id gradind form => context id ...
          Hide
          David Mudrak added a comment -

          Thanks for the comment Jerome. However, my understanding of the situation is slightly different. IMHO Martin objects against using contexts in WS conceptually, not just in the syntax (param name). I can really see no point of calling the contextid as plain "id" as it actually makes the documentation more sucky (less self-descriptive).

          Also note that the context id itself is not unique identifier of these "elements" (as you call them) above. You need the common "holy four" - context, component, area and itemid.

          Show
          David Mudrak added a comment - Thanks for the comment Jerome. However, my understanding of the situation is slightly different. IMHO Martin objects against using contexts in WS conceptually, not just in the syntax (param name). I can really see no point of calling the contextid as plain "id" as it actually makes the documentation more sucky (less self-descriptive). Also note that the context id itself is not unique identifier of these "elements" (as you call them) above. You need the common "holy four" - context, component, area and itemid.
          Hide
          Paul Charsley added a comment -

          I agree that using context ids is not good for client applications since it requires them to store information that should normally be kept internal to Moodle. How about the following parameters?:

          • array of course module ids
          • areaname (e.g 'submissions')
          Show
          Paul Charsley added a comment - I agree that using context ids is not good for client applications since it requires them to store information that should normally be kept internal to Moodle. How about the following parameters?: array of course module ids areaname (e.g 'submissions')
          Hide
          David Mudrak added a comment -

          BTW I just noticed that the core subsystem for this is "grading" not "gradingform" so the name of the function should have core_grading_ prefix if I remember the naming policy correctly.

          If it is decided to keep hiding contexts from WS (which is IMHO not longitudinaly sustainable though), let me suggest that the name of the WS method somehow reflects the fact that it provides access just to some grading forms (those attached to course modules). Something like core_grading_get_activity_grading_forms_definition() but probably better.

          Show
          David Mudrak added a comment - BTW I just noticed that the core subsystem for this is "grading" not "gradingform" so the name of the function should have core_grading_ prefix if I remember the naming policy correctly. If it is decided to keep hiding contexts from WS (which is IMHO not longitudinaly sustainable though), let me suggest that the name of the WS method somehow reflects the fact that it provides access just to some grading forms (those attached to course modules). Something like core_grading_get_activity_grading_forms_definition() but probably better.
          Hide
          Jérôme Mouneyrac added a comment -

          Hi David, thanks for the clarification. Can you suggest a function that I should try to implement to understand how required/beneficial are 'context, component, area and itemid' as web service parameters. For the moment I'll try to implement core_comment_get_comments (MDL-30094) without the "holy four". You mentioned that comments need the "holy four".

          In the meanwhile I like Paul suggestion for the parameters. To not slow down Paul in his work, I think Paul you can start on implementing this function. Maybe just change the issue name for what David suggested.

          Cheers.

          Show
          Jérôme Mouneyrac added a comment - Hi David, thanks for the clarification. Can you suggest a function that I should try to implement to understand how required/beneficial are 'context, component, area and itemid' as web service parameters. For the moment I'll try to implement core_comment_get_comments ( MDL-30094 ) without the "holy four". You mentioned that comments need the "holy four". In the meanwhile I like Paul suggestion for the parameters. To not slow down Paul in his work, I think Paul you can start on implementing this function. Maybe just change the issue name for what David suggested. Cheers.
          Hide
          Jérôme Mouneyrac added a comment -

          Just a note for people who come over to talk about contextid, it was decided that contextid will not be used as web service function parameters. The functions will decide which is the right context following what the function is doing and the other parameters.

          Show
          Jérôme Mouneyrac added a comment - Just a note for people who come over to talk about contextid, it was decided that contextid will not be used as web service function parameters. The functions will decide which is the right context following what the function is doing and the other parameters.
          Hide
          Jérôme Mouneyrac added a comment -

          Hi Yang,
          what's the status of this issue? Are you still working on it or is it to peer review?
          Cheers,
          Jerome

          Show
          Jérôme Mouneyrac added a comment - Hi Yang, what's the status of this issue? Are you still working on it or is it to peer review? Cheers, Jerome
          Hide
          Jérôme Mouneyrac added a comment - - edited

          Assigning to Andrew for peer-review. I'm not sure who to assign this issue to. I had to pick someone between Marina/David/Andrew, the grade experts that I know. You mainly need to review the code logic in get_definitions($cmids, $areaname).

          Show
          Jérôme Mouneyrac added a comment - - edited Assigning to Andrew for peer-review. I'm not sure who to assign this issue to. I had to pick someone between Marina/David/Andrew, the grade experts that I know. You mainly need to review the code logic in get_definitions($cmids, $areaname).
          Hide
          Yang Yang added a comment -

          Hello Jerome

          Is this issue still under the peer-review? It seems like the status has been changed to "development in progress". Any feedback from the reviewer?

          Thanks
          Yang

          Show
          Yang Yang added a comment - Hello Jerome Is this issue still under the peer-review? It seems like the status has been changed to "development in progress". Any feedback from the reviewer? Thanks Yang
          Hide
          Jérôme Mouneyrac added a comment -

          Hi Yang,
          my peer-review (web service only, I don't know much about grading form):

          • run moodlechecker / codechecker on it the new file. I can see some errors. https://github.com/moodlehq/moodle-local_codechecker and https://github.com/marinaglancy/moodle-local_moodlecheck. Also try to have consistent indentation.
          • you don't need the VALUE_REQUIRED, they are the default for parameters
          • the array definitions ($warnings = array();, $result = array();,...) should not be all at the beginning. I would define them when it is necessary to define them. For example $rubric = array(); should be at the beginning of the inner foreach using it. This will save you to redeclare it a second time at the end of the foreach. Have a look to all these declarations and place them as close as you can from where you are using them.
          • as recently decided warnings should not be used for skipping: http://docs.moodle.org/dev/Errors_handling_in_web_services#When_to_send_a_warning_on_the_server_side. I don't think you need any warnings anymore you can throw exception all the time (I'm sure you should not use warnings for course not found or missing capabilities, and I suppose neither for all the grading errors)

          That's all

          Note when I'll send it for integration, I didn't test:

          • relevance
          • the grading form code logic: is there a core function doing the same thing? Is it 100% matching core featured? Is the capability and checks used matching 100% of what they allow the user to do in web interface?
          Show
          Jérôme Mouneyrac added a comment - Hi Yang, my peer-review (web service only, I don't know much about grading form): run moodlechecker / codechecker on it the new file. I can see some errors. https://github.com/moodlehq/moodle-local_codechecker and https://github.com/marinaglancy/moodle-local_moodlecheck . Also try to have consistent indentation. you don't need the VALUE_REQUIRED, they are the default for parameters the array definitions ($warnings = array();, $result = array();,...) should not be all at the beginning. I would define them when it is necessary to define them. For example $rubric = array(); should be at the beginning of the inner foreach using it. This will save you to redeclare it a second time at the end of the foreach. Have a look to all these declarations and place them as close as you can from where you are using them. as recently decided warnings should not be used for skipping: http://docs.moodle.org/dev/Errors_handling_in_web_services#When_to_send_a_warning_on_the_server_side . I don't think you need any warnings anymore you can throw exception all the time (I'm sure you should not use warnings for course not found or missing capabilities, and I suppose neither for all the grading errors) That's all Note when I'll send it for integration, I didn't test: relevance the grading form code logic: is there a core function doing the same thing? Is it 100% matching core featured? Is the capability and checks used matching 100% of what they allow the user to do in web interface?
          Hide
          Jérôme Mouneyrac added a comment - - edited

          Can you apply these code cleaning changes on the other lightwork issues in peer-review thanks. (MDL-31682, MDL-31683, and MDL-31873)

          Show
          Jérôme Mouneyrac added a comment - - edited Can you apply these code cleaning changes on the other lightwork issues in peer-review thanks. ( MDL-31682 , MDL-31683 , and MDL-31873 )
          Hide
          Yang Yang added a comment -

          Hi Jerome

          This web service takes multiple cmids. My thinking is that if a user has the capabilities to view grading forms from all the courses but one, he should be able to retrieve the grading forms via the web service from the courses he is allowed to. If we threw an exception, it would prevent the user from getting any grading forms even if he is capable of.

          Same reason for the grading errors. When an assignment was created and either rubric or guide was selected as the grading option, but the grading form has not been created yet. If an exception was thrown, no other grading forms of other cmids can be returned via the web service.

          What are your thoughts on this?

          Regards,
          Yang

          Show
          Yang Yang added a comment - Hi Jerome This web service takes multiple cmids. My thinking is that if a user has the capabilities to view grading forms from all the courses but one, he should be able to retrieve the grading forms via the web service from the courses he is allowed to. If we threw an exception, it would prevent the user from getting any grading forms even if he is capable of. Same reason for the grading errors. When an assignment was created and either rubric or guide was selected as the grading option, but the grading form has not been created yet. If an exception was thrown, no other grading forms of other cmids can be returned via the web service. What are your thoughts on this? Regards, Yang
          Hide
          Jérôme Mouneyrac added a comment -

          Hi Yang,
          I guess you are missing a different web service function that let you know what you should be asking for. We must follow what has been decided otherwise all web service functions will keep getting inconsistent. The current decision is, as mentioned in the Moodledocs:

          If something is going to be skipped or not done (WRITE operation) then it's an exception. Warnings only inform (WRITE operation) about some decisions made by the functions. Warnings must never prevents the WRITE operation to happen. It's the same for a READ operation. The results will be always returned, with or without warnings. if something prevents the results to be returned then it's an exception, not a warning.

          Show
          Jérôme Mouneyrac added a comment - Hi Yang, I guess you are missing a different web service function that let you know what you should be asking for. We must follow what has been decided otherwise all web service functions will keep getting inconsistent. The current decision is, as mentioned in the Moodledocs: If something is going to be skipped or not done (WRITE operation) then it's an exception. Warnings only inform (WRITE operation) about some decisions made by the functions. Warnings must never prevents the WRITE operation to happen. It's the same for a READ operation. The results will be always returned, with or without warnings. if something prevents the results to be returned then it's an exception, not a warning.
          Hide
          Paul Charsley added a comment -

          Hi Jerome,

          We've read the page on the Moodle docs but I think we need to think about the implications.
          Imagine the following scenario:

          A web service client calls the get_definitions web service with 30 course module ids in its $cmids parameter that the user has previously had the permissions to view.

          Since the previous web service call, for whatever reason, the user's rights to access one of these course
          modules (module x) has been removed. This can be a normal and frequent occurence in Moodle.

          It would NOT be sensible to throw an exception in this scenario. It would be much better to return the results for the 29 course modules that the user does have the rights to access plus a warning message informing them that they no longer have the right to access course module x.

          What do you think?
          Paul

          Show
          Paul Charsley added a comment - Hi Jerome, We've read the page on the Moodle docs but I think we need to think about the implications. Imagine the following scenario: A web service client calls the get_definitions web service with 30 course module ids in its $cmids parameter that the user has previously had the permissions to view. Since the previous web service call, for whatever reason, the user's rights to access one of these course modules (module x) has been removed. This can be a normal and frequent occurence in Moodle. It would NOT be sensible to throw an exception in this scenario. It would be much better to return the results for the 29 course modules that the user does have the rights to access plus a warning message informing them that they no longer have the right to access course module x. What do you think? Paul
          Hide
          Jérôme Mouneyrac added a comment -

          In order to resolve your issue:
          a) your client receives the exception error code
          b) your client calls a ws function to update user's rights to access all of these courses
          c) your client does a new call with correct courses

          If you really don't like the current design (http://docs.moodle.org/dev/Errors_handling_in_web_services#When_to_send_a_warning_on_the_server_side) you are welcome to bring up the topic in the dev chat. Come over around 4PM Perth time when most of HQ is present.

          Show
          Jérôme Mouneyrac added a comment - In order to resolve your issue: a) your client receives the exception error code b) your client calls a ws function to update user's rights to access all of these courses c) your client does a new call with correct courses If you really don't like the current design ( http://docs.moodle.org/dev/Errors_handling_in_web_services#When_to_send_a_warning_on_the_server_side ) you are welcome to bring up the topic in the dev chat. Come over around 4PM Perth time when most of HQ is present.
          Hide
          Martin Dougiamas added a comment -

          Although I know it's a harder and more painful method, Jerome is quite correct here.

          If the client does not have an up-to-date view of the data in Moodle, how can it be trusted to update the data? It may work for some particular web services in some particular cases, but there could be all sorts of unintended consequences if this was allowed for the general case.

          Show
          Martin Dougiamas added a comment - Although I know it's a harder and more painful method, Jerome is quite correct here. If the client does not have an up-to-date view of the data in Moodle, how can it be trusted to update the data? It may work for some particular web services in some particular cases, but there could be all sorts of unintended consequences if this was allowed for the general case.
          Hide
          Paul Charsley added a comment -

          Hi Jerome, By returning full information about the errors (instead of throwing an exception and stopping processing as you propose) the client is able to make an informed decision about the correct action to take.
          Another scenario to consider is the case of multiple errors. Throwing an exception for the first error would mean that the client has no information about subsequent errors.
          I will create a discussion topic to get feedback from other web service users since it would be good to open this up to a wider audience.
          Cheers, Paul

          Show
          Paul Charsley added a comment - Hi Jerome, By returning full information about the errors (instead of throwing an exception and stopping processing as you propose) the client is able to make an informed decision about the correct action to take. Another scenario to consider is the case of multiple errors. Throwing an exception for the first error would mean that the client has no information about subsequent errors. I will create a discussion topic to get feedback from other web service users since it would be good to open this up to a wider audience. Cheers, Paul
          Hide
          Paul Charsley added a comment -

          Hi Martin, Jerome,
          To answer your point, "if a client does not have an up to date view of the data in Moodle, how can it be trusted to update the data correctly?".
          Continuing with the course module example and using error messages instead of an exception, the client will have full data for the course modules for which it has permissions and no data except a useful explanatory message for the course modules for which it's access rights have been withdrawn. I don't see why this would cause it to update the data incorrectly.
          Another factor to consider is the amount of unnecessary processing required by forcing the client to re fetch all data just because of an enrollment change in one of many course modules requested. Certainly in an environment like we have at Massey with a very large number of courses and modules, performance is a very big factor in the successful uptake of a web service client.
          How about the following? We add a "strictprocessing" flag to the web service function parameters. If set to true, an exception is thrown for all errors. If false, certain errors are listed and returned to the client through the warnings mechanism. Note that obviously some errors are always thrown as exceptions, e.g the user does not have the correct access rights to call the function.
          Paul

          Show
          Paul Charsley added a comment - Hi Martin, Jerome, To answer your point, "if a client does not have an up to date view of the data in Moodle, how can it be trusted to update the data correctly?". Continuing with the course module example and using error messages instead of an exception, the client will have full data for the course modules for which it has permissions and no data except a useful explanatory message for the course modules for which it's access rights have been withdrawn. I don't see why this would cause it to update the data incorrectly. Another factor to consider is the amount of unnecessary processing required by forcing the client to re fetch all data just because of an enrollment change in one of many course modules requested. Certainly in an environment like we have at Massey with a very large number of courses and modules, performance is a very big factor in the successful uptake of a web service client. How about the following? We add a "strictprocessing" flag to the web service function parameters. If set to true, an exception is thrown for all errors. If false, certain errors are listed and returned to the client through the warnings mechanism. Note that obviously some errors are always thrown as exceptions, e.g the user does not have the correct access rights to call the function. Paul
          Hide
          Paul Charsley added a comment -

          Hi Martin, Jerome,

          Please can you let me know whether or not my last suggestion would be acceptable, i.e. to add a "strictprocessing" flag parameter to the web service function which by default is set to "true" and makes the function behave as you require. When set to false, it returns error messages for course modules where the data cannot be returned instead of immediately stopping and throwing an exception.

          Thanks, Paul

          Show
          Paul Charsley added a comment - Hi Martin, Jerome, Please can you let me know whether or not my last suggestion would be acceptable, i.e. to add a "strictprocessing" flag parameter to the web service function which by default is set to "true" and makes the function behave as you require. When set to false, it returns error messages for course modules where the data cannot be returned instead of immediately stopping and throwing an exception. Thanks, Paul
          Hide
          Jérôme Mouneyrac added a comment -

          Hi Paul,
          It looks to me that this "strictprocessing" parameter is just a solution to bypass the reason why we decided to throw exceptions.

          The reason we return exception is too avoid bad client design where the client just deals everywhere with "corrupted" information and do not bother to sync anything. By returning exception Moodle forces the client to have synced data. I understand it's more work on the client to have synced information all the time, but at the end:

          • you have a client for single users: an error happens, the client detects a missing info, your client syncs the information, and try again. The sync should never be very long for user use case.
          • you have a client that do long and important system operations. In this case, it seems like a very important client, you most likely don't want to have any corrupted data all over the place.

          At the end the issue could be more how to keep clients and Moodle data synced together?

          Now if there is no solution for having proper synced data, you still can open a new issue where more people can talk about. Also if you can come up with some client code do demonstrate that there is no viable solution when the web service function execution is stopping, it could help to study/understand/resolve this problem.

          Thanks,
          Jerome

          Show
          Jérôme Mouneyrac added a comment - Hi Paul, It looks to me that this "strictprocessing" parameter is just a solution to bypass the reason why we decided to throw exceptions. The reason we return exception is too avoid bad client design where the client just deals everywhere with "corrupted" information and do not bother to sync anything. By returning exception Moodle forces the client to have synced data. I understand it's more work on the client to have synced information all the time, but at the end: you have a client for single users: an error happens, the client detects a missing info, your client syncs the information, and try again. The sync should never be very long for user use case. you have a client that do long and important system operations. In this case, it seems like a very important client, you most likely don't want to have any corrupted data all over the place. At the end the issue could be more how to keep clients and Moodle data synced together? Now if there is no solution for having proper synced data, you still can open a new issue where more people can talk about. Also if you can come up with some client code do demonstrate that there is no viable solution when the web service function execution is stopping, it could help to study/understand/resolve this problem. Thanks, Jerome
          Hide
          Paul Charsley added a comment -

          Hi Jerome,

          First of all it is not a question of corrupted information. For all our Web services we are returning
          information for course modules where possible plus descriptive error messages when the data cannot be returned.

          What our code is doing is dealing with errors appropriately and not just throwing exceptions without considering the implications. This is standard programming practice and good design.

          One of the important aspects of Lightwork is its ability to provide descriptive information to teachers during or following the synchronization process. If we use your approach then it will be very difficult for us to do this.

          Lightwork has been around for a number of years now and so we have a lot of experience with using web services to synchronize large quantities of data between a client application and Moodle. Please feel free to look at the client Java synchronization code at

          http://lightwork.massey.ac.nz/repositories/browse/fat/branches/lightwork-branch-3.1/lightwork/nz.ac.massey.lightwork/src/nz/ac/massey/lightwork/moodle
          http://lightwork.massey.ac.nz/repositories/browse/fat/branches/lightwork-branch-3.1/lightwork/nz.ac.massey.lightwork/src/nz/ac/massey/lightwork/moodlews

          I'm concerned that you continue to talk about "corrupted" data and really don't understand why you are not willing to consider my suggestion for a "strictprocessing" flag which defaults to your wishes.

          This is an important issue for all our web services and so I'll need to add a section to our Lightwork design page that describes in detail all the errors that can occur in our web service functions and whether exceptions should be thrown or warning messages created. Since this affects all web services I'll create a web services discussion topic instead of continuing to use this issue.

          Cheers, Paul

          Show
          Paul Charsley added a comment - Hi Jerome, First of all it is not a question of corrupted information. For all our Web services we are returning information for course modules where possible plus descriptive error messages when the data cannot be returned. What our code is doing is dealing with errors appropriately and not just throwing exceptions without considering the implications. This is standard programming practice and good design. One of the important aspects of Lightwork is its ability to provide descriptive information to teachers during or following the synchronization process. If we use your approach then it will be very difficult for us to do this. Lightwork has been around for a number of years now and so we have a lot of experience with using web services to synchronize large quantities of data between a client application and Moodle. Please feel free to look at the client Java synchronization code at http://lightwork.massey.ac.nz/repositories/browse/fat/branches/lightwork-branch-3.1/lightwork/nz.ac.massey.lightwork/src/nz/ac/massey/lightwork/moodle http://lightwork.massey.ac.nz/repositories/browse/fat/branches/lightwork-branch-3.1/lightwork/nz.ac.massey.lightwork/src/nz/ac/massey/lightwork/moodlews I'm concerned that you continue to talk about "corrupted" data and really don't understand why you are not willing to consider my suggestion for a "strictprocessing" flag which defaults to your wishes. This is an important issue for all our web services and so I'll need to add a section to our Lightwork design page that describes in detail all the errors that can occur in our web service functions and whether exceptions should be thrown or warning messages created. Since this affects all web services I'll create a web services discussion topic instead of continuing to use this issue. Cheers, Paul
          Hide
          Paul Charsley added a comment -

          Hi Jerome,

          Continuing on from our discussions outside of this issue, I am listing below how this web service will manage exceptions and warnings. Before reviewing the list, please could you read the user story that I have added at http://docs.moodle.org/dev/Lightwork#Using_Lightwork_with_Moodle_for_offline_marking

          I have added this to explain how this web service might be used in the larger context of offline marking with Lightwork

          1) In the test for moodle/grade:viewall capability for an individual cmid we will modify the code to throw an exception if the user does not have the capability for that course module

          2) In the test where a course module is not found we will modify the code to throw an exception

          3) In all other cases, the warnings will remain as warnings because they represent perfectly valid cases of a partially completed marking guide or rubric. There may not even be a marking guide or rubric for a course module. This is perfectly valid. It could be that the teacher has either not started it yet or maybe doesn't intend to use one (Moodle does not force teachers to use Marking guides or rubrics).

          Let me know what you think,
          Cheers, Paul

          Show
          Paul Charsley added a comment - Hi Jerome, Continuing on from our discussions outside of this issue, I am listing below how this web service will manage exceptions and warnings. Before reviewing the list, please could you read the user story that I have added at http://docs.moodle.org/dev/Lightwork#Using_Lightwork_with_Moodle_for_offline_marking I have added this to explain how this web service might be used in the larger context of offline marking with Lightwork 1) In the test for moodle/grade:viewall capability for an individual cmid we will modify the code to throw an exception if the user does not have the capability for that course module 2) In the test where a course module is not found we will modify the code to throw an exception 3) In all other cases, the warnings will remain as warnings because they represent perfectly valid cases of a partially completed marking guide or rubric. There may not even be a marking guide or rubric for a course module. This is perfectly valid. It could be that the teacher has either not started it yet or maybe doesn't intend to use one (Moodle does not force teachers to use Marking guides or rubrics). Let me know what you think, Cheers, Paul
          Hide
          Yang Yang added a comment -

          Hi Jerome

          Incorrect cmid and missing moodle/grade:viewall capability have been changed from throw warnings to exceptions.

          Cheers
          Yang

          Show
          Yang Yang added a comment - Hi Jerome Incorrect cmid and missing moodle/grade:viewall capability have been changed from throw warnings to exceptions. Cheers Yang
          Hide
          Jérôme Mouneyrac added a comment -

          Thanks Yang/Paul. I'll check that after 2.3 release.

          Important point, we came back on the decision to force throwing exception as more people suggested that bulk operations should not be stopping execution. I updated the doc: http://docs.moodle.org/dev/Errors_handling_in_web_services#When_to_send_a_warning_on_the_server_side

          Thank you for debating about it

          Show
          Jérôme Mouneyrac added a comment - Thanks Yang/Paul. I'll check that after 2.3 release. Important point, we came back on the decision to force throwing exception as more people suggested that bulk operations should not be stopping execution. I updated the doc: http://docs.moodle.org/dev/Errors_handling_in_web_services#When_to_send_a_warning_on_the_server_side Thank you for debating about it
          Hide
          Yang Yang added a comment - - edited

          Thanks Jerome

          Incorrect cmid and missing moodle/grade:viewall capability have been changed back from exceptions to warnings.

          Cheers
          Yang

          Show
          Yang Yang added a comment - - edited Thanks Jerome Incorrect cmid and missing moodle/grade:viewall capability have been changed back from exceptions to warnings. Cheers Yang
          Hide
          Jérôme Mouneyrac added a comment -

          Hello guys,
          following the Moodle peer-review process, I reassigned the peer-reviewer to the component maintainer. If the component maintainer is unknown assign the peer-reviewer to moodle.com.
          Thanks.

          Note for the peer-reviewer: I understand the issue history is long. However you just need review the "Pull Master Diff URL". Then you can look at the issue history if anything seems strange. If any question you can contact me in private, I'll be happy to help. There is also a Moodledocs to help peer-reviewing web service contribution: http://docs.moodle.org/dev/How_to_peer_review_a_core_web_service_function. Thanks. Jerome.

          Show
          Jérôme Mouneyrac added a comment - Hello guys, following the Moodle peer-review process, I reassigned the peer-reviewer to the component maintainer. If the component maintainer is unknown assign the peer-reviewer to moodle.com. Thanks. Note for the peer-reviewer: I understand the issue history is long. However you just need review the "Pull Master Diff URL". Then you can look at the issue history if anything seems strange. If any question you can contact me in private, I'll be happy to help. There is also a Moodledocs to help peer-reviewing web service contribution: http://docs.moodle.org/dev/How_to_peer_review_a_core_web_service_function . Thanks. Jerome.
          Hide
          Jérôme Mouneyrac added a comment -

          Note: Don't forget to add your function to the WS API list: http://docs.moodle.org/dev/Web_services_Roadmap#Core_web_service_functions (this is an automatic message)

          Show
          Jérôme Mouneyrac added a comment - Note: Don't forget to add your function to the WS API list: http://docs.moodle.org/dev/Web_services_Roadmap#Core_web_service_functions (this is an automatic message)
          Hide
          Paul Charsley added a comment -

          Added PHPUnit tests and assigned for peer review.

          Show
          Paul Charsley added a comment - Added PHPUnit tests and assigned for peer review.
          Hide
          Paul Charsley added a comment -

          Rebased with latest weekly release and successfully ran PHPUnit tests

          Show
          Paul Charsley added a comment - Rebased with latest weekly release and successfully ran PHPUnit tests
          Hide
          Paul Charsley added a comment -

          Rebased with the latest weekly release

          Show
          Paul Charsley added a comment - Rebased with the latest weekly release
          Hide
          Marina Glancy added a comment -

          Hi, sorry Paul, I actually forgot about this issue. Thanks for reminding.

          And sorry again but I completely disagree with your approach. Though your code looks very neat and easy to read.

          Maybe your code works for rubrics and marking guide and mod_assign but will fail completely for any other module supporting advanced grading or any other contributed advanced grading plugin.

          Your check for capability 'assign:view' makes code not working for mod_assignment (and all non-core modules supporting advanced grading). Also you query table 'gradingform_'.$def->method.'_criteria' and it is completely up to grading method which table to use for storing data.

          There MUST be function(s) inside grading methods plugins to return such information for external queries. Externallib should ONLY be calling API functions and make no direct queries to DB.

          There are already functions get_definition_copy() and get_definition_for_editing() that return the definition. If for externallib we need definition with the original ids and not processed file links, feel free to add a new function to the parent class and overwrite it in rubrics and marking guide. This way contributed plugins that need to support externallib will implement this method as well.

          The same with the capability to view the module (or definition) - it must be generic. Also please don't forget that 'mod:xxx/view' is not even a standard capability. Module may not define it. Besides this is not the only capability for user to view the advanced grading definition.

          In case of both rubrics and marking guide user must have either capability 'moodle/grade:managegradingforms' or capability to view module but only in case an option 'alwaysshowdefinition' is on and definition is ready. Students can never see not active definition.

          Well, the resume:
          core_grade_external::get_definition() should not define structure for criteria and levels because this is rubric/guide - specific structure. This function should only define fields that are the same for ANY grading method (including non-core ones). Afaik externallib does not support free format in returned xml. In this case additional method-specific data (such as criteria/levels) should be returned in some serialised string.

          core_grade_external::get_definitions() should call functions from grading methods and not do DB queries. Also capabilities check should be more generic and most likely be a callback to grading method again.

          Actually I'm pretty sure we discussed it already.

          I've spent some time recently looking for bugs in course categories creation in externallib. The result was one-line fix (see MDL-38144) but it took a while to find out why. And this could be avoided if externallib did not copy the code but rather use the same functions as core (and created those functions if they did not exist).

          Well sorry again to fail your work and that it took me a while to start reviewing.

          Show
          Marina Glancy added a comment - Hi, sorry Paul, I actually forgot about this issue. Thanks for reminding. And sorry again but I completely disagree with your approach. Though your code looks very neat and easy to read. Maybe your code works for rubrics and marking guide and mod_assign but will fail completely for any other module supporting advanced grading or any other contributed advanced grading plugin. Your check for capability 'assign:view' makes code not working for mod_assignment (and all non-core modules supporting advanced grading). Also you query table 'gradingform_'.$def->method.'_criteria' and it is completely up to grading method which table to use for storing data. There MUST be function(s) inside grading methods plugins to return such information for external queries. Externallib should ONLY be calling API functions and make no direct queries to DB. There are already functions get_definition_copy() and get_definition_for_editing() that return the definition. If for externallib we need definition with the original ids and not processed file links, feel free to add a new function to the parent class and overwrite it in rubrics and marking guide. This way contributed plugins that need to support externallib will implement this method as well. The same with the capability to view the module (or definition) - it must be generic. Also please don't forget that 'mod:xxx/view' is not even a standard capability. Module may not define it. Besides this is not the only capability for user to view the advanced grading definition. In case of both rubrics and marking guide user must have either capability 'moodle/grade:managegradingforms' or capability to view module but only in case an option 'alwaysshowdefinition' is on and definition is ready. Students can never see not active definition. Well, the resume: core_grade_external::get_definition() should not define structure for criteria and levels because this is rubric/guide - specific structure. This function should only define fields that are the same for ANY grading method (including non-core ones). Afaik externallib does not support free format in returned xml. In this case additional method-specific data (such as criteria/levels) should be returned in some serialised string. core_grade_external::get_definitions() should call functions from grading methods and not do DB queries. Also capabilities check should be more generic and most likely be a callback to grading method again. Actually I'm pretty sure we discussed it already. I've spent some time recently looking for bugs in course categories creation in externallib. The result was one-line fix (see MDL-38144 ) but it took a while to find out why. And this could be avoided if externallib did not copy the code but rather use the same functions as core (and created those functions if they did not exist). Well sorry again to fail your work and that it took me a while to start reviewing.
          Hide
          Marina Glancy added a comment -

          Example of grading method that does not have table "criteria": https://moodle.org/plugins/pluginversions.php?plugin=gradingform_checklist

          Show
          Marina Glancy added a comment - Example of grading method that does not have table "criteria": https://moodle.org/plugins/pluginversions.php?plugin=gradingform_checklist
          Hide
          Paul Charsley added a comment -

          Hi Marina,

          Thanks for the feedback. The challenge here is to build this in such a way as to be entirely generic and to be practical and useable for a web service client that wants to manipulate and change rubrics and marking guides in its own client application. I will re think the design of this method and take on board your comments. Thanks also for the link to the contributed plugin which will be useful.

          Cheers, Paul

          Show
          Paul Charsley added a comment - Hi Marina, Thanks for the feedback. The challenge here is to build this in such a way as to be entirely generic and to be practical and useable for a web service client that wants to manipulate and change rubrics and marking guides in its own client application. I will re think the design of this method and take on board your comments. Thanks also for the link to the contributed plugin which will be useful. Cheers, Paul
          Hide
          Paul Charsley added a comment -

          Hi Marina,

          I've redesigned based on your suggestions. The work is not quite ready for peer review yet since I still need to fix up capabilities, unit tests etc.

          I have defined a new function get_external_definition_details() on gradingform_controller that can optionally be overridden by sub classes. If a grading method wishes to expose it's method-specific data it can override this function as I've done for rubric and marking guide. Other methods, such as checklist, can just return common data. I believe that this meets both the requirement to be generic and to return the method specific data that will make it useful for a web service client application.

          Hopefully, I'll have this ready for you to peer review shortly although if you get a chance it would be great to have some early feedback!

          Thanks, Paul

          Show
          Paul Charsley added a comment - Hi Marina, I've redesigned based on your suggestions. The work is not quite ready for peer review yet since I still need to fix up capabilities, unit tests etc. I have defined a new function get_external_definition_details() on gradingform_controller that can optionally be overridden by sub classes. If a grading method wishes to expose it's method-specific data it can override this function as I've done for rubric and marking guide. Other methods, such as checklist, can just return common data. I believe that this meets both the requirement to be generic and to return the method specific data that will make it useful for a web service client application. Hopefully, I'll have this ready for you to peer review shortly although if you get a chance it would be great to have some early feedback! Thanks, Paul
          Hide
          Marina Glancy added a comment -

          Hi Paul, great, that's even better - you found the way how to support different definitions. In core_grade_external::definition() I would also wrap plugin-dependend definition in tag containing plugin name as well to better readability and to avoid naming conflicts.

          Show
          Marina Glancy added a comment - Hi Paul, great, that's even better - you found the way how to support different definitions. In core_grade_external::definition() I would also wrap plugin-dependend definition in tag containing plugin name as well to better readability and to avoid naming conflicts.
          Hide
          Paul Charsley added a comment -

          Rebased with latest weekly release and requested peer review.

          Show
          Paul Charsley added a comment - Rebased with latest weekly release and requested peer review.
          Hide
          Paul Charsley added a comment -

          Rebased with latest weekly release

          Show
          Paul Charsley added a comment - Rebased with latest weekly release
          Hide
          Marina Glancy added a comment -

          Hi Paul, sorry I did not realise it was waiting for peer review - I did not receive any message from the tracker. I like this solution very much.
          Can you please correct phpdocs for return value in get_external_definition_details() in both rubrics and guide:
          It does not return external_multiple_structure, it returns an array and actually in https://github.com/Lightwork-Marking/moodle/compare/master...MDL-31681#L0R150 you expect it to be array. Also parent function get_external_definition_details() must have more detailed phpdocs because this is what plugin developers will see. Also it should mention that the return value is expected to match the result of get_definition(). You don't need to send it to me for peer review again after that

          Cheers,
          Marina

          Show
          Marina Glancy added a comment - Hi Paul, sorry I did not realise it was waiting for peer review - I did not receive any message from the tracker. I like this solution very much. Can you please correct phpdocs for return value in get_external_definition_details() in both rubrics and guide: It does not return external_multiple_structure, it returns an array and actually in https://github.com/Lightwork-Marking/moodle/compare/master...MDL-31681#L0R150 you expect it to be array. Also parent function get_external_definition_details() must have more detailed phpdocs because this is what plugin developers will see. Also it should mention that the return value is expected to match the result of get_definition(). You don't need to send it to me for peer review again after that Cheers, Marina
          Hide
          Paul Charsley added a comment -

          Documentation changes made as required by Marina.

          Show
          Paul Charsley added a comment - Documentation changes made as required by Marina.
          Hide
          Paul Charsley added a comment -

          Hi Marina, Jerome,

          I've rebased, made the changes and rerun the unit tests but I haven't got the rights to move the issue to integration review. Would you be able to do that for me please?

          Thanks, Paul

          Show
          Paul Charsley added a comment - Hi Marina, Jerome, I've rebased, made the changes and rerun the unit tests but I haven't got the rights to move the issue to integration review. Would you be able to do that for me please? Thanks, Paul
          Hide
          Marina Glancy added a comment -

          submitting for integration on Paul's behalf

          Show
          Marina Glancy added a comment - submitting for integration on Paul's behalf
          Hide
          Damyon Wiese added a comment -

          Thanks Paul (and Marina).

          This looks like a good webservice function and the patch is almost perfect.

          The only major thing I found is that you need to use format_external_text for returning text/format fields.

          This documentation was updated earlier this year with these instructions:
          http://docs.moodle.org/dev/How_to_contribute_a_web_service_function_to_core

          Not sure the best way to do that with your code here (but checking for keys ending in format and then applying the change to both fields would probably work):

                          foreach ($details as $key => $value) {                                                                              
                              $items[$key] = $def->{$key};                                                                                    
                          }
          

          Other than that this code is good.

          Can you make these changes and add a comment here? Then I'll take another look.

          Cheers - Damyon

          Show
          Damyon Wiese added a comment - Thanks Paul (and Marina). This looks like a good webservice function and the patch is almost perfect. The only major thing I found is that you need to use format_external_text for returning text/format fields. This documentation was updated earlier this year with these instructions: http://docs.moodle.org/dev/How_to_contribute_a_web_service_function_to_core Not sure the best way to do that with your code here (but checking for keys ending in format and then applying the change to both fields would probably work): foreach ($details as $key => $value) { $items[$key] = $def->{$key}; } Other than that this code is good. Can you make these changes and add a comment here? Then I'll take another look. Cheers - Damyon
          Hide
          Damyon Wiese added a comment -

          This needs the updates for handling external text before it can be integrated. Taking it out of integration for this week.

          Show
          Damyon Wiese added a comment - This needs the updates for handling external text before it can be integrated. Taking it out of integration for this week.
          Hide
          CiBoT added a comment -

          Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

          Show
          CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
          Hide
          Paul Charsley added a comment -

          I've updated the code so that all text/format fields are now processed using format_external_text.
          Thanks, Paul

          Show
          Paul Charsley added a comment - I've updated the code so that all text/format fields are now processed using format_external_text. Thanks, Paul
          Hide
          Paul Charsley added a comment -

          Hi Marina, Damyon,
          Please could you re submit this for integration. After fixing the problems found by Damyon I've rebased with the latest weekly release and successfully re run the tests.
          Thanks, Paul

          Show
          Paul Charsley added a comment - Hi Marina, Damyon, Please could you re submit this for integration. After fixing the problems found by Damyon I've rebased with the latest weekly release and successfully re run the tests. Thanks, Paul
          Hide
          Damyon Wiese added a comment -

          Thanks Paul, submitting again.

          Show
          Damyon Wiese added a comment - Thanks Paul, submitting again.
          Hide
          Damyon Wiese added a comment -

          Thanks for the updates Paul they look correct to me.

          This has been integrated now - I added some commit to fix some indentation issues with the patch.

          Can you make sure this is added to the list of core webservices for 2.5?

          This page:
          http://docs.moodle.org/dev/Web_services_Roadmap

          Thanks, Damyon

          Show
          Damyon Wiese added a comment - Thanks for the updates Paul they look correct to me. This has been integrated now - I added some commit to fix some indentation issues with the patch. Can you make sure this is added to the list of core webservices for 2.5? This page: http://docs.moodle.org/dev/Web_services_Roadmap Thanks, Damyon
          Hide
          Ankit Agarwal added a comment -

          OK (1 test, 9 assertions)
          Passing thanks.

          Show
          Ankit Agarwal added a comment - OK (1 test, 9 assertions) Passing thanks.
          Hide
          Damyon Wiese added a comment -

          Thanks for your hard work. This issue has been integrated upstream and is now available via git (and in some hours, via mirrors and downloads).

          Show
          Damyon Wiese added a comment - Thanks for your hard work. This issue has been integrated upstream and is now available via git (and in some hours, via mirrors and downloads).
          Hide
          Michael de Raadt added a comment -

          Removing the dev_docs_required label as this is now documented in the embedded Web services documentation.

          Show
          Michael de Raadt added a comment - Removing the dev_docs_required label as this is now documented in the embedded Web services documentation.

            People

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

              Dates

              • Created:
                Updated:
                Resolved: