-
-
Notifications
You must be signed in to change notification settings - Fork 165
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
add controls for when grades are sent to the LMS #2617
Conversation
I suggest that the defaults should be |
I just looked at your changes with |
I don't think I intentionally changed anything with how/when |
@drgrice1 Wouldn't my suggested defaults above do just that? Grades will be sent once attempted (from the first time they hit submit), or after the closed date? |
Do I understand this right? If there is a set of 10 questions, and Just seeing Jaimos's post. Is the behavior you want achieved by using This is reminding me that I will have to change the message students get about whether or not (and when) their score will be sent to the LMS. I hadn't considered that. |
But I too found it confusing that |
Oh I see, you do not want the score passed back during mass grade passback, and using |
Did I change something with |
OK, I read my own words and I see it. I was acting on the idea that whatever calls the routines in So we will want to know if these routines were called my mass passback or by a passback upon submit. |
c10b84b
to
26946c7
Compare
@Alex-Jordan I almost commented on this, but then thought the defaults I suggested would work for me, so left it alone. I did find the wording a little confusing, "This is subject to the above settings." makes it sound like you meant that now the grade will only pass back if it meets the conditions set in |
Okay, I just double checked, and yes these settings do affect |
@Alex-Jordan ahh I see you answered your own question while I was typing and I missed it. One thing this already appears to do is make the configuration a bit more complicated. |
Right, I set the defaults so that nothing should change. [Technically, there are currently some unrealistic situations where a grade would be submitted before the set opens, and this PR would change things in those unrealistic situations and no longer send grades.] Three things can trigger grade passback:
They all call the subroutines in But I could make the three things behave differently and that won't be hard. The question is though, that what @drgrice1 is requesting is not what I would want, even if I were using How about if
I don't have a preference about defaults. For my purposes I'll just set the settings to what I would like them to be. So whether defaults preserve current behavior or move to a new better behavior, whatever people want is good. |
Yes, that is what I want. In fact, I realized that all I really want is when mass grade pushback occurs is a grade for a test that has no versions is not sent until after the due/course date. The biggest issue that keeps happening is a student opens a test from the LMS but doesn't start it. So now Webwork has what it needs to pass back grades, but no version exists. Then mass pass back submits a zero, and I get an email from a student asking me to re open the test for them (they don't bother to check to see that they can still go into the test and take it). Note that an empty score from the LMS is not zero, and for LTI 1.1, even $LTICheckPrior doesn't prevent grade pass back. |
As I mentioned in the previous pull request, this is already known in all of the SubmitGrade.pm methods (for both LTI 1.1 and 1.3). If |
I was somewhat surprised to find that:
does not cause an error. It results in With LTI 1.3 at least, I confirmed that Yeah I don't know about LTI 1.1. I also wonder if the LMS might pass back different things for different LMSs when the grade is null in the LMS. I can't test that with our D2L instance anymore. |
For LTI 1.1, the LMS returns XML containing |
So there is a bug in the LTI 1.3 submit_grade method in the case that a user has opened a test, but not started a test. When mass grade passback occurs and |
Sorry about that. I just added a commit that I think addresses it. I'll work on the |
I don't have a setup where I can test this, but I'm okay with additional configuration options (even if it is a bit confusing at first). As for |
I have a question about this. Suppose a person is using "course" grade passback mode. What I've done so far means that when an overall course grade is calculated to send to the LMS, it will only factor in those sets where the threshold ('attempted' or some score) has been reached, or for which it is past the Note that during any mass passback, this is how the grade calculation will be made. @drgrice1 has requested that possibly with the right configuration, that if a student submits an answer to a set or grades a test, then grades should be passed back regardless of that set's date or threshold being met. What does this mean for a set
Either:
(1) is pointless because the score will omit the set that triggered the passback. (2) and (3) will change the score in the LMS. But then later, suppose there is a mass update and this set has still not met its threshold or reached its So should this feature that @drgrice1 would like to have be unavailable for "course" passback mode? |
I agree with your analysis, If you want to add a feature that Also, would it be worth while extending this beyond LMS passback. What about in the WeBWorK gradebook itself, these settings could control what sets are included in the total grade for users who show that to students (just a thought). |
I'm just going to restate my above suggestion, I think since we are adding these features we should update the defaults to be |
Here is my new write-up to document the intended behavior of all these variables. I'm looking for feedback before I return to checking the behavior matches this description.
|
Overall sounds good, a few things I notice over my first quick read through.
|
I tried to use GH tools to address a small merge conflict that arose. Maybe a mistake, maybe not. Later (this weekend?) I will do testing with the functionality of all this again. If you can confirm you tested with LTI 1.3 with Canvas and Moodle, I may be OK to try this all out with D2L and LTI 1.3. I just have to try it out in production. |
It looks like you fixed the merge conflict correctly. I have tested this with Moodle, but I haven't gotten around to testing with Canvas yet. I will do that as soon as I can. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have now tested this with Canvas. Everything seems to be working well. If you feel like it you could add debugging messages as suggested in my comments.
? grade_gateway($db, $userSet, $userSet->set_id, $userID) | ||
: (grade_set($db, $userSet, $userID, 0))[ 0, 1 ]); | ||
my $score = getSetPassbackScore($db, $ce, $userID, $userSet, !$self->{post_processing_mode}); | ||
return -1 unless $score; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might want to add a debug message here. I was just testing this with Canvas, and found it useful to determine that a grade was not sent due to it not meeting the requirements. Something like
return -1 unless $score; | |
if ($submittedSet && !getSetPassbackScore($db, $ce, $userID, $submittedSet, 1)) { | |
$self->warning('It is not after the $LTISendScoresAfterDate and the $LTISendGradesEarlyThreshold was not met. ' | |
. 'Not updating grade.'); | |
return -1; | |
} |
I am not sold on using the variable names in the message, although this is just a debugging message. Perhaps just use words?
Also a similar message above for the submit_course_grade
case would be good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm just checking if there was importance behind the condition in the if
in your comment here. I have something like this now but it's just if (!$score)
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this was inside the submit_course_grade
method. Currently line 136 in LTIAdvanced/SubmitGrade.pm
and line 218 of LTIAdvantage/SubmitGrade.pm
which is return -1 if $submittedSet && !getSetPassbackScore($db, $ce, $userID, $submittedSet, 1);
there is no message.
This looks like it is about ready to go to me. I can't fully test this as I don't have a development system using LTI, but the defaults and ideas look good to me. @Alex-Jordan do you want to update the final debugging suggestions? |
I am wandering if the For the following, I assume that LTI 1.3 is in use. Assume that it is the beginning of the semester, and a student enters webwork via a link in the LMS, but doesn't submit any answers for anything. So now webwork has everything it needs to pass grades back for this student. Now assume that a mass grade pass back occurs. The |
I would not want to check the course grade against the numerical threshold. The score could be lower than the threshold and you may still want that course score to go to the LMS simply because various critical dates have passed. Here is an idea I have not thought through yet (and I have to leave for the day soon for PT conferences). What if Getting |
That sounds like a reasonable solution. |
As long as I'm changing that, is there any reason to make it more than a simple count? An alternative is that it could be an array reference with references to each of the included user set objects. |
One reason I see where that may be helpful is that I can add a log message listing the sets that were included in the calculation when the overall course score is sent to the LMS. It could be a bit of a long log message. What do you think? |
Sounds fine to me. Certainly, the list of included user set objects would generally be more useful, and provide much more information. |
…user sets that were included in the scoring
OK, I added a commit that does that. Hopefully it is easy to just review that commit. Also I caught an error during testing that is fixed in a separate commit. Thirdly, I notices that in "homework" mode, the log would have entries like I did a fairly thorough round of testing with LTI 1.1. Things with LTI 1.3 are not really different now, as far as how this PR goes now. I haven't tested with LTI 1.3, but I feel OK that if it works for LTI 1.1, it will work for LTI 1.3. The necessary differences in the two |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure that I like that the messages Submitting score for $userID...
were moved down. For debugging purposes it is good to have a message in any case. Furthermore, there are now messages that occur earlier then this message that refer to the user. For example, the message "LMS user id is not available for this user" in the submit_set_grade
method for LTI 1.3 now occurs with no reference to which user the message regards. So if this message is moved down, then the messages before that need to now include the $userID
.
I think moving the message down also makes the messages I previously suggested for when a grade is not submitted necessary.
I could move the messages back but then I think they need to change what they say. The log was looking like:
And this was happening in situations where no grade was in fact submitted. Then there was no other message related to an LTI grade passback attempt, and so the implication was something was indeed sent to the LMS. I think that would be pretty misleading to someone reading the logs while debugging/configuring. |
I agree, but that is why I had previously suggested adding messages in the case that grades were not sent. You don't need to move the messages back, but the other messages need to be updated to accommodate the change. The messages that say there is missing pass back data for the student need to include the |
You could also move the message back, but change it to say something like "Considering submitting grade for $userID", although perhaps better wording. Hopefully you get the idea from that poorly chosen wording though. |
2c87c17
to
a717243
Compare
OK, take a look now at the debug messages. Sorry, before I had not seen your comments about those from Nov 21. Also I reverted to letting |
Yeah, since nothing is using the Now all of the |
5b31fb8
to
c5fdb38
Compare
return await $self->submit_grade($user->lis_source_did, $score); | ||
} else { | ||
$self->warning("No sets for user $userID meet criteria to be included in course grade calculation."); | ||
return 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be return -1
. A return value of 0 generally means that something went wrong with the grade pass back process. A return value of 1 means success (not sending a grade because it was unchanged with $LTICheckPrior
is considered success). A return value of -1 means that the grade was not passed back because the criteria were not met which is what is happening in this case.
The different return values give a different type of messaged in the passbackGradeOnSubmit
method. For a return value of -1, the message is just "'Your score will be sent to the LMS at a later time." It isn't appropriate to say that the grade will be passed back in some specified amount of time, because at that time the criteria may not be met still, and so the grade may still not be passed back.
The same change of course is needed at this place in the LTI 1.3 method.
Thanks for fixing that. I am going to merge this. |
This is a reworking of #2614. Some discussion there may be helpful but imo this can be considered a new PR. As with that PR though, I can not test this with D2L until it's less risky, because for now I can only test in production. If anyone else can test in Canvas or Moodle, and things get to the point they are working, I will test with D2L. I apologize in advance for my mistakes! I can at least say I tested that
webwork2
starts, and nothing goes bad when I visit pages.A few changes are packaged here. The big things are:
$LTISendScoresAfterDate
can be set, for example, to"close_date"
. It's described in the newconf/authen_LTI.conf.dist
.$LTISendGradesEarlyThreshold
can be set to a number from 0 to 1, or'attempted'
. It's described inconf/authen_LTI.conf.dist
.There are 2x2 pathways to consider when implementing these. First, is it LTI 1.1 or 1.3? And then, is grade passback in
"course"
or"homework"
mode? For"homework"
passback, the work is almost entirely in the twoSubmitGrade.pm
files. Specifically in thesubmit_set_grade
subroutines. There is a small reliance on three new utilities inlib/WeBWorK/Utils/Sets.pm
.For
"course"
grade passback, the work happens inlib/WeBWorK/Utils/Sets.pm
with itsgrade_all_sets
subroutine, where it always has. But now with parameters sent so thatgrade_all_sets
can understand the two new config settings and act accordingly.Additional changes concerning
$LTICheckPrior
:$LTICheckPrior
to the list of LTI config options that could be enabled on the Config page$LTICheckPrior
is set and somehow an LMS score is like 0.9999, it would not update to 1 even when the WeBWorK score is now 1. So I changed it to ignore a small nonzero difference if the WW score is 1.$LTICheckPrior
, suppose an LMS score is null and the corresponding WW score is 0. Previously, the LMS would not be updated to 0. Now it will be updated to 0 if we are past the new$LTISendScoresAfterDate
.