Closed Syxton closed 8 years ago
@Hiroshi-p Yes
@Syxton, I reviewed this code. Do you observe that user removal scenario does not trigger user_enrolment_deleted event, but triggers user_enrolment_updated event? Also, do you observe that both events are fired for normal unerolment scenario (user is still active, but pulled from the course)?
@Hiroshi-p I didn't check if user removal also removes role, but if a role is deactivated, or reactivated, or changed (via external enrollment for example) that is when the enrolment_updated event fires
@Syxton, We need to clarify it, otherwise I cannot determine the consitency between this change and existing rolling sync code. If you may do that and come up with complete change, that's great. If not, Panopto will look into it whne we visit rolling sync code next time.
@Hiroshi-p I've gone through and tested the following: Upon course creation: coursecreated is triggered Upon enroll of user: enrollmentcreated AND roleadded is triggered Upon unassigning role: roledeleted is triggered Upon reassigning role: roleadded is triggered Upon setting enrollment status to suspended: enrolmentupdated is triggered Upon setting enrollment status to active: enrolmentupdated is triggered Upon unenroll of user: roledeleted AND enrollmentdeleted is triggered
@Syxton Thank you very much for the detail report. That is extremely helpful! Based on your information, enrolmentupdated event comes into play only for suspended/reactivated case. That sounds unrelated to #61, rather #62. I would separate out these two issues. Could you create a separate pull request for #62 with only the changes needed for #62?
Now, let's focus on #61. Which part is needed to fix this problem? According to your report, unenroll scenario calls both roledeleted and enrollmentdeleted. It looks like enrollmentdeted ends up with update_user's enroll_remove case and remove_course_user is called. Did you find this does not work correctly for the scenario of #61?
I am sorry for extremely picky review. The primary reason for this is that I am very worried about the change in panopto_data.php. I have not been able to determine if this is the right thing or this won't cause any regression.
@Hiroshi-p Thank you..I'll separate the two requests in a bit.
This PR is now migrated.
@Syxton Thank you for sending this. Am I correct that this is for https://github.com/Panopto/Moodle-2.0-plugin-for-Panopto/issues/61 ?