Support for Extended Matching Item (R-type) MCQs

Activity

SAKTRUNK-37 15

Keyboard shortcuts  
Summarize the review outcomes (optional)
 
#permalink

Details

Warning: no files are visible, they have all been filtered.
Participant Role Time Spent Comments Latest Comment
Jaques Smith (deleted user)
Author 2h 46m 6 Thanks Anthony and Lydia.
Reviewer - 15% reviewed 37m 3 There is some discussion of this on the sakai-pmc list wh...
Reviewer - 0% reviewed 29m 2 I agree with everything that Anthony said https://crucibl...
Reviewer - 0% reviewed 6m    
Reviewer - 0% reviewed      
Reviewer - 3% reviewed 18m 3 Hi Jaques, The way Sakai patches are usually done is that...
Diana Lee Perpich (deleted user)
Reviewer - 0% reviewed      
Reviewer - 0% reviewed 6m    
Anand Mehta (deleted user)
Reviewer - 0% reviewed      
Total   4h 22m 15  
#permalink

Objectives

There are no specific objectives for this review.

Branches in review

#permalink

Issues Raised From Comments

Key Summary State Assignee
#permalink

General Comments

Sam Ottenhoff

This is a pretty massive patch with lots of whitespace changes, comment chang...

This is a pretty massive patch with lots of whitespace changes, comment changes, import changes, etc., that are most likely not essential for this functionality. I don't see any way we can consider this patch for Sakai 10 unless this patch is made substantially smaller.

Jaques Smith (deleted user)

Yip, this patch is big. The import changes is mostly where stuff was not need...

Yip, this patch is big. The import changes is mostly where stuff was not needed and where we added collections we used interfaces and generics from the start. The comment changes was to update out of data comments.

This is massive, and will take time. I know there are some extra language bundles that I have not removed (please let me know if I should) but otherwise the code is needed for EMI's.

Jaques Smith (deleted user)

I do see a few things that I can cleanup. I was so focused on the functionali...

I do see a few things that I can cleanup.
I was so focused on the functionality I did miss some merge clutter.

I'm cleaning up now.

Sam Ottenhoff

Is there a way to separate out your general improvements to Samigo from the m...

Is there a way to separate out your general improvements to Samigo from the minimum functionality needed to implement the extending matching questions?

Jaques Smith (deleted user)

With the new patch that is the EMI functionality. Unfortunately we cannot mak...

With the new patch that is the EMI functionality. Unfortunately we cannot make it smaller, then the question type will not work.

Sam Ottenhoff

Hi Jaques, The way Sakai patches are usually done is that one discreet issue ...

Hi Jaques, The way Sakai patches are usually done is that one discreet issue is addressed with a specific patch and one commit to the Subversion repository. This patch represents many separate issues from EMIs to general code cleanup.

How did you generate this patch? Do you have the possibility of generating distinct patches from separate commits to a source control repository? Ideally, this patch would become several JIRAs: one for EMIs, one for comments, one for generics, one for general cleanup, etc.

--Sam

Jaques Smith (deleted user)

Hi Sam. Yes, this patch did get into a lot of spaces. The comments that was u...

Hi Sam.
Yes, this patch did get into a lot of spaces. The comments that was updated is to remove the unnecessary 'named', incorrect and 'still in dev' comments that slipped in on a previous commit, I'm not sure why that happened.

The generics, that is like a virus, it spreads easily. The idea was to not change what was not needed to change but yet use good practices where possible. Slowly the generics did creep in and was not a separate action from the development of EMI's.

This branch has bean develop over some time, see Stephen's comments below. You are right that this patch is not totally clean. I do take not of all the comments and suggestions. At this stage my feeling is that the effort to make it clean will be too much.

Stephen Marquard

The mysql & oracle conversion scripts need to move from their 2-8-x to 2.9.0 ...

The mysql & oracle conversion scripts need to move from their 2-8-x to 2.9.0 locations to 2.9.0 to 10.x conversion scripts, though I'm confused to see these scripts don't yet exist in:

https://source.sakaiproject.org/svn/sam/trunk/docs/conversion/

Are there no other code changes with db schema updates since 2.9.0?

Jaques Smith (deleted user)

Updated the conversion scripts as requested.

Updated the conversion scripts as requested.

Lydia Li

Jaques, is this patch running in production at UCT?

Jaques, is this patch running in production at UCT?

Stephen Marquard

The 2.9.x version of this code has been running in production at UCT for a ye...

The 2.9.x version of this code has been running in production at UCT for a year. We also did some QA on this trunk version and did not find any obvious problems.

Stephen Marquard

There is some discussion of this on the sakai-pmc list which I am not a membe...

There is some discussion of this on the sakai-pmc list which I am not a member of and cannot post to.

Here is the history (in code) of this patch:

First pre-2.9 implementation is in

https://source.sakaiproject.org/svn/sam/branches/SAK-17243/ (SAK-17243 because SAM-1943 when T&Q JIRA issues moved to SAM)

Then there is a branch of 2.9.x with EMIs added which is what UCT has been running in production for a year:

https://source.sakaiproject.org/svn/msub/uct.ac.za/sam/branches/SAM-2.9.x/

Bugfixes that came up from testing and production experience were committed to this branch.

Then as preparation for merging EMIs to trunk (i.e. this process), OpenCollab created this 10.x branch of T&Q with EMIs:

https://source.sakaiproject.org/svn/msub/uct.ac.za/sam/branches/SAM-2.10.x/

from which the patch is created.

The design of T&Q is such that adding a new question type (with full support for import / export, printing, preview, delivery, authoring, etc.) does touch a large number of files.

It appears there are some changes in this patch which are not strictly EMI-related, and they probably arose from general T&Q improvements in the 2.9.x UCT branch. It might be possible to split them out further, but if they are valid on their own merit, then it is probably worth just committing them as well.

This is a complex question type, and we do not expect people to be able to cherry-pick a few commits to merge back to their 2.9.x branch, as T&Q has changed too much to make that a simple process. (It was complex updating the 2.9.x EMI code to the 10.x T&Q codebase).

UCT has done testing on the 10.x msub build, i.e. effectively trunk T&Q with EMIs, and has not identified any regressions so far. We would do further testing once the EMI code is committed, and also intend to branch 10.x code to run in production in our 2.9.x Sakai instance if feasible.

Anthony Whyte

The patch is not ideal. It bundles together several discrete improvements and...

The patch is not ideal. It bundles together several discrete improvements and fixes that should be submitted individually as separate diffs. The promiscuous quality of such patches increases review costs, invites delay and can undermine trust between contributors. Generally, we should reject patches of this type. However, in this case, I recommend that we take the patch and merge it to trunk for the following reasons:

1. The EMI+ patch (as I will term it) represents a new question type of value to the entire Sakai Community.
2. The EMI+ patch represents an institutional investment in Sakai by the University of Cape Town. These days, we need to encourage not discourage such investments.
3. The EMI+ patch is not a raw diff but represents production code in service at UCT for over a year. Production experience counts.
4. From what I read no one has suggested that the incremental improvements that otherwise bloat the patch are either inappropriate or bad for Samigo.
5. Pushing back on the EMI+ patch in defense of an ideal does little to move the project forward.
6. The Sakai 10 clock is ticking. Further delays only compound the slow velocity of change that has for too long marked this project.
7. UCT has conducted preliminary 10 testing and has not identified any regressions.
8. I trust Stephen Marquard. If he says the patch is good, then merge it. Treat it as an exception to our practices and keep moving.

Lydia Li

I agree with everything that Anthony said https://crucible.sakaiproject.org/s...

I agree with everything that Anthony said
Since Stephen said they had done preliminary Sakai 10.x testing and they 'would do further testing once the EMI code is committed', Karen is going to merge the patch to the trunk in the next few days.

Jaques Smith (deleted user)

Thanks Anthony and Lydia.

Thanks Anthony and Lydia.

/docs/conversion/samigo_2_9_x-10_0_mysql_conversion.sql Added  
Open in IDE #permalink
/docs/conversion/samigo_2_9_x-10_0_oracle_conversion.sql Added  
Open in IDE #permalink
/samigo-api/src/.../assessment/AnswerIfc.java Changed  
Open in IDE #permalink
/samigo-api/.../assessment/AssessmentAccessControlIfc.java Changed  
Open in IDE #permalink
/samigo-api/.../assessment/AttachmentIfc.java Changed  
Open in IDE #permalink
/samigo-api/.../assessment/ItemDataIfc.java Changed  
Open in IDE #permalink
/samigo-api/.../assessment/ItemTextAttachmentIfc.java Added  
Open in IDE #permalink
/samigo-api/.../assessment/ItemTextIfc.java Changed  
Open in IDE #permalink
/samigo-api/.../assessment/SecureDeliveryModuleIfc.java Changed  
Open in IDE #permalink
/samigo-api/src/.../ifc/shared/TypeIfc.java Changed  
Open in IDE #permalink
/samigo-api/src/.../samlite/api/Answer.java Changed  
Open in IDE #permalink
/samigo-api/src/.../samlite/api/Question.java Changed  
Open in IDE #permalink
/samigo-api/.../grading/GradingSectionAwareServiceAPI.java Changed  
Open in IDE #permalink
/samigo-api/src/.../v1p2/emiTemplate.xml Added  
Open in IDE #permalink
/samigo-api/src/.../v2p0/emiTemplate.xml Added  
Open in IDE #permalink
/samigo-api/.../xsd/ccv1p2_qtiasiv1p2p1_v1p0.xsd Added  
Open in IDE #permalink
/samigo-api/src/.../v1p2/extractEMIItem.xsl Added  
Open in IDE #permalink
/samigo-api/src/.../v1p2/extractItem.xsl Changed  
Open in IDE #permalink
/samigo-app/.../renderer/RichTextEditArea.java Changed  
Open in IDE #permalink
/samigo-app/src/.../tag/RichTextEditArea.java Changed  
Open in IDE #permalink
/samigo-app/src/.../util/SamigoJsfTool.java Changed  
Open in IDE #permalink
/samigo-app/.../bundle/AuthorMessages.properties Changed  
Open in IDE #permalink
/samigo-app/.../bundle/EvaluationMessages.properties Changed  
Open in IDE #permalink
/samigo-app/src/.../author/AnswerBean.java Changed  
Open in IDE #permalink
/samigo-app/src/.../author/AssessmentBean.java Changed  
Open in IDE #permalink
/samigo-app/.../author/AssessmentSettingsBean.java Changed  
Open in IDE #permalink
/samigo-app/src/.../author/ItemAuthorBean.java Changed  
Open in IDE #permalink
/samigo-app/src/.../bean/author/ItemBean.java Changed  
Open in IDE #permalink
/samigo-app/src/.../author/ItemConfigBean.java Changed  
Open in IDE #permalink
/samigo-app/.../author/PublishedAssessmentSettingsBean.java Changed  
Open in IDE #permalink
/samigo-app/.../delivery/CalculatedQuestionBean.java Changed  
Open in IDE #permalink
/samigo-app/src/.../delivery/DeliveryBean.java Changed  
Open in IDE #permalink
/samigo-app/src/.../bean/delivery/FibBean.java Changed  
Open in IDE #permalink
/samigo-app/src/.../bean/delivery/FinBean.java Changed  
Open in IDE #permalink
/samigo-app/.../delivery/ItemContentsBean.java Changed  
Open in IDE #permalink
/samigo-app/src/.../delivery/MatchingBean.java Changed  
Open in IDE #permalink
/samigo-app/.../delivery/MatrixSurveyBean.java Changed  
Open in IDE #permalink
/samigo-app/.../delivery/SelectionBean.java Changed  
Open in IDE #permalink
/samigo-app/.../evaluation/ExportResponsesBean.java Changed  
Open in IDE #permalink
/samigo-app/.../evaluation/HistogramBarBean.java Changed  
Open in IDE #permalink
/samigo-app/.../evaluation/HistogramQuestionScoresBean.java Changed  
Open in IDE #permalink
/samigo-app/.../evaluation/HistogramScoresBean.java Changed  
Open in IDE #permalink
/samigo-app/.../evaluation/QuestionScoresBean.java Changed  
Open in IDE #permalink
/samigo-app/.../evaluation/SubmissionStatusBean.java Changed  
Open in IDE #permalink
/samigo-app/.../evaluation/TotalScoresBean.java Changed  
Open in IDE #permalink
/samigo-app/.../print/PDFAssessmentBean.java Changed  
Open in IDE #permalink
/samigo-app/.../author/ItemAddListener.java Changed  
Open in IDE #permalink
/samigo-app/.../author/ItemModifyListener.java Changed  
Open in IDE #permalink
/samigo-app/.../author/ResetItemAttachmentListener.java Changed  
Open in IDE #permalink
/samigo-app/.../author/SaveAssessmentSettings.java Changed  
Open in IDE #permalink
/samigo-app/.../author/StartCreateItemListener.java Changed  
Open in IDE #permalink
/samigo-app/.../delivery/DeliveryActionListener.java Changed  
Open in IDE #permalink
/samigo-app/.../delivery/SaCharCountListener.java Changed  
Open in IDE #permalink
/samigo-app/.../delivery/SubmitToGradingActionListener.java Changed  
Open in IDE #permalink
/samigo-app/.../evaluation/HistogramListener.java Changed  
Open in IDE #permalink
/samigo-app/.../evaluation/QuestionScoreListener.java Changed  
Open in IDE #permalink
/samigo-app/.../evaluation/StudentScoreListener.java Changed  
Open in IDE #permalink
/samigo-app/.../evaluation/StudentScoreUpdateListener.java Changed  
Open in IDE #permalink
/samigo-app/.../evaluation/TotalScoreListener.java Changed  
Open in IDE #permalink
/samigo-app/.../questionpool/SortQuestionListListener.java Changed  
Open in IDE #permalink
/samigo-app/.../select/SelectActionListener.java Changed  
Open in IDE #permalink
/samigo-app/src/.../util/TimeUtil.java Changed  
Open in IDE #permalink
/samigo-app/src/.../delivery/LoginServlet.java Changed  
Open in IDE #permalink
/samigo-app/.../helper/AuthoringHelperTest.java Changed  
Open in IDE #permalink
/samigo-app/src/.../WEB-INF/faces-config.xml Changed  
Open in IDE #permalink
/samigo-app/src/webapp/WEB-INF/samigo.tld Changed  
Open in IDE #permalink
/samigo-app/.../jquery.alerts-1.1/jquery.alerts.css Added  
Open in IDE #permalink
/samigo-app/.../ui-lightness/jquery-ui-1.7.2.custom.css Changed  
Open in IDE #permalink
/samigo-app/src/webapp/css/tool_sam.css Changed  
Open in IDE #permalink
/samigo-app/src/webapp/js/authoring-emi.js Added  
Open in IDE #permalink
/samigo-app/src/webapp/js/authoring.js Changed  
Open in IDE #permalink
/samigo-app/src/.../js/jquery.alerts-1.1.js Added  
Open in IDE #permalink
/samigo-app/src/webapp/js/utils-emi.js Added  
Open in IDE #permalink
/samigo-app/src/.../item/emiWhatsThis.jsp Added  
Open in IDE #permalink
/samigo-app/.../item/extendedMatchingItems.jsp Added  
Open in IDE #permalink
/samigo-app/src/.../item/itemHeadings.jsp Changed  
Open in IDE #permalink
/samigo-app/.../preview_item/ExtendedMatchingItems.jsp Added  
Open in IDE #permalink
/samigo-app/.../author/confirmCopyAssessment.jsp Changed  
Open in IDE #permalink
/samigo-app/src/.../author/editAssessment.jsp Changed  
Open in IDE #permalink
/samigo-app/.../author/previewAssessment.jsp Changed  
Open in IDE #permalink
/samigo-app/.../item/deliverExtendedMatchingItems.jsp Added  
Open in IDE #permalink
/samigo-app/src/.../delivery/confirmSubmit.jsp Changed  
Open in IDE #permalink
/samigo-app/.../delivery/deliverAssessment.jsp Changed  
Open in IDE #permalink
/samigo-app/.../item/displayExtendedMatchingItems.jsp Added  
Open in IDE #permalink
/samigo-app/.../evaluation/detailedStatistics.jsp Changed  
Open in IDE #permalink
/samigo-app/.../evaluation/gradeStudentResult.jsp Changed  
Open in IDE #permalink
/samigo-app/.../evaluation/histogramScores.jsp Changed  
Open in IDE #permalink
/samigo-app/.../evaluation/questionScore.jsp Changed  
Open in IDE #permalink
/samigo-app/.../preview_item/ExtendedMatchingItems.jsp Added  
Open in IDE #permalink
/samigo-app/src/.../print/printAssessment.jsp Changed  
Open in IDE #permalink
/samigo-app/src/.../questionpool/editPool.jsp Changed  
Open in IDE #permalink
/samigo-app/.../questionpool/poolTreeTable.jsp Changed  
Open in IDE #permalink
/samigo-app/.../questionpool/questionTreeTable.jsp Changed  
Open in IDE #permalink
/samigo-app/.../questionpool/removePool.jsp Changed  
Open in IDE #permalink
/samigo-app/src/.../questionpool/sharePool.jsp Changed  
Open in IDE #permalink
/samigo-app/.../samlite/samLiteValidation.jsp Changed  
Open in IDE #permalink
/samigo-app/pom.xml Changed  
Open in IDE #permalink
/samigo-hibernate/.../assessment/Answer.java Changed