GRBK-689

adding tamsler and constance.i.fuller to reviewers

(hopefully I have the right Constance there)

[tamsler, 2012-02-04 00:38:35]

http://codereview.appspot.com/5618043/diff/1/client/src/java/org/sakaiproject...
File
client/src/java/org/sakaiproject/gradebook/gwt/client/gxt/view/panel/ImportPanel.java
(left):

http://codereview.appspot.com/5618043/diff/1/client/src/java/org/sakaiproject...
client/src/java/org/sakaiproject/gradebook/gwt/client/gxt/view/panel/ImportPanel.java:758:
refreshSetupPanel();
Do we need to adjust the wizard's title. At line 775, we have:

wizard.setHeaderTitle("Scantron");

... so that we either show Scantron or Clicker, or make it generic?

http://codereview.appspot.com/5618043/diff/1/client/src/java/org/sakaiproject...
File
client/src/java/org/sakaiproject/gradebook/gwt/client/gxt/view/panel/ImportPanel.java
(right):

http://codereview.appspot.com/5618043/diff/1/client/src/java/org/sakaiproject...
client/src/java/org/sakaiproject/gradebook/gwt/client/gxt/view/panel/ImportPanel.java:784:
private Double getBoundaryScoreForItem(ItemModel i, List<Learner> rows, boolean
findlower) {
Change boolean "findlower" to "findLower"

http://codereview.appspot.com/5618043/diff/1/client/src/java/org/sakaiproject...
client/src/java/org/sakaiproject/gradebook/gwt/client/gxt/view/panel/ImportPanel.java:805:
if (findlower) {
May want to combine the two if statements to:

if (findlower && boundary == Double.MAX_VALUE) {

    boundary = 0d;
}
else if(boundary == 0d) {
    boundary = 100d;
}

When I first read it, I thought the indentation was off.

http://codereview.appspot.com/5618043/diff/1/server/src/java/org/sakaiproject...
File
server/src/java/org/sakaiproject/gradebook/gwt/server/ImportExportUtilityImpl.java
(right):

http://codereview.appspot.com/5618043/diff/1/server/src/java/org/sakaiproject...
server/src/java/org/sakaiproject/gradebook/gwt/server/ImportExportUtilityImpl.java:1075:
private boolean isClickerSheetForJExcelApi(Sheet s) {
This seems to be dead code

http://codereview.appspot.com/5618043/diff/1/server/src/java/org/sakaiproject...
server/src/java/org/sakaiproject/gradebook/gwt/server/ImportExportUtilityImpl.java:1086:
private boolean isClickerSheetFromPoi(org.apache.poi.ss.usermodel.Sheet sheet) {
This seems to be dead code

http://codereview.appspot.com/5618043/diff/1/server/src/java/org/sakaiproject...
server/src/java/org/sakaiproject/gradebook/gwt/server/ImportExportUtilityImpl.java:1100:
private boolean isClickerHeaderRowPoi(Row row) {
This seems to be dead code. Only called from above method

http://codereview.appspot.com/5618043/diff/1/server/src/java/org/sakaiproject...
server/src/java/org/sakaiproject/gradebook/gwt/server/ImportExportUtilityImpl.java:1110:
private boolean poiRowContainsString(Row row, String text) {
This seems to be dead code. Only called from above method.

http://codereview.appspot.com/5618043/diff/1/server/src/java/org/sakaiproject...
server/src/java/org/sakaiproject/gradebook/gwt/server/ImportExportUtilityImpl.java:2155:
if (categoryRow == null  || categoryRow.length < 3)
What is the significance of 3. Can you add a comment please?

http://codereview.appspot.com/5618043/diff/1/shared/src/java/org/sakaiproject...
File
shared/src/java/org/sakaiproject/gradebook/gwt/client/DataTypeConversionUtil.java
(left):

http://codereview.appspot.com/5618043/diff/1/shared/src/java/org/sakaiproject...
shared/src/java/org/sakaiproject/gradebook/gwt/client/DataTypeConversionUtil.java:126:
public static String unpackItemIdFromKey(String key) {
Yes we need to change this, or at least we need to add a comment showing what
the key looks like

http://codereview.appspot.com/5618043/diff/1/shared/src/java/org/sakaiproject...
File
shared/src/java/org/sakaiproject/gradebook/gwt/client/I18nConstants.properties
(right):

http://codereview.appspot.com/5618043/diff/1/shared/src/java/org/sakaiproject...
shared/src/java/org/sakaiproject/gradebook/gwt/client/I18nConstants.properties:91:
gb2ImportScantronSameName=An item with that name already exists. We have changed
the item name so that it will be imported uniquely. If you wanted to replace the
item currently in the gradebook then please change the name back. (Note: if you
target the 'Unassigned' category then a new item will be created regardless.)
Let's review this during Training & Support meeting.

[tamsler, 2012-03-23 21:09:23]

FindBugs found a potential NPE. Please see comment.

http://codereview.appspot.com/5618043/diff/1/client/src/java/org/sakaiproject...
File
client/src/java/org/sakaiproject/gradebook/gwt/client/gxt/view/panel/ImportPanel.java
(right):

http://codereview.appspot.com/5618043/diff/1/client/src/java/org/sakaiproject...
client/src/java/org/sakaiproject/gradebook/gwt/client/gxt/view/panel/ImportPanel.java:800:
boundary = d>boundary ? d : boundary;
do we need to handle the case where d==boundary?

http://codereview.appspot.com/5618043/diff/1/server/src/java/org/sakaiproject...
File
server/src/java/org/sakaiproject/gradebook/gwt/server/ImportExportUtilityImpl.java
(right):

http://codereview.appspot.com/5618043/diff/1/server/src/java/org/sakaiproject...
server/src/java/org/sakaiproject/gradebook/gwt/server/ImportExportUtilityImpl.java:1115:
for (org.apache.poi.ss.usermodel.Cell cell : row) {
FingBugs flags this as potential NPE because we null check "row" above.