GRBK-680 2nd Review

[tamsler, 2011-05-02 23:57:45]

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

http://codereview.appspot.com/4438084/diff/1/server/src/java/org/sakaiproject...
server/src/java/org/sakaiproject/gradebook/gwt/sakai/Gradebook2ComponentServiceImpl.java:3908:

in the next line, we check if category is null, yet here we just reference it.

[jnajdi_exchange.vt.edu, 2011-05-03 12:59:59]

Yes. you are right. Thanks.  To prevent a null pointer, line 3907 needs to be
inside the if statement. 

Once the review is completed. I'll provide a new patch addressing all issues
raised. 

Thanks, 
Jihane

On May 2, 2011, at 7:57 PM, tamsler@gmail.com wrote:

> Reviewers: jpgorrono_ucdavis.edu, mjwenk_ucdavis.edu,
> 
> 
>
http://codereview.appspot.com/4438084/diff/1/server/src/java/org/sakaiproject...
> File
>
server/src/java/org/sakaiproject/gradebook/gwt/sakai/Gradebook2ComponentServiceImpl.java
> (right):
> 
>
http://codereview.appspot.com/4438084/diff/1/server/src/java/org/sakaiproject...
>
server/src/java/org/sakaiproject/gradebook/gwt/sakai/Gradebook2ComponentServiceImpl.java:3908:
> 
> in the next line, we check if category is null, yet here we just
> reference it.
> 
> 
> 
> Please review this at http://codereview.appspot.com/4438084/
> 
> Affected files:
>   M      
>
server/src/java/org/sakaiproject/gradebook/gwt/sakai/Gradebook2ComponentServiceImpl.java
> 
> 
> Index:  
>
server/src/java/org/sakaiproject/gradebook/gwt/sakai/Gradebook2ComponentServiceImpl.java
> ===================================================================
> ---  
>
server/src/java/org/sakaiproject/gradebook/gwt/sakai/Gradebook2ComponentServiceImpl.java	

> (revision 73976)
> +++  
>
server/src/java/org/sakaiproject/gradebook/gwt/sakai/Gradebook2ComponentServiceImpl.java	

> (working copy)
> @@ -3680,7 +3680,7 @@
>  		if (droppedValues != null) {
>  			isDropped = droppedValues.get(assignmentId) != null ?  
> droppedValues.get(assignmentId): false;
>  		} else {
> -			isDropped = gradeRecord == null ? false : gradeRecord.isDropped() !=  
> null && gradeRecord.isDropped().booleanValue();
> +			isDropped = gradeRecord == null ? false :  
> Util.checkBoolean(gradeRecord.isDropped());
>  		}
> 
>  		if (isDropped || isExcused)
> @@ -3878,7 +3878,7 @@
>  		}
> 
>  		//GRBK-680 - 'Give Ungraded No Credit' - Zeros given in scores not  
> marked when dropped
> -		Map<Long, Boolean> droppedValues = getDroppedValues(userRecord,  
> categories);
> +		Map<Long, Boolean> droppedValues = getDroppedValues(gradebook,  
> userRecord, categories);
>  		
>  		if (assignments != null) {
> 
> @@ -3900,20 +3900,54 @@
>  		return new LearnerImpl(cellMap);
>  	}
>  	
> -	private Map<Long, Boolean> getDroppedValues(UserRecord userRecord,  
> List<Category> categories) {
> -
> +	private Map<Long, Boolean> getDroppedValues(Gradebook gradebook,  
> UserRecord userRecord, List<Category> categories) {
>  		Map<Long, Boolean> droppedValues = new HashMap<Long, Boolean>();
>  		if (categories != null) {
>  			for (Category category: categories) {
> -				if (category != null && !category.isRemoved()  
> && !category.isUnweighted()&& !category.isExtraCredit() &&  
> category.getDrop_lowest() > 0) {
> +				Boolean isExtraCredit = Util.checkBoolean(category.isExtraCredit());
> +				
> +				if (category != null && !category.isRemoved() && !isExtraCredit &&  
> category.getDrop_lowest() > 0) {
> +					
>  					List<Assignment> assignmentsByCategory = category.getAssignmentList();
> -					droppedValues.putAll(isDropped(assignmentsByCategory, userRecord,  
> category.getDrop_lowest()));
> +					
> +					// Currently GB allows drop lowest score when categories are weighted  
> or when all the items within a category have the same possible point.
> +					// When GRBK-480 (Allow dropped score in category when all items  
> don't have same point value) is deployed this portion of the code needs to  
> be
> +					// modified to reflect that.
> +					Boolean enforcePointWeighting =  
> Util.checkBoolean(category.isEnforcePointWeighting());
> +					if ((isGradebookWeighted(gradebook) && !enforcePointWeighting) ||  
> hasEqualPossiblePoints(assignmentsByCategory)) {
> +						droppedValues.putAll(isDropped(assignmentsByCategory, userRecord,  
> category.getDrop_lowest()));
> +					}
>  				}
>  			}
>  		}
>  		return droppedValues;
>  	}
> +	
> +	private boolean hasEqualPossiblePoints(
> +			List<Assignment> assignmentsByCategory) {
> +		
> +		Double lastPointValue = null;
> 
> +		for (Assignment assignment : assignmentsByCategory) {
> +			if (assignment.isRemoved() || assignment.isNotCounted())
> +				continue;
> +
> +			// Exclude extra credit items from determining whether drop lowest  
> should be allowed
> +			if (Util.checkBoolean(assignment.isExtraCredit()))
> +				continue;
> +
> +			if (lastPointValue != null && assignment.getPointsPossible() != null  
> && !lastPointValue.equals(assignment.getPointsPossible())) {
> +				return false;
> +			}
> +			lastPointValue = assignment.getPointsPossible();
> +		}
> +		return true;
> +	}
> +
> +	private boolean isGradebookWeighted(Gradebook gradebook) {
> +		return GradebookService.CATEGORY_TYPE_WEIGHTED_CATEGORY ==  
> gradebook.getCategory_type();
> +	}
> +
>  	private Map<Long, Boolean> isDropped(List<Assignment>  
> assignmentsByCategory, UserRecord userRecord, int dropLowest) {
>  		Map<Long, Boolean> droppedValues = new HashMap<Long, Boolean>();
>  		Map<Long, AssignmentGradeRecord> studentGradeMap = null;
> @@ -3929,29 +3963,45 @@
>  		int numberOfUnitsDropped = 0;
> 
>  		// start with populating the values form the existing student records
> -		if (studentGradeMap != null) {
> -			for (Assignment assignment : assignmentsByCategory) {
> -				if (assignment.getId() != null) {
> +		for (Assignment assignment : assignmentsByCategory) {
> +			
> +			if (assignment != null && assignment.getId() != null) {
> +				boolean isCountNullsAsZeros =  
> Util.checkBoolean(assignment.getCountNullsAsZeros());
> +			
> +				if (studentGradeMap != null) {
>  					AssignmentGradeRecord assignmentGradeRecord =  
> studentGradeMap.get(assignment.getId());
> +				
>  					if (assignmentGradeRecord != null) {
> -						Boolean isDropped = assignmentGradeRecord.isDropped();
> +						Boolean isDropped =  
> Util.checkBoolean(assignmentGradeRecord.isDropped());
> +					
>  						if (isDropped != null){
>  							droppedValues.put(assignment.getId(), isDropped);
> +						
>  							if (isDropped) {
>  								numberOfUnitsDropped++;
>  							}
>  						} else {
>  							droppedValues.put(assignment.getId(), false);
>  						}
> +					} else if (!isCountNullsAsZeros) {
> +						// not include "ungraded" assignments in drop values
> +						droppedValues.put(assignment.getId(), false);
>  					}
> +				
> +				} else if (!isCountNullsAsZeros) {
> +					// not include "ungraded" assignments in drop values
> +					droppedValues.put(assignment.getId(), false);
>  				}
>  			}
>  		}
> -		//populate the rest of the records
> +			
> +		//populate the rest of the records without student records
>  		for (Assignment assignment : assignmentsByCategory) {
> -			if (assignment.getId() != null && droppedValues.get(assignment.getId())  
> == null) {
> -				//!unit.isExcused()
> -				if (assignment.isCounted() && numberOfUnitsDropped < dropLowest  
> && !assignment.isExtraCredit()) {
> +			
> +			if (assignment != null && assignment.getId() != null &&  
> droppedValues.get(assignment.getId()) == null) {
> +				boolean isExtraCredit = Util.checkBoolean(assignment.isExtraCredit());
> +			
> +				if (assignment.isCounted() && numberOfUnitsDropped < dropLowest  
> && !isExtraCredit) {
>  					droppedValues.put(assignment.getId(), true);
>  					numberOfUnitsDropped ++;
>  				} else {
> @@ -3963,7 +4013,6 @@
>  		return droppedValues;
> 
>  	}
> -	
>  	private Learner buildStudentRow(Gradebook gradebook, UserRecord  
> userRecord, List<FixedColumn> columns, List<Assignment> assignments,  
> List<Category> categories, boolean isShowWeighted) {
>  		return buildLearnerGradeRecord(gradebook, userRecord, columns,  
> assignments, categories, isShowWeighted);
>  	}
> 
>