SAK-49998 Assignments improve performance and UX in assignments by student view #12539

stetsche · 2024-04-24T13:16:34Z

https://sakaiproject.atlassian.net/browse/SAK-49998

bgarciaentornos

Fantastic work, thanks!

I just added some minor requests plus a few "think aloud" comments that probably don't need addressing.

bgarciaentornos · 2024-04-25T07:04:34Z

assignment/tool/src/java/org/sakaiproject/assignment/tool/AssignmentAction.java

-                // filter to obtain only grade-able assignments
-                List<Assignment> rv = assignments.stream()
-                        .filter(a -> assignmentService.allowGradeSubmission(AssignmentReferenceReckoner.reckoner().assignment(a).reckon().getReference()))
-                        .collect(Collectors.toList());


Can we keep the comment? The variable is probably self-explaining but at the same time the comment gives an extra bit of purpose imo.

I was also wondering if the sort is necessary here, but yep, it's probably the best point. Assignments might not come ordered from the findAssignmentsBySite query and afterwards is just the loop so...

If the hasAtLeastOneAnonAssigment has to be kept we could maybe merge both loops through the assignment variable?

bgarciaentornos · 2024-04-25T07:13:57Z

...gnment/tool/src/webapp/vm/assignment/chef_assignments_instructor_student_list_submissions.vm

-			#if( !$hasAtLeastOneAnonAssignment )
-				#searchFilterPanel("$form_search", $searchString, "doView_submission_list_search", "doView_submission_list_search_clear")
-			#end


Can this actually be removed? The context variable is still added.

This removes the search field, which is replaced by the datatables search. I'll check if we can remove the context variable, or if it's used elsewhere

We might still need the variable and display a banner based on that condition.

bgarciaentornos · 2024-04-25T07:15:34Z

...gnment/tool/src/webapp/vm/assignment/chef_assignments_instructor_student_list_submissions.vm

+							#if ($!isTimesheet)
+								<td headers="time"></td>
+							#end
+							<td headers="grade"></td>


Fix spacing

bgarciaentornos · 2024-04-25T07:17:35Z

...gnment/tool/src/webapp/vm/assignment/chef_assignments_instructor_student_list_submissions.vm

+							#end
+							<td headers="grade"></td>
+							</tr>
+						#else
 							#set($assignments=false)
 							#set($assignments=$!studentAssignmentsTable.get($member))
 							#foreach ($assignment in $!assignments)
 								#set ($assignmentReference = $!service.assignmentReference($assignment.Id))
 								#set ($isAnon = $!service.assignmentUsesAnonymousGrading($assignment))


Wondering if this could be done in a more efficient way, probably not and not worth it anyway.

Also checking the file there's this code that appear twice and maybe we could put together to avoid extra calls to the service:

#set ($submissionSubmitter = $!service.getSubmissionSubmittee($submission)) #if ($!submissionSubmitter.isPresent())

velocity/tool/src/templates/VM_chef_library.vm

assignment/tool/src/webapp/js/assignmentsByStudent.js

adrianfish · 2024-04-25T10:10:39Z

assignment/tool/src/java/org/sakaiproject/assignment/tool/AssignmentAction.java

@@ -16757,4 +16773,12 @@ private String getRateSubmissionTimeSpent(AssignmentSubmission submission) {
        }
        return assignmentService.intToTime(assigmentRateSpent);
    }
+
+    private Optional<User> getUser(String userId) {


You could move this into the UserDirectoryService. Maybe use the method "getOptionalUser"? We could perhaps use that naming pattern as a consistent way of overloading existing methods like getUser.

Yeah, I think it makes sense, but I didn't want to go so far. I'll do it.

Maybe we could keep it inline in the stream (was it like that?), it's a tad uglier but doesn't need kernel updates

I have a special "Master adjustments" commit on this branch, I would be fine with adding it there as well. Because I think this pattern makes sense and will be introduced at some point anyway. If we anticipate it now, we won't have duplicated code later. For 22 based branched this::getOptionalUser and master userDirectoyService::getOptionalUser. What do you think?

It's fine. I would rephrase it as "Master master adjustments" though

Agree with everyone here that adding a new method UserDirectoyService::getOptionalUser to return an Optional aka
public Optional<User> getOptionalUser(String userId)

I approve of the method name here, and the original method should be annotated as deprecated. (it's a shame that the return type is not part of a method signature)

this also has a positive side effect that the Exception UserNotDefinedException does not need to be thrown. TBH this was what returning NULL or Optional<> is for. An Exception is for communicating to the callee of the method with a return value other than Objects presence or absence is not to be trusted.

I did add the new method to UserDirectoryService, but I did not deprecate the original one yet

ern · 2024-04-26T16:03:32Z

Nice changes @stetsche

stetsche · 2024-04-29T06:47:39Z

I could address some of the comments, but I have to move to a different project now. One of my colleagues will finish this. We also have found some cases to fix when testing.

stetsche

Hey @JuanDavid102, thanks for continuing work on this! I added a few comments below.

assignment/tool/src/java/org/sakaiproject/assignment/tool/AssignmentAction.java

...gnment/tool/src/webapp/vm/assignment/chef_assignments_instructor_student_list_submissions.vm

…tudent view

…udent

stetsche

@JuanDavid102 I added your latest changes and everything is looking good. Thanks for the great work!

Also rebased on master.

stetsche requested a review from bgarciaentornos April 24, 2024 13:16

bgarciaentornos requested changes Apr 25, 2024

View reviewed changes

stetsche commented Apr 25, 2024

View reviewed changes

velocity/tool/src/templates/VM_chef_library.vm Outdated Show resolved Hide resolved

adrianfish requested changes Apr 25, 2024

View reviewed changes

ern changed the title ~~Assignments Improve performance and UX of Assignments by student view~~ SAK-49998 Assignments Improve performance and UX of Assignments by student view Apr 26, 2024

ern changed the title ~~SAK-49998 Assignments Improve performance and UX of Assignments by student view~~ SAK-49998 Assignments improve performance and UX in assignments by student view Apr 26, 2024

stetsche force-pushed the SAK-49998 branch from 0db8d70 to 10452d9 Compare April 29, 2024 06:44

stetsche marked this pull request as ready for review May 17, 2024 13:02

stetsche commented May 20, 2024

View reviewed changes

Markus Stetschnig and others added 4 commits May 24, 2024 19:03

SAK-49998 Assignments: Improve performance and UX of Assignments by s…

84f40fc

…tudent view

SAK-49998 Master adjustments

a03ff59

SAK-49998 Assignments improve performance and UX in assignments by st…

7f82ded

…udent

SAK-49998 Use method reference

bc9bb68

stetsche force-pushed the SAK-49998 branch from 5ebdf53 to bc9bb68 Compare May 24, 2024 17:03

stetsche commented May 24, 2024

View reviewed changes

bgarciaentornos requested review from ern and adrianfish May 29, 2024 07:37

bgarciaentornos approved these changes May 29, 2024

View reviewed changes

ern merged commit 9854dae into sakaiproject:master Jun 3, 2024
4 of 5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SAK-49998 Assignments improve performance and UX in assignments by student view #12539

SAK-49998 Assignments improve performance and UX in assignments by student view #12539

stetsche commented Apr 24, 2024

bgarciaentornos left a comment

bgarciaentornos Apr 25, 2024

bgarciaentornos Apr 25, 2024

bgarciaentornos Apr 25, 2024

stetsche Apr 25, 2024 •

edited

stetsche Apr 29, 2024

bgarciaentornos Apr 25, 2024

bgarciaentornos Apr 25, 2024

bgarciaentornos Apr 25, 2024

adrianfish Apr 25, 2024

stetsche Apr 26, 2024

bgarciaentornos Apr 26, 2024 •

edited

stetsche Apr 26, 2024

bgarciaentornos Apr 26, 2024

ern Apr 26, 2024

stetsche Apr 29, 2024

ern commented Apr 26, 2024

stetsche commented Apr 29, 2024

stetsche left a comment

stetsche left a comment

SAK-49998 Assignments improve performance and UX in assignments by student view #12539

SAK-49998 Assignments improve performance and UX in assignments by student view #12539

Conversation

stetsche commented Apr 24, 2024

bgarciaentornos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stetsche Apr 25, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bgarciaentornos Apr 26, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ern commented Apr 26, 2024

stetsche commented Apr 29, 2024

stetsche left a comment

Choose a reason for hiding this comment

stetsche left a comment

Choose a reason for hiding this comment

stetsche Apr 25, 2024 •

edited

bgarciaentornos Apr 26, 2024 •

edited