Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding another grading method according to attend duration #477

Merged
merged 1 commit into from
Feb 29, 2024

Conversation

fmido88
Copy link
Contributor

@fmido88 fmido88 commented May 1, 2023

  • Grading now according to the duration of meeting attendance
  • set the id with the name for non-registered users so we use it a unique identifier for the user.
  • updating records of participants instate of adding new record in case if the non-registered user left the meeting and return

@smbader smbader added the help wanted We need your help to make this possible label May 4, 2023
@jrchamp
Copy link
Collaborator

jrchamp commented May 8, 2023

Hi @fmido88,

Thank you for sharing your work on grading attendance as a percentage of meeting attendance. This is a very challenging feature to implement and it's clear that you have put a lot of thought into this as there were a few notable resiliencies (such as if a user gets disconnected and needs to reconnect) that you worked hard to add. There are far more risks with percentage attendance grading (because of limitations in Zoom's interfaces) than our current "full grade" approach.

For completeness, I'll list as many as I am aware of (including ones you've addressed):

  1. We need to accurately map meeting participants to Moodle users.
    1. For organizations that have all of their users added to their enterprise-style Zoom account, this is possible because the user accounts are very controlled and meeting reports will include their Zoom account identifier.
    2. If the users do not have Zoom accounts or their Zoom accounts are outside of your organization account, account identifier details will not be available via the reports (for privacy reasons).
  2. We need to determine the amount of time the user was present in the meeting.
    1. If the user joins once and leaves once, then there is only one entry for that user and this is easy.
    2. Each time the user disconnects and rejoins, another entry will exist for the user and these need to be combined in some way.
    3. It is possible for a user to join multiple times simultaneously, such as via a computer and a mobile device. These overlapping times should not be counted twice.
  3. We need to know the effective duration of the meeting.

This implementation attempts to resolve these problems in these ways:

  1. By adding the Moodle user.id to the uname field, the hope is that the reports will reliably include that information.
    • Unfortunately, Zoom does not use the uname query parameter in some cases when a user is already logged in to the Zoom client.
  2. For each "user", adding up the durations of each entry for that user.
    • Unfortunately, this can result in overlapping entries that would give a user extra credit for the extra overlapping time.
    • To accurately calculate the duration, the timeline segments would need to be combined into a single timeline for the user.
  3. The duration of the meeting is based on the scheduled start and end time in Moodle.
    • Unfortunately, if a meeting starts early or late, or ends early or late, this will impact a student's grade.
    • Example: If a student joins an hour-long meeting at 11:00 and the meeting ends at 11:30, the student would get 50%.
    • Example: If a student joins an hour-long meeting at 10:45 and then leaves the meeting at 11:45, the student would get 100% even though they left early.
    • Example: If the instructor starts the meeting at 11:15 and ends the meeting at 12:00, students would get at most 75%.

Changes needed:

  1. Modifying the user display name should not be required, it should be an option that defaults off.
    • We don't want to change the default behavior.
    • The setting should mention all of the limitations of this grading method.
  2. The meeting duration and the user duration timelines need to be aligned.
    • If the meeting is schedule for 11:00 to 12:00, then if the meeting starts after 11:00 or ends before 12:00, those minutes should count for the student. It's not the student's fault the meeting was not active.
    • The student timeline for the duration of the meeting must be unique to avoid counting attendance minutes multiple times.
    • For a meeting that is scheduled from 11:00 to 12:00, minutes that the student joined early or stayed late should not count for the student. The student should not be expected to join outside of the meeting times, nor should they be rewarded for it.

@fmido88
Copy link
Contributor Author

fmido88 commented May 9, 2023

@jrchamp
Thanks for pointing me for these points.
I'll work on these changes in few days and update the fork.

Copy link
Collaborator

@jrchamp jrchamp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely making progress, but I want to make sure that we are very comfortable with the logic, because it will affect grades.

classes/task/get_meeting_reports.php Outdated Show resolved Hide resolved
classes/task/get_meeting_reports.php Outdated Show resolved Hide resolved
locallib.php Outdated Show resolved Hide resolved
locallib.php Outdated Show resolved Hide resolved
lang/en/zoom.php Outdated Show resolved Hide resolved
locallib.php Outdated Show resolved Hide resolved
locallib.php Outdated Show resolved Hide resolved
settings.php Outdated Show resolved Hide resolved
settings.php Outdated Show resolved Hide resolved
classes/task/get_meeting_reports.php Outdated Show resolved Hide resolved
@fmido88
Copy link
Contributor Author

fmido88 commented May 25, 2023

Hello @jrchamp
I think I finished, unless there is any comments or reviews on the code, I'm glad to hear your opinion.

@jrchamp
Copy link
Collaborator

jrchamp commented May 25, 2023

Thanks @fmido88, I can tell you've worked a lot on this. The one unresolved comment is still true: This needs PHPUnit tests. The grading and duration logic should be outside of the data load loop/function so that grade calculation can be done separately, which should make the PHPUnit tests easier to write.

I'm sorry I haven't responded more quickly and this review is not as detailed - it's an extremely busy time for me right now, but I think this should help address the remaining large issues and any small issues can hopefully be folded in at the end.

@fmido88
Copy link
Contributor Author

fmido88 commented May 25, 2023

Thanks @jrchamp for your response.
I got idea of how to put the grading logic outside the loop but not the duration, the best I did is to make it in outside function but not the loop.
The second thought that could solve the problem is to put back the code as it was and create a new database table which contain the calculated durations of users from the participants table, which will be done in another loop outside the data loop function
I think this will be a cleaner process but harder to be done.
I think it the best is the first solution.
What do you think?

@jrchamp
Copy link
Collaborator

jrchamp commented May 25, 2023

I think putting back the data load code as it was (except we should fix the "only delete if we have data" problem, and I have ideas for that). We don't need a new database table to store the calculated durations, because calculating the grades only needs to happen when the data changes for a user (I have an idea for this too).

Rough steps, so I don't forget the ideas:

  1. Load participant data similar to before and only insert new rows, do not ever delete rows.
  2. If the grading method is period, then mark the user's grade_grade as needing to be graded (only the users who have new rows).
  3. After the data load, look up the grade_grades that need to be graded and grade them. For safety, only if the new grade is higher should we update the grade_grade.
    • The duration calculation function will likely use meeting start time, meeting end time and an array of participant data to determine the relevant attendance duration.

@fmido88
Copy link
Contributor Author

fmido88 commented May 26, 2023

Hi again @jrchamp
I'm sorry for these to much commits, I'm hoping that I'm not bothering you.
I'm really doing my best and I just want to contribute.
Could you (in free time) take a look on the new code before I start building the test for it?

If it is ok I'll start making the tests for both functions get_participant_overlap_time() and grading_participant_upon_duration()

@jrchamp
Copy link
Collaborator

jrchamp commented May 26, 2023

You are not bothering me! 😄 I can tell how much you care about doing a good job and I appreciate it. I'm excited about the structure of this new version. I think it's a good idea to move forward with the tests, which will help confirm if the logic is correct. Once you have a test created, I should have more time next week to help.

@fmido88
Copy link
Contributor Author

fmido88 commented May 26, 2023

Thanks @jrchamp
I'll start building the test and definitely I'll need a hand on it because I didn't built a php unit test before 😅, I'll try my best and you could help me perfecting it.

@fmido88
Copy link
Contributor Author

fmido88 commented May 26, 2023

PHPUnit test added to test the overlapping time and the new grading method.
Feel free to check if more cases need to be added to the test.

@fmido88
Copy link
Contributor Author

fmido88 commented May 27, 2023

In the last commit I added a notification function that sends teachers in the course containing the meeting a message about grades.
This only work if the grading method is according duration.
This message also contains names and grades of participants which couldn't be identified as moodle users to be graded manually.

Also updating the PHPUnit test for grading:
1- recalling process_meeting_reports again to check that there in no mess-up with the participants report or grades.
2- testing that teachers receive notification each time calling it.

sorry for any grammatical or spelling mistakes, it will be great if someone check them specially in the language file.

@fmido88
Copy link
Contributor Author

fmido88 commented May 28, 2023

Last thing, is getting list of user clicked join meeting and send those that aren't reconized to teachers to narrow the search for users needs manual grading

I think I'm done if there is no other suggestions or comments in the last modifications

@fmido88
Copy link
Contributor Author

fmido88 commented Jun 12, 2023

@jrchamp Hi again
Let me know once you take a look on the code if it is ok.

Copy link
Collaborator

@jrchamp jrchamp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @fmido88,

There's definitely progress here. I wasn't able to review everything, because it's time for me to go home, but I think these suggestions will help keep this moving along.

classes/task/get_meeting_reports.php Outdated Show resolved Hide resolved
classes/task/get_meeting_reports.php Outdated Show resolved Hide resolved
classes/task/get_meeting_reports.php Outdated Show resolved Hide resolved
classes/task/get_meeting_reports.php Outdated Show resolved Hide resolved
classes/task/get_meeting_reports.php Outdated Show resolved Hide resolved
classes/task/get_meeting_reports.php Outdated Show resolved Hide resolved
classes/task/get_meeting_reports.php Outdated Show resolved Hide resolved
classes/task/get_meeting_reports.php Outdated Show resolved Hide resolved
classes/task/get_meeting_reports.php Outdated Show resolved Hide resolved
lang/en/zoom.php Outdated Show resolved Hide resolved
@fmido88
Copy link
Contributor Author

fmido88 commented Jun 15, 2023

Hi @jrchamp ;
I reviewed the suggestions and modified some, and test results all fine.

Copy link
Collaborator

@jrchamp jrchamp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for incorporating the previous suggestions so quickly. I was able to review some more today, so here's some more.

classes/task/get_meeting_reports.php Outdated Show resolved Hide resolved
classes/task/get_meeting_reports.php Outdated Show resolved Hide resolved
classes/task/get_meeting_reports.php Outdated Show resolved Hide resolved
classes/task/get_meeting_reports.php Show resolved Hide resolved
classes/task/get_meeting_reports.php Outdated Show resolved Hide resolved
lang/en/zoom.php Outdated Show resolved Hide resolved
settings.php Outdated Show resolved Hide resolved
tests/get_meeting_reports_test.php Outdated Show resolved Hide resolved
tests/get_meeting_reports_test.php Outdated Show resolved Hide resolved
classes/task/get_meeting_reports.php Outdated Show resolved Hide resolved
@fmido88
Copy link
Contributor Author

fmido88 commented Jun 16, 2023

All suggestions reviewed. Sorry for my rusty language because English isn't my mother language so as I see there so many corrections for grammar and spelling mistakes.

I considered your suggestion about any user front strings and checking if the user enrolled in the course before adding the score.

I hope the final code is ok, and if there is any more test cases seem to be added , inform me.

@fmido88 fmido88 changed the title changing the grading method Adding another grading method according to attend duration Jul 3, 2023
@fmido88
Copy link
Contributor Author

fmido88 commented Jul 14, 2023

@jrchamp there is a problem with grunt test in form.js but I didn't change any thing in it.

@karenliulll
Copy link
Contributor

Tried reviewing the code changes (sorry for the delay), the logic makes sense to me.

@smbader smbader assigned sgrandh3 and unassigned karenliulll Oct 19, 2023
@sgrandh3
Copy link
Collaborator

Found the following :
1.The settings for grading is admin level, not at the course level
2. The instructor is also being graded
3. The instructor was graded for more than 100%
4. When student did not join, did not see the grade as zero.

@fmido88
Copy link
Contributor Author

fmido88 commented Nov 13, 2023

Found the following : 1.The settings for grading is admin level, not at the course level 2. The instructor is also being graded 3. The instructor was graded for more than 100% 4. When student did not join, did not see the grade as zero.

Hi @sgrandh3!
I updated the code such that:
1- The grading method no exist in the module level not in the admin level.
2- The instructor get more than 100% that is because according to the report the duration of the instructor more than the meeting duration, so now no one gets more than 100%.

What didn't change:
Grading the instructor:
This is also exist in the old grading method so if it is ok I can modify the function zoom_grade_item_update(), but I think that there is no harm for it to be exist.
No join user grade:
As the same as any item else, like quiz or assignment, the user didn't get any grade unless they submit.
So according to both grading methods, if the user didn't click join meeting, no grade added.

@jrchamp
Copy link
Collaborator

jrchamp commented Nov 16, 2023

The backup/restore process will probably need to add the new field to the list of fields that get backed up. Also, gradefor doesn't tell me as a developer what the value/data does. Perhaps grademethod or similar?

@fmido88
Copy link
Contributor Author

fmido88 commented Nov 16, 2023

The backup/restore process will probably need to add the new field to the list of fields that get backed up. Also, gradefor doesn't tell me as a developer what the value/data does. Perhaps grademethod or similar?

In first I thinked about naming it gradingmethod, but there is another field in the mod form with this name in the grading section, so I thought about gradingfor or gradefor, I'll think about something else.

I'll work on backup/resotre code as soon as possible.

@fmido88
Copy link
Contributor Author

fmido88 commented Nov 29, 2023

Hi @jrchamp I updated the code such that changing the field name gradefor to grading_method, also added the field to backup list of fields.

@sgrandh3
Copy link
Collaborator

sgrandh3 commented Dec 18, 2023

What is the expected behavior of this use case: 1. The instructor created a Zoom meeting and started the meeting as per the schedule. The instructor got graded accordingly. 2. After the meeting, the instructor modified the same Zoom meeting to a different time. Now, what should happen to the existing grades?
This could be an edge case, but could happen.

Rest all looks good, did not find any other issues while testing

@fmido88
Copy link
Contributor Author

fmido88 commented Dec 18, 2023

Hello @sgrandh3

The grades should be updated automatically, only for higher grades.
For examples:
The meeting 1:before modification
student 1 joined the meeting and got 9 marks
Student 2 joined the meeting and got 5 marks
Student 3 not joined

After modification
Student 1 joined the meeting and should get 5 marks in the new one which is lower that the previous will not be updated
Student 2 joined the meeting and should get 10 marks which is greater than the previous it will be updated to 10
Student 3 get its marks for joining the new one

@sgrandh3
Copy link
Collaborator

Thank you for the quick reply. @fmido88

- Grade according to the duration of meeting attendance
- set the id with the name for non-registered users so we use it a unique identifier for the user.
- updating records of participants instate of adding new record in case if the non-registered user left the meeting and return

fixing some codes and try to fix phpunit test

add name condition

add a condition for checking the existence of the participant by name along with user id to avoid update existence record if the user id is null

this should pass the phpunit test

adding a condition to check the existence of a data in zoom_meeting_participants by name along with user id to prevent updating records of users with null id.

options for grading methods and display name

- Adding setting to choose the grading method.
- Adding setting to choose the display name.
- Removing the overlapped time when merging a participant record in duration.

get_participant_overlap_time

creating a function that properly calculate the overlap timing in the meeting reports.

Just make the code checker happier.

Update get_meeting_reports.php

forgot to include join_time field.

Creating a function to calculate users grades

- Creating a function to calculate users grades according to their duration in the meeting.
- Fixing the function get_participant_overlap_time().
- Move the code to calculate the grades outside the loop.
- return the original inserting data to the participants table.
- prevent inserting multiple data for exist record.
- not updating grades until the new grade is larger than the old one.

testing the new grading method

Notify teachers about grades

Send teachers a notification about grades in meeting according to duration.

test the notification

update tests

try preventResetByRollback() maybe messages test work!

try to make messages work

changing teacher role to editingteacher

change graders array key

fix message index

fix misspelling

Add and fix comments

remove wrong using class and fix some comments

check the existence of user after integer check

Try not to be spamy

As the task get_meeting_reports run, don't send notifications unless there is new records in meeting participants table

Help teacher to fine ungraded users

Narrowing the options for teachers needing to grade non-recognized students in participant meeting report.

Adding a function to get a list of users clicked (join meeting)
using this function and excluding already recognized students
displaying a list of the rest student in the notification message to teachers

try to solve null readers

try to skip error: Call to a member function get_events_select() on null on PHPUnit test.

phpcs: small cleanups

New grading method is in module level

- Add new field to mod_form to specify the grading method instead of specify it on the admin level.

fix typo and version

Change gradefor field to grading_method

namespace: convert to use statements

use messages

Co-authored-by: Jonathan Champ <[email protected]>
Copy link
Collaborator

@jrchamp jrchamp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that the v5.1 series seems to have stabilized, it's time for v5.2!

@jrchamp jrchamp merged commit 127218b into ncstate-delta:main Feb 29, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Adds new functionality help wanted We need your help to make this possible
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants