Skip to content

Commit

Permalink
SAK-46093 GBNG > import > user data loss when navigating backwards th…
Browse files Browse the repository at this point in the history 
…rough the wizard
  • Loading branch information
@plukasew @ern
plukasew authored and ern committed Aug 31, 2021
1 parent f6730e3 commit dfc348b
Show file tree
Hide file tree
Showing 3 changed files with 161 additions and 79 deletions.
54 changes: 54 additions & 0 deletions gradebookng/tool/src/java/org/sakaiproject/gradebookng/business/util/ImportGradesHelper.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,8 @@
import com.opencsv.CSVReader;
import com.opencsv.CSVReaderBuilder;
import com.opencsv.exceptions.CsvValidationException;
import java.util.Collections;
import java.util.Set;

import lombok.extern.slf4j.Slf4j;

Expand Down Expand Up @@ -355,6 +357,16 @@ public static boolean setupImportWizardModelForSelectionStep(ImportExportPage so
return false;
}

// Track selections if coming back from the item creation steps
List<ProcessedGradeItem> oldSelections = importWizardModel.getSelectedGradeItems();
Set<String> selectedGbItemTitles = Collections.emptySet();
Set<String> selectedCommentTitles = Collections.emptySet();
if (oldSelections != null)
{
selectedGbItemTitles = oldSelections.stream().filter(p -> p.getType() == ProcessedGradeItem.Type.GB_ITEM).map(ProcessedGradeItem::getItemTitle).collect(Collectors.toSet());
selectedCommentTitles = oldSelections.stream().filter(p -> p.getType() == ProcessedGradeItem.Type.COMMENT).map(ProcessedGradeItem::getItemTitle).collect(Collectors.toSet());
}

// If there are duplicate headings, tell the user now
boolean hasValidationErrors = false;
HeadingValidationReport headingReport = spreadsheetWrapper.getHeadingReport();
Expand Down Expand Up @@ -474,6 +486,48 @@ public static boolean setupImportWizardModelForSelectionStep(ImportExportPage so
sourcePage.clearFeedback();
sourcePage.updateFeedback(target);

// If returning from the creation pages, the ProcessedGradeItems in importWizardModel are now stale and don't reflect changes made in the creation pages. Update them matching on getItemTitle()
if (!selectedGbItemTitles.isEmpty() || !selectedCommentTitles.isEmpty())
{
Map<String, Assignment> titlesToAssignments = new HashMap<>();
Map<ProcessedGradeItem, Assignment> staleAssignmentsToCreate = importWizardModel.getAssignmentsToCreate();
for (Map.Entry<ProcessedGradeItem, Assignment> entry : staleAssignmentsToCreate.entrySet())
{
titlesToAssignments.put(entry.getKey().getItemTitle(), entry.getValue());
}

List<ProcessedGradeItem> selectedGradeItems = new ArrayList<>();
Map<ProcessedGradeItem, Assignment> assignmentsToCreate = new LinkedHashMap<>();
for (ProcessedGradeItem item : processedGradeItems)
{
String title = item.getItemTitle();
switch (item.getType())
{
case COMMENT:
if (selectedCommentTitles.contains(title))
{
selectedGradeItems.add(item);
}
break;
case GB_ITEM:
if (selectedGbItemTitles.contains(title))
{
selectedGradeItems.add(item);
}

Assignment toCreate = titlesToAssignments.get(title);
if (toCreate != null)
{
assignmentsToCreate.put(item, toCreate);
}
break;
}
}

importWizardModel.setSelectedGradeItems(selectedGradeItems);
importWizardModel.setAssignmentsToCreate(assignmentsToCreate);
}

// Setup and return the model
importWizardModel.setProcessedGradeItems(processedGradeItems);
importWizardModel.setUserReport(userReport);
Expand Down
162 changes: 83 additions & 79 deletions ...l/src/java/org/sakaiproject/gradebookng/tool/panels/importExport/CreateGradeItemStep.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@
*/
package org.sakaiproject.gradebookng.tool.panels.importExport;

import java.util.Collection;
import java.util.List;
import java.util.Map;

Expand Down Expand Up @@ -98,52 +97,7 @@ public void onInitialize() {

@Override
protected void onSubmit(final AjaxRequestTarget target, final Form<?> form) {

final Assignment newAssignment = (Assignment) form.getDefaultModel().getObject();
final ImportExportPage page = (ImportExportPage) getPage();
log.debug("GradebookAssignment: {}", newAssignment);

// validate name is unique, first among existing gradebook items, second against new items to be created
boolean validated = true;
final List<Assignment> existingAssignments = CreateGradeItemStep.this.businessService.getGradebookAssignments();
if (!assignmentNameIsUnique(existingAssignments, newAssignment.getName())
|| !assignmentNameIsUnique(newAssignment, importWizardModel.getAssignmentsToCreate().values())) {
validated = false;
error(getString("error.addgradeitem.title"));
page.updateFeedback(target);
}

if (validated) {

// sync up the assignment data so we can present it for confirmation
processedGradeItem.setItemTitle(newAssignment.getName());
processedGradeItem.setItemPointValue(String.valueOf(newAssignment.getPoints()));
processedGradeItem.setRubricParameters(getRubricParameters(""));

// add to model
importWizardModel.getAssignmentsToCreate().put(processedGradeItem, newAssignment);

// Figure out if there are more steps
// If so, go to the next step (ie do it all over again)
Component newPanel;
if (step < importWizardModel.getTotalSteps()) {
importWizardModel.setStep(step + 1);
newPanel = new CreateGradeItemStep(CreateGradeItemStep.this.panelId, Model.of(importWizardModel));
} else {
// If not, continue on in the wizard
newPanel = new GradeImportConfirmationStep(CreateGradeItemStep.this.panelId, Model.of(importWizardModel));
}

// clear any previous errors
page.clearFeedback();
page.updateFeedback(target);

// AJAX the new panel into place
newPanel.setOutputMarkupId(true);
final WebMarkupContainer container = page.container;
container.addOrReplace(newPanel);
target.add(newPanel);
}
saveItemAndProceed(true, target, form);
}

@Override
Expand All @@ -159,33 +113,15 @@ protected void onError(final AjaxRequestTarget target, final Form<?> form) {

@Override
public void onSubmit(final AjaxRequestTarget target, final Form<?> form) {
saveItemAndProceed(false, target, form); // OWL
}

// clear any previous errors
@Override
protected void onError(final AjaxRequestTarget target, final Form<?> form) {
final ImportExportPage page = (ImportExportPage) getPage();
page.clearFeedback();
page.updateFeedback(target);

// Create the previous panel
Component previousPanel;
if (step > 1) {
importWizardModel.setStep(step - 1);
previousPanel = new CreateGradeItemStep(CreateGradeItemStep.this.panelId, Model.of(importWizardModel));
} else {
// Reload everything. Rationale: final step can have partial success and partial failure. If content was imported from
// the spreadsheet, the item selection page should reflect this when we return to it
ImportGradesHelper.setupImportWizardModelForSelectionStep(page, CreateGradeItemStep.this, importWizardModel,
CreateGradeItemStep.this.businessService, target);
previousPanel = new GradeItemImportSelectionStep(CreateGradeItemStep.this.panelId, Model.of(importWizardModel));
}

// AJAX the previous panel into place
previousPanel.setOutputMarkupId(true);
final WebMarkupContainer container = page.container;
container.addOrReplace(previousPanel);
target.add(container);
}
};
backButton.setDefaultFormProcessing(false);
form.add(backButton);

final AjaxButton cancelButton = new AjaxButton("cancelbutton") {
Expand All @@ -211,6 +147,75 @@ public void onSubmit(final AjaxRequestTarget target, final Form<?> form) {
form.add(this.previewGradesPanel);
}

private void saveItemAndProceed(boolean forward, final AjaxRequestTarget target, final Form<?> form)
{
final ImportWizardModel importWizardModel = model.getObject();
final int step = importWizardModel.getStep();
// original data
final ProcessedGradeItem processedGradeItem = importWizardModel.getItemsToCreate().get(step - 1);

final Assignment newAssignment = (Assignment) form.getDefaultModel().getObject();
final ImportExportPage page = (ImportExportPage) getPage();
log.debug("GradebookAssignment: {}", newAssignment);

// validate name is unique, first among existing gradebook items, second against new items to be created
boolean validated = true;
final List<Assignment> existingAssignments = CreateGradeItemStep.this.businessService.getGradebookAssignments();
if (!assignmentNameIsUnique(existingAssignments, newAssignment.getName()) || !assignmentNameIsUnique(newAssignment)) {
validated = false;
error(getString("error.addgradeitem.title"));
page.updateFeedback(target);
}

if (validated) {

// sync up the assignment data so we can present it for confirmation
processedGradeItem.setAssignmentTitle(newAssignment.getName()); // need to retain the original title for matching later
processedGradeItem.setItemPointValue(String.valueOf(newAssignment.getPoints())); // doesn't seem like anything actually uses this, but will leave it
processedGradeItem.setRubricParameters(getRubricParameters(""));

// add to model
importWizardModel.getAssignmentsToCreate().put(processedGradeItem, newAssignment);

Component newPanel;
if (forward)
{
// Figure out if there are more steps
// If so, go to the next step (ie do it all over again)
if (step < importWizardModel.getTotalSteps()) {
importWizardModel.setStep(step + 1);
newPanel = new CreateGradeItemStep(CreateGradeItemStep.this.panelId, Model.of(importWizardModel));
} else {
// If not, continue on in the wizard
newPanel = new GradeImportConfirmationStep(CreateGradeItemStep.this.panelId, Model.of(importWizardModel));
}
}
else // back
{
if (step > 1) {
importWizardModel.setStep(step - 1);
newPanel = new CreateGradeItemStep(CreateGradeItemStep.this.panelId, Model.of(importWizardModel));
} else {
// Reload everything. Rationale: final step can have partial success and partial failure. If content was imported from
// the spreadsheet, the item selection page should reflect this when we return to it
ImportGradesHelper.setupImportWizardModelForSelectionStep(page, CreateGradeItemStep.this, importWizardModel,
CreateGradeItemStep.this.businessService, target);
newPanel = new GradeItemImportSelectionStep(CreateGradeItemStep.this.panelId, Model.of(importWizardModel));
}
}

// clear any previous errors
page.clearFeedback();
page.updateFeedback(target);

// AJAX the new panel into place
newPanel.setOutputMarkupId(true);
final WebMarkupContainer container = page.container;
container.addOrReplace(newPanel);
target.add(newPanel);
}
}

/**
* Checks if an assignment is unique given a list of existing assignments
*
Expand All @@ -233,18 +238,17 @@ private boolean assignmentNameIsUnique(final List<Assignment> assignments, final
* @param assignmentsToCreate
* @return
*/
private boolean assignmentNameIsUnique(final Assignment newAssignment, final Collection<Assignment> assignmentsToCreate) {
boolean retVal = true;
private boolean assignmentNameIsUnique(final Assignment newAssignment) {

for (final Assignment assignmentToCreate : assignmentsToCreate) {
final List<ProcessedGradeItem> itemsToCreate = model.getObject().getItemsToCreate();
final Map<ProcessedGradeItem, Assignment> assignmentsToCreate = model.getObject().getAssignmentsToCreate();

// Skip comparison of itself; if newAssignment name equals assignmentToCreate name, name is not unique
if (!newAssignment.equals(assignmentToCreate) && StringUtils.equals(newAssignment.getName(), assignmentToCreate.getName())) {
retVal = false;
break;
}
}
// assignment equality is based on id, which will be null until the assignments are persisted, so we can't use it
// instead, we determine if the assignment object for the current item is already in the map, and set our expected
// number of name matches accordingly
final ProcessedGradeItem thisItem = itemsToCreate.get(model.getObject().getStep() - 1);
int expectedNameMatchesIfUnique = assignmentsToCreate.get(thisItem) == null ? 0 : 1;

return retVal;
return assignmentsToCreate.values().stream().filter(a -> StringUtils.equals(newAssignment.getName(), a.getName())).count() == expectedNameMatchesIfUnique;
}
}
24 changes: 24 additions & 0 deletions ...a/org/sakaiproject/gradebookng/tool/panels/importExport/GradeItemImportSelectionStep.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@

import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
Expand Down Expand Up @@ -149,6 +150,28 @@ public void onClick(final AjaxRequestTarget target) {
final Map<String, ProcessedGradeItem> commentMap = createCommentMap(allItems);
final Map<String, ProcessedGradeItem> gradeItemMap = createGradeItemMap(allItems);

// Set up the previous selections, if any
final List<ProcessedGradeItem> selectedItems = importWizardModel.getSelectedGradeItems() == null ? Collections.emptyList() : importWizardModel.getSelectedGradeItems();
for (ProcessedGradeItem prevSelection : selectedItems)
{
final String title = prevSelection.getItemTitle();
ProcessedGradeItem item = null;
switch (prevSelection.getType())
{
case GB_ITEM:
item = gradeItemMap.get(title);
break;
case COMMENT:
item = commentMap.get(title);
break;
}

if (item != null && item.isSelectable())
{
item.setSelected(true);
}
}

final Form<?> form = new Form("form");
add(form);

Expand Down Expand Up @@ -223,6 +246,7 @@ protected void onSubmit(AjaxRequestTarget target, Form<?> form) {
log.debug("Items to modify: {}", itemsToModify.size());

// set data for next page
importWizardModel.setSelectedGradeItems(itemsToProcess); // This is grades and comments now
importWizardModel.setItemsToCreate(itemsToCreate);
importWizardModel.setItemsToUpdate(itemsToUpdate);
importWizardModel.setItemsToModify(itemsToModify);
Expand Down

0 comments on commit dfc348b

Please sign in to comment.