Spreadsheet Interface for Student Data UI: Section field should not be compulsory during enrollment #8994 #8996
Conversation
tanhengyeow
force-pushed
the
8994-section-field-not-compulsory
branch
from
487a7de
to
821b8ad
Compare
|
@wkurniawan07 @tran-tien-dat Ready for review. |
|
Let's try to include this in the upcoming release @xpdavid FYI |
| @@ -90,7 +90,7 @@ private void testEnrollAction() throws Exception { | |||
| InstructorCourseEnrollResultPage resultsPage = enrollPage.enroll(enrollString); | |||
|
|
|||
| // This is the full HTML verification for Instructor Course Enroll Results Page, the rest can all be verifyMainHtml | |||
| resultsPage.verifyHtml("/instructorCourseEnrollPageResult.html"); | |||
| resultsPage.verifyHtml("/instructorCourseEnrollFirstPageResult.html"); | |||
There was a problem hiding this comment.
Are there better names for the 2 pages beside "First" and "Second"?
There was a problem hiding this comment.
Hmm I can't think of any that won't result in a long file name. Any other possible suggestions?
There was a problem hiding this comment.
I think it's okay for the names to be long, since they are already pretty long and some other HTML file names are pretty long too. Can just be sth like ...AllFields or ...EmptySection
| if (columns[sectionColumnIndex].isEmpty()) { | ||
| hasSection = false; | ||
| } | ||
|
|
There was a problem hiding this comment.
I don't like this approach, as I think these has... variables should purely reflect the status of the header row. You can just add a && !columns[sectionColumnIndex].isEmpty() to the check below.
|
Ready for review again. |
tran-tien-dat
added
s.FinalReview
| if (hasSection && columns.length > sectionColumnIndex) { | ||
| /* Since the migration to spreadsheet interfaces, section header index will always be present. | ||
| Therefore, if the user chooses not to include a section entry in the student row during enrollment, | ||
| we treat it that the student has no section. */ |
There was a problem hiding this comment.
Ideally, a future reader of this comment should not need to know there was a data migration in the past. Code comments should explain the current state irrespective of what happened in the past.
|
Ready to merge :) |
wkurniawan07
added
s.ToMerge
xpdavid
added
the
c.Feature
Fixes #8994
Outline of Solution
StudentAttributesFactoryto address empty sections during enrollment.InstructorCourseEnrollPageUiTestto verify success of enrolling students with no sections.