•  

Comment Results

Review Name Created Custom Fields Content
CR-8 08 Dec 2014

Read token (or key secret) from configuration file.

CR-8 08 Dec 2014

If there are going to be any errors in the data for building this hash you may want to print the name/id to make it easier to find out where the problem is.

CR-8 08 Dec 2014

Can there be some in-line documentation of the section data structure? This will be easier to change in the future if the context can be clearer.

CR-8 08 Dec 2014

Make source_course and dest_course an arguement read in from command line

CR-8 08 Dec 2014

The ESB had an attribute "SectionType" with value "LEC" for lecture and "LAB" for laboratory, etc.

CR-8 08 Dec 2014

Consider putting the configuration information in a hash or an object so that when it gets changed it only needs to be changed (and tested) in a single location.

CR-8 08 Dec 2014

Is section of array type? I saw the operation is always on section[1] item

CR-8 08 Dec 2014

I'd rather use the Net::HTTP class rather than curl – here's some documentation I found:
http://www.ruby-doc.org/stdlib-2.1.3/libdoc/net/http/rdoc/Net/HTTP.html
https://www.socialtext.net/open/very_simple_rest_in_ruby_part_1

CR-9 10 Dec 2014

can insert a break statement after line 151

CR-9 10 Dec 2014

Is it possible that the Mneme assessment is assoicated with normal internal Gradebook assignment, instead of an external Gradebook assignment?

CR-9 10 Dec 2014

Fix indentation

CR-9 11 Dec 2014

Added break statement.

CR-9 11 Dec 2014

Formatted code.

CR-9 12 Dec 2014

Discussed this with Zhen and concluded there is no way to link to internal gradebook assignment from mneme tool.

CR-10 12 Dec 2014

I changed the signature of the createItemSessionElement() method to pass in the whole Assessment object instead of passing to the method all the different pieces of information it needed. So there is a bit more refactoring here than usual, hopefully for the better. Let me know if questions or clarification needed.

CR-10 17 Dec 2014

"Never" should not be hard-coded – this will likely fail if another language is used (see assessmentSettings.properties)

CR-10 17 Dec 2014

is test.getShowModelAnswer() returning String or boolean? I am confused about this if statement

CR-10 17 Dec 2014

If test == null can't you just return itemSessionControlElement and skip the nesting?

CR-10 17 Dec 2014

Yes. I changed the code to do that.

CR-10 17 Dec 2014

test.getShowModelAnswer() returns a Boolean (object). That is why the null check first.

CR-10 17 Dec 2014

Changed to use ReviewTiming.never.name() instead of hard-coded string. This achieves the same result.

CR-11 17 Dec 2014

Looks fine to me!

CR-11 18 Dec 2014

Looks good!

CR-13 22 Dec 2014

Ship it!

CR-13 22 Dec 2014

Ship it!!

CR-18 13 Jul 2015

Looks good!

CR-20 28 Aug 2015

Looks good.

CR-21 25 Sep 2015

indentation.

CR-21 25 Sep 2015

Done

CR-23 03 Nov 2015

I'd rather not see this code duplicated - I'd suggest changing this to call getCurrentNewsLinks(sakaiId, siteId, false);

CR-23 03 Nov 2015

Need a better variable name.

CR-23 03 Nov 2015

I'd use a better variable name than "feed" - it shouldn't reflect where the method is called from, but what the method does (e.g. include the calendar event URL). This applies to all the methods with the "feed" variable. Maybe "includeInfoLinkUrl"?

CR-23 03 Nov 2015

Is this feed variable used? It should be more than a placeholder.

CR-23 03 Nov 2015

Log error/warning when the source type is null

CR-23 03 Nov 2015

Should the item.getItemCount() ==1? What does "item.getItemCount() > 1" mean?

CR-23 03 Nov 2015

Same comment as Beth's. I do not see the feed value used.

CR-23 03 Nov 2015

that pieces of code was there before nothing i have added new to getCurrentNewsLinks workflow

CR-23 03 Nov 2015

I will fix that

CR-23 03 Nov 2015

This 'feed'( i will change the name) variable is not used B'coz, that whole idea of method overloaded approach is we want to keep the UI call separate from the Entity broker feed call for Dashboad UI performance slow down . I don't see a purpose of using this variable any further. Since the new method call from here go to dao.getCurrentNewsLinks(sakaiId,siteId) and if i made a change suggested as 'dao.getCurrentNewsLinks(sakaiId,siteId,false)' then i need to make changes in DashboardDao, DashboardDaoImpl, DashboardDaoMock. If i even made that change the call from Dao class is an sql query and 'feed' variable won't be used their too. We are not getting the 'directUrl' from the database since it is not stored directly in the database.
I don't see the purpose of using the variable.

CR-23 03 Nov 2015

Done

CR-23 03 Nov 2015

Done

CR-23 03 Nov 2015

Done

CR-23 03 Nov 2015

Variable Feed is renamed to 'includeInfoLinkUrl' and is being used now

CR-25 27 Jan 2016

Changes look good!

CR-26 28 Jan 2016

LGTM

CR-27 12 Feb 2016

please put some comments for the newly added functions.

CR-27 12 Feb 2016

could be "return !ac.isEmpty();"

CR-27 12 Feb 2016

same as above. Comments needed.

CR-27 12 Feb 2016

Done. Added to the sakai/master as well

CR-27 12 Feb 2016

DOne