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

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

Merged
merged 4 commits into from
Jun 3, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@

package org.sakaiproject.assignment.api;

import java.util.List;

/**
* Store the constants used by Assignment tool and service
*
Expand Down Expand Up @@ -385,5 +387,8 @@ public enum IMSGradingProgress {
public static final String SAK_PROP_ALLOW_LINK_TO_EXISTING_GB_ITEM = "assignment.allowLinkToExistingGBItem";
public static final boolean SAK_PROP_ALLOW_LINK_TO_EXISTING_GB_ITEM_DFLT = true;

public static final String SAK_PROP_NON_SUBMITTER_PERMISSIONS = "assignment.submitter.remove.permission";
public static final List<String> SAK_PROP_NON_SUBMITTER_PERMISSIONS_DEFAULT = List.of(AssignmentServiceConstants.SECURE_ADD_ASSIGNMENT);

public static final String ASSIGNMENT_INPUT_ADD_SUBMISSION_TIME_SPENT = "value_ASSIGNMENT_INPUT_ADD_SUBMISSION_TIME_SPENT";
}
Original file line number Diff line number Diff line change
Expand Up @@ -530,6 +530,7 @@ public interface AssignmentService extends EntityProducer {

public boolean permissionCheck(String permission, String resource, String user);

public boolean permissionCheckInGroups(String permission, Assignment assignment, String user);
/**
* Access the internal reference which can be used to assess security clearance.
*
Expand Down
18 changes: 18 additions & 0 deletions assignment/api/src/resources/assignment.properties
Original file line number Diff line number Diff line change
Expand Up @@ -1311,3 +1311,21 @@ tags_option_students_default=Default (hide tags to students)
tags_option_students_yes=Show tags to students and allow them to use the filter component

select=Select

datatables.aria.orderable=Activate to sort
datatables.aria.orderableRemove=Activate to remove sorting
datatables.aria.orderableReverse=Activate to invert sorting
datatables.aria.sortAscending=Activate to sort column ascending
datatables.aria.sortDescending=Activate to sort column descending
datatables.custom.first=First
datatables.custom.last=Last
datatables.custom.next=Next
datatables.custom.previous=Previous
datatables.emptyTable=No entries found
datatables.entries=entries
datatables.info=Showing _START_ to _END_ of _TOTAL_ entries
datatables.infoEmpty=Showing 0 to 0 of 0 entries
datatables.infoFiltered=(filtered from _MAX_ total entries)
datatables.lengthMenu=Show _MENU_ entries
datatables.search=Search:
datatables.zeroRecords=No matching entries found
18 changes: 18 additions & 0 deletions assignment/api/src/resources/assignment_ca.properties
Original file line number Diff line number Diff line change
Expand Up @@ -1281,3 +1281,21 @@ tags_search=Cercar usant etiquetes
tags_option_students=Mostrar etiquetes als estudiants
tags_option_students_default=Predeterminat (ocultar etiquetes als estudiants).
tags_option_students_yes=Mostrar etiquetes als estudiantes i permetre'ls usar el filtre.

datatables.aria.orderable=Activar per ordenar
datatables.aria.orderableRemove=Activar per eliminar l\u0027ordenaci\u00f3
datatables.aria.orderableReverse=Activar per invertir l\u0027ordenaci\u00f3
datatables.aria.sortAscending=Activar per ordenar columna de forma ascendent
datatables.aria.sortDescending=Activar per ordenar columna de forma descendent
datatables.custom.first=Primer
datatables.custom.last=\u00daltim
datatables.custom.next=Seg\u00fcent
datatables.custom.previous=Anterior
datatables.emptyTable=No s\u0027han trobat entrades
datatables.entries=entrades
datatables.info=Mostrant _START_ a _END_ de _TOTAL_ entrades
datatables.infoEmpty=Mostrant 0 a 0 de 0 entrades
datatables.infoFiltered=(filtrat de _MAX_ total entrades)
datatables.lengthMenu=Mostrar _MENU_ entrades
datatables.search=Cercar:
datatables.zeroRecords=No s\u0027han trobat entrades coincidents
18 changes: 18 additions & 0 deletions assignment/api/src/resources/assignment_es.properties
Original file line number Diff line number Diff line change
Expand Up @@ -1274,3 +1274,21 @@ tags_search=Buscar usando etiquetas
tags_option_students=Mostrar etiquetas a los estudiantes.
tags_option_students_default=Por defecto (ocultar etiquetas a los estudiantes).
tags_option_students_yes=Mostrar etiquetas a los estudiantes y permitirles el uso del filtro.

datatables.aria.orderable=Activar para ordenar
datatables.aria.orderableRemove=Activar para quitar ordenaci\u00f3n
datatables.aria.orderableReverse=Activar para invertir ordenaci\u00f3n
datatables.aria.sortAscending=Activar para ordenar columna de forma ascendente
datatables.aria.sortDescending=Activar para ordenar columna de forma descendente
datatables.custom.first=Primero
datatables.custom.last=\u00daltimo
datatables.custom.next=Siguiente
datatables.custom.previous=Anterior
datatables.emptyTable=No se encontraron entradas
datatables.entries=entradas
datatables.info=Mostrando _START_ a _END_ de _TOTAL_ entradas
datatables.infoEmpty=Mostrando 0 a 0 de 0 entradas
datatables.infoFiltered=(filtrado de _MAX_ total entradas)
datatables.lengthMenu=Mostrar _MENU_ entradas
datatables.search=Buscar:
datatables.zeroRecords=No se encontraron entradas coincidentes
Original file line number Diff line number Diff line change
Expand Up @@ -3237,6 +3237,10 @@ public boolean permissionCheck(String permission, String resource, String user)
return access;
}

public boolean permissionCheckInGroups(String permission, Assignment assignment, String user) {
return this.permissionCheckWithGroups(permission, assignment, user);
}

private boolean permissionCheckWithGroups(final String permission, final Assignment assignment, final String user) {
if (StringUtils.isBlank(permission) || assignment == null) return false;
String siteReference = siteService.siteReference(assignment.getContext());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,7 @@
import java.util.SortedSet;
import java.util.concurrent.ConcurrentSkipListSet;
import java.util.function.Function;
import java.util.function.Predicate;
import java.util.regex.Matcher;
import java.util.regex.Pattern;
import java.util.stream.Collectors;
Expand Down Expand Up @@ -1086,6 +1087,10 @@ public class AssignmentAction extends PagedResourceActionII {
* To know if grade_submission go from view_students_assignment view or not
**/
private static final String FROM_VIEW = "from_view";
/**
* Whether or not to reset the table state stored on the frontend
*/
private static final String RESET_TABLE_STATE = "reset_table_state";

/**
* Site property for export rubric in pdf
Expand Down Expand Up @@ -1113,6 +1118,7 @@ public class AssignmentAction extends PagedResourceActionII {
private static final String CONTEXT_GO_NEXT_UNGRADED_ENABLED = "goNextUngradedEnabled";
private static final String CONTEXT_GO_PREV_UNGRADED_ENABLED = "goPrevUngradedEnabled";
private static final String PARAMS_VIEW_SUBS_ONLY_CHECKBOX = "chkSubsOnly1";
private static final String EXPANDED_USER_ID = "expandedUserId";
private static ResourceLoader rb = new ResourceLoader("assignment");
private boolean nextUngraded = false;
private boolean prevUngraded = false;
Expand Down Expand Up @@ -5712,18 +5718,27 @@ private String build_instructor_view_students_assignment_context(VelocityPortlet
// cleaning from view attribute
state.removeAttribute(FROM_VIEW);

// Null or true means we want to reset the tables state
boolean resetTableState = !Boolean.FALSE.equals(state.getAttribute(RESET_TABLE_STATE));
context.put("resetTableState", resetTableState);
state.setAttribute(RESET_TABLE_STATE, false);

String contextString = (String) state.getAttribute(STATE_CONTEXT_STRING);
Site site;
try {
site = siteService.getSite(contextString);
} catch (IdUnusedException e) {
throw new IllegalStateException("Can not build context for invalid site with id [" + contextString + "]");
}

initViewSubmissionListOption(state);
String allOrOneGroup = (String) state.getAttribute(VIEW_SUBMISSION_LIST_OPTION);
String search = (String) state.getAttribute(VIEW_SUBMISSION_SEARCH);
Boolean searchFilterOnly = (state.getAttribute(SUBMISSIONS_SEARCH_ONLY) != null && ((Boolean) state.getAttribute(SUBMISSIONS_SEARCH_ONLY)) ? Boolean.TRUE : Boolean.FALSE);

String accessPointUrl = serverConfigurationService.getAccessUrl() +
AssignmentReferenceReckoner.reckoner().context(contextString).reckon().getReference() +
"?contextString=" + contextString +
"&viewString=" + allOrOneGroup +
"&searchString=" + search +
"&searchFilterOnly=" + searchFilterOnly.toString() +
"&estimate=true";
context.put("accessPointUrl", accessPointUrl);
Expand All @@ -5739,25 +5754,40 @@ private String build_instructor_view_students_assignment_context(VelocityPortlet
}
context.put("hasAtLeastOneAnonAssignment", hasAtLeastOneAnonAssigment);

Map<String, User> studentMembers = assignments.stream()
// flatten to a single List<String>
.flatMap(a -> assignmentService.getSubmitterIdList(searchFilterOnly.toString(), allOrOneGroup, search, AssignmentReferenceReckoner.reckoner().assignment(a).reckon().getReference(), contextString).stream())
// collect into set for uniqueness
.collect(Collectors.toSet()).stream()
// convert to User
.map(s -> {
try {
return userDirectoryService.getUser(s);
} catch (UserNotDefinedException e) {
log.warn("User is not defined {}, {}", s, e.getMessage());
return null;
}
})
// filter nulls
.filter(Objects::nonNull)
// collect to Map<String, User>
.collect(Collectors.toMap(User::getId, Function.identity()));
List<String> nonSubmitterPermissions = serverConfigurationService.getStringList(AssignmentConstants.SAK_PROP_NON_SUBMITTER_PERMISSIONS,
AssignmentConstants.SAK_PROP_NON_SUBMITTER_PERMISSIONS_DEFAULT);

Predicate<String> isNonSubmitter = (userId) -> nonSubmitterPermissions.stream()
.filter(permission -> securityService.unlock(userId, permission, site.getReference()))
.findAny()
.isEmpty();

AuthzGroup groupSelection = StringUtils.isBlank(allOrOneGroup) || AssignmentConstants.ALL.equals(allOrOneGroup)
? site
: site.getGroup(allOrOneGroup);

Map<String, User> studentMembers = groupSelection.getUsers().stream()
.filter(isNonSubmitter)
.map(userDirectoryService::getOptionalUser)
.flatMap(Optional::stream)
.collect(Collectors.toMap(User::getId, Function.identity()));

Comparator<Assignment> assignmentComparator = new AssignmentComparator(state, SORTED_BY_DEFAULT, Boolean.TRUE.toString());

Map<User, Iterator<Assignment>> showStudentAssignments = new HashMap<>();
Set<String> expandedStudents = (Set<String>) state.getAttribute(STUDENT_LIST_SHOW_TABLE);
if (expandedStudents != null) {
context.put("studentListShowSet", expandedStudents);
for (String userId : expandedStudents) {
Set<Assignment> userSubmittableAssignments = assignments.stream()
.filter(Predicate.not(assignmentService::assignmentUsesAnonymousGrading))
.collect(Collectors.toSet());

showStudentAssignments.put(studentMembers.get(userId), userSubmittableAssignments.iterator());
}
}

context.put("expandedUserId", state.getAttribute(EXPANDED_USER_ID));
context.put("studentMembersMap", studentMembers);
context.put("studentMembers", new SortedIterator(studentMembers.values().iterator(), new AssignmentComparator(state, SORTED_USER_BY_SORTNAME, Boolean.TRUE.toString())));
context.put("viewGroup", state.getAttribute(VIEW_SUBMISSION_LIST_OPTION));
Expand All @@ -5766,25 +5796,6 @@ private String build_instructor_view_students_assignment_context(VelocityPortlet
Collection groups = getAllGroupsInSite(contextString);
context.put("groups", new SortedIterator(groups.iterator(), new AssignmentComparator(state, SORTED_BY_GROUP_TITLE, Boolean.TRUE.toString())));

Map<User, Iterator<Assignment>> showStudentAssignments = new HashMap<>();

Set<String> showStudentListSet = (Set<String>) state.getAttribute(STUDENT_LIST_SHOW_TABLE);
if (showStudentListSet != null) {
context.put("studentListShowSet", showStudentListSet);
for (String userId : showStudentListSet) {
User user = studentMembers.get(userId);

// 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());
Comment on lines -5777 to -5780
Copy link
Contributor

Choose a reason for hiding this comment

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

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...

Copy link
Contributor

Choose a reason for hiding this comment

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

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


// sort the assignments into the default order before adding
Iterator assignmentSortFinal = new SortedIterator(rv.iterator(), new AssignmentComparator(state, SORTED_BY_DEFAULT, Boolean.TRUE.toString()));

showStudentAssignments.put(user, assignmentSortFinal);
}
}

context.put("studentAssignmentsTable", showStudentAssignments);
context.put("currentTime", Instant.now());
Expand Down Expand Up @@ -11017,7 +11028,9 @@ public void doGrade_submission(RunData data) {
String assignmentId = params.getString("assignmentId");
state.setAttribute(EXPORT_ASSIGNMENT_REF, assignmentId);
String submissionId = params.getString("submissionId");

String userId = params.getString("user_id");
state.setAttribute(EXPANDED_USER_ID, userId);

// SAK-29314 - put submission information into state
boolean viewSubsOnlySelected = stringToBool((String) data.getParameters().getString(PARAMS_VIEW_SUBS_ONLY_CHECKBOX));
putSubmissionInfoIntoState(state, assignmentId, submissionId, viewSubsOnlySelected);
Expand Down Expand Up @@ -11224,6 +11237,7 @@ public void doShow_student_submission(RunData data) {
ParameterParser params = data.getParameters();

String id = params.getString("studentId");
state.setAttribute(EXPANDED_USER_ID, id);
// add the student id into the table
t.add(id);

Expand All @@ -11240,6 +11254,7 @@ public void doHide_student_submission(RunData data) {
ParameterParser params = data.getParameters();

String id = params.getString("studentId");
state.removeAttribute(EXPANDED_USER_ID);
// remove the student id from the table
t.remove(id);

Expand Down
75 changes: 75 additions & 0 deletions assignment/tool/src/webapp/js/assignmentsByStudent.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
(() => {

// Include datatables dependencies
window.includeWebjarLibrary('datatables');
window.includeWebjarLibrary('datatables-rowgroup');

// Make sure assignments namespace exists
window.assignments = window.assignments ?? {};

// Assignments By Students "global" namespace
window.assignments.byStudent = {
datatablesConfig: {
dom: '<<".dt-header-row"<".dt-header-slot">lf><t><".dt-footer-row"ip>>',
stateSave: true,
columnDefs: [
{
sortable: false,
targets: "no-sort",
},
],
rowGroup: {
dataSrc(row) {
const dataCellHtml = row[0].display;
return parseDataCell(dataCellHtml).studentUserId;
},
startRender(rows, group) {
const firstRow = rows.data()[0];
const dataCellHtml = firstRow[0].display;

const data = parseDataCell(dataCellHtml);

return renderGrouping(data);
},
},
},
}

// Private functions

function parseDataCell(html) {
const template = document.createElement('template');
template.innerHTML = html;
const cell = template.content.children[0];

const expanded = cell.getAttribute("data-expanded") == "true";
const actionLink = cell.getAttribute("data-action-href");
const studentUserId = cell.getAttribute("data-user-id");
const studentName = cell.innerText.trim();

return {
actionLink,
expanded,
studentName,
studentUserId,
};
}

function renderGrouping({ studentName, actionLink, expanded, studentUserId }) {
const template = document.createElement('template');
template.innerHTML = `
<tr>
<td class="border-0">
<a href="${actionLink}" id="${studentUserId}">
<span class="expand-icon si ${expanded ? "si-expanded" : "si-collapsed"}"
aria-hidden="true"></span>
<span>${studentName}</span>
</a>
</td>
</tr>
`;

return template.content;
}

})();