Moodle

possible enhancements to sync_metacourse

Details

  • Type: Bug Bug
  • Status: Closed Closed
  • Priority: Major Major
  • Resolution: Fixed
  • Affects Version/s: 1.7
  • Fix Version/s: 2.0
  • Component/s: Roles / Access
  • Labels:
    None
  • Database:
    Any
  • Affected Branches:
    MOODLE_17_STABLE
  • Fixed Branches:
    MOODLE_20_STABLE

Description

I think all the hard work can be done with a couple of database queries. The attached file is a reimplementation.

This certainly runs without errors on Postgres. From reading the docs, I believe MySQL will undersand the two queries, but I have not had time to test yet. I see no reason why other databases should not cope with this SQL.

I think it is doing the right thing, but it would definitely be good if people could code review this.

It runs 10 or 100 times faster than the old code on some of our big metacourses. Manually assigning or unassigning a role in a child course went from 25 seconds to 1.5 on one of our courses. (For OU people, that was courseid 1111 on learnacct.)

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Eloy Lafuente (stronk7) added a comment -

SQL compatibility ok (EXISTS clauses are 100% cross-db) checked against MySQL, MSSQL and Oracle.

Show
Eloy Lafuente (stronk7) added a comment - SQL compatibility ok (EXISTS clauses are 100% cross-db) checked against MySQL, MSSQL and Oracle.
Hide
Permalink
Eloy Lafuente (stronk7) added a comment -

From HQ-Chat (friday):

(just wondering if you need to select ALWAYS the id as FIRST field or you will be losing records in the transformation to ASSOC array)

Show
Eloy Lafuente (stronk7) added a comment - From HQ-Chat (friday): (just wondering if you need to select ALWAYS the id as FIRST field or you will be losing records in the transformation to ASSOC array)
Hide
Permalink
Tim Hunt added a comment -

Right, I have checked my change into HEAD. When we have confidence in it, we need to backport it to the 1.7 stable branch. (The OU will be going live with this code on Thursday, assuming we don't find any bugs.)

Show
Tim Hunt added a comment - Right, I have checked my change into HEAD. When we have confidence in it, we need to backport it to the 1.7 stable branch. (The OU will be going live with this code on Thursday, assuming we don't find any bugs.)
Hide
Permalink
Tim Hunt added a comment -

This patch does the TODO about timestart and timeend, assuming I have understood it correctly.

That is, it gets the timestart and timeend from the child course enrolment, and sets them on the parent course enrolment.

Show
Tim Hunt added a comment - This patch does the TODO about timestart and timeend, assuming I have understood it correctly. That is, it gets the timestart and timeend from the child course enrolment, and sets them on the parent course enrolment.
Hide
Permalink
Tim Hunt added a comment -

This patch is an improvement in the way metacourse managers are handled. Instead of getting a list of userids of people with moodle/course:managemetacourse capability, and not unassigning any role they have, instead we get a list of roles with that capability, and never unassign those roles in the metacourse. That seems more correct to me, but I am not sure how it is supposed to work.

Show
Tim Hunt added a comment - This patch is an improvement in the way metacourse managers are handled. Instead of getting a list of userids of people with moodle/course:managemetacourse capability, and not unassigning any role they have, instead we get a list of roles with that capability, and never unassign those roles in the metacourse. That seems more correct to me, but I am not sure how it is supposed to work.
Hide
Permalink
Martín Langhoff added a comment -

Looks good to me

IIRC, the original metacourse code had a similar SQL implementation of the sync that was not happy with MySQL v3.x so Penny rewrote it to be MySQLv3 compliant.

Show
Martín Langhoff added a comment - Looks good to me IIRC, the original metacourse code had a similar SQL implementation of the sync that was not happy with MySQL v3.x so Penny rewrote it to be MySQLv3 compliant.
Hide
Permalink
Penny Leach added a comment -

+1 !!

Definitely, it had to work with MySQL when it was originally written (against moodle 1.4!)

Show
Penny Leach added a comment - +1 !! Definitely, it had to work with MySQL when it was originally written (against moodle 1.4!)
Hide
Permalink
Petr Škoda (skodak) added a comment -

1/ I have reviewed and tested the code in HEAD, it is IMO ok.

2/ the timestart and timeend is also ok, but it does not solve the case when the dates are changed in child courses AFAIK

3/ the original logic was to allow following case

  • managemetacourse assigned at course category level
  • normal teachers do not have managemetacourse permission
  • metacourse manager assigns himself as teacher directly in metacourse - this would not be IMHO possible with your patch
Show
Petr Škoda (skodak) added a comment - 1/ I have reviewed and tested the code in HEAD, it is IMO ok. 2/ the timestart and timeend is also ok, but it does not solve the case when the dates are changed in child courses AFAIK 3/ the original logic was to allow following case
  • managemetacourse assigned at course category level
  • normal teachers do not have managemetacourse permission
  • metacourse manager assigns himself as teacher directly in metacourse - this would not be IMHO possible with your patch
Hide
Permalink
Tim Hunt added a comment -

The speed issue has been solved. Changing the summary to reflect the other items in this bug.

Show
Tim Hunt added a comment - The speed issue has been solved. Changing the summary to reflect the other items in this bug.
Hide
Permalink
Petr Škoda (skodak) added a comment -

Hello, the metacourse support was reimplemented in 2.0dev, please test and file new issues if necessary.
Thank you for the report and patches.

Petr Skoda

Show
Petr Škoda (skodak) added a comment - Hello, the metacourse support was reimplemented in 2.0dev, please test and file new issues if necessary. Thank you for the report and patches. Petr Skoda

People

Vote (0)
Watch (4)

Dates

  • Created:
    Updated:
    Resolved:
Atlassian JIRA (v4.4.3#663-r165197) | Report a problem
Powered by a free Atlassian JIRA open source license for Moodle. Try JIRA - bug tracking software for your team.