Issue 1518: InstructorCourseEnroll: ignore empty columns

Issue 1518: InstructorCourseEnroll: ignore empty columns

[damith, 2014-03-05 15:22:53]

https://codereview.appspot.com/71530043/diff/1/src/main/java/teammates/common...
File src/main/java/teammates/common/datatransfer/StudentAttributesFactory.java
(right):

https://codereview.appspot.com/71530043/diff/1/src/main/java/teammates/common...
src/main/java/teammates/common/datatransfer/StudentAttributesFactory.java:8:
public class StudentAttributesFactory {
Write a header comment for the class

https://codereview.appspot.com/71530043/diff/1/src/main/java/teammates/common...
src/main/java/teammates/common/datatransfer/StudentAttributesFactory.java:89:
//Here we assume that the enrollLine must have the same numbers of columns as
the header row
Assert the assumption?

https://codereview.appspot.com/71530043/diff/1/src/main/java/teammates/common...
src/main/java/teammates/common/datatransfer/StudentAttributesFactory.java:104:
private int locateColumnIndex(String headerRow) {
This name doesn't fit the purpose. ...Indexes?

https://codereview.appspot.com/71530043/diff/1/src/test/java/teammates/test/c...
File src/test/java/teammates/test/cases/common/StudentAttributesFactoryTest.java
(right):

https://codereview.appspot.com/71530043/diff/1/src/test/java/teammates/test/c...
src/test/java/teammates/test/cases/common/StudentAttributesFactoryTest.java:77:
saf = new StudentAttributesFactory("team|name|email|comment");
Use different valid values e.g. Team, TEAMS etc. 
That is better than repeating the same valid value.

https://codereview.appspot.com/71530043/diff/1/src/test/java/teammates/test/c...
src/test/java/teammates/test/cases/common/StudentAttributesFactoryTest.java:172:
//		int retVal = (Integer) privateMethod.invoke(new StudentAttributesFactory(),
line);
Why commented lines?

https://codereview.appspot.com/71530043/diff/1/src/test/java/teammates/test/c...
File src/test/java/teammates/test/cases/common/StudentAttributesTest.java
(right):

https://codereview.appspot.com/71530043/diff/1/src/test/java/teammates/test/c...
src/test/java/teammates/test/cases/common/StudentAttributesTest.java:50:
assertFalse(invalidStudent.isValid());
This course ID doesn't have a space, although it says "Course Id with space"

[Oo Theong Siang, 2014-03-05 17:07:34]

https://codereview.appspot.com/71530043/diff/1/src/main/java/teammates/common...
File src/main/java/teammates/common/datatransfer/StudentAttributesFactory.java
(right):

https://codereview.appspot.com/71530043/diff/1/src/main/java/teammates/common...
src/main/java/teammates/common/datatransfer/StudentAttributesFactory.java:8:
public class StudentAttributesFactory {
On 2014/03/05 15:22:54, damith wrote:
> Write a header comment for the class

Done.

https://codereview.appspot.com/71530043/diff/1/src/main/java/teammates/common...
src/main/java/teammates/common/datatransfer/StudentAttributesFactory.java:89:
//Here we assume that the enrollLine must have the same numbers of columns as
the header row
On 2014/03/05 15:22:54, damith wrote:
> Assert the assumption?

Just realized that the assumption shouldn't be there. In the case of manual
enrollment and no header row, some rows might have comment column and some might
not have it. Moreover, the preceding if-statement already checks for case where
the required columns are not enough.

https://codereview.appspot.com/71530043/diff/1/src/main/java/teammates/common...
src/main/java/teammates/common/datatransfer/StudentAttributesFactory.java:104:
private int locateColumnIndex(String headerRow) {
On 2014/03/05 15:22:54, damith wrote:
> This name doesn't fit the purpose. ...Indexes?

Done.

https://codereview.appspot.com/71530043/diff/1/src/test/java/teammates/test/c...
File src/test/java/teammates/test/cases/common/StudentAttributesFactoryTest.java
(right):

https://codereview.appspot.com/71530043/diff/1/src/test/java/teammates/test/c...
src/test/java/teammates/test/cases/common/StudentAttributesFactoryTest.java:77:
saf = new StudentAttributesFactory("team|name|email|comment");
On 2014/03/05 15:22:54, damith wrote:
> Use different valid values e.g. Team, TEAMS etc. 
> That is better than repeating the same valid value.

Done.

https://codereview.appspot.com/71530043/diff/1/src/test/java/teammates/test/c...
src/test/java/teammates/test/cases/common/StudentAttributesFactoryTest.java:172:
//		int retVal = (Integer) privateMethod.invoke(new StudentAttributesFactory(),
line);
On 2014/03/05 15:22:54, damith wrote:
> Why commented lines?

My mistakes for not removing them, which are used for double checking purpose.

https://codereview.appspot.com/71530043/diff/1/src/test/java/teammates/test/c...
File src/test/java/teammates/test/cases/common/StudentAttributesTest.java
(right):

https://codereview.appspot.com/71530043/diff/1/src/test/java/teammates/test/c...
src/test/java/teammates/test/cases/common/StudentAttributesTest.java:50:
assertFalse(invalidStudent.isValid());
On 2014/03/05 15:22:54, damith wrote:
> This course ID doesn't have a space, although it says "Course Id with space"

Actually "Course Id with space" itself is the course id. :-P
The courseId argument is passed in as the last argument of the constructor.