Beth Kirschner

Looks good (just a few comments added) - please create PR for Sakaiproject github repo too

Looks good (just a few comments added) - please create PR for Sakaiproject github repo too

Delete? Not used

Delete? Not used

Another comment typo

Another comment typo

Cut-n-paste error?

Cut-n-paste error?

updated google readme
this should fail
[NOJIRA] note regarding move to github
[NOJIRA] note regarding move to github
  • More
  • CR-23
  • finished reviewing
Merge pull request #1252 from pushyamig/DASH-344-3

DASH-344 including the directURL in the entity broker call only

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 wi...

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"?

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

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

Need a better variable name.

Need a better variable name.

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

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

[nojira] clean up directory structure
    • -67
    • +0
    /umich/lti-utils/prepareSvnCopy.sh.tmpl
Can we have another code review with Pushyami's suggested changes?

Can we have another code review with Pushyami's suggested changes?

remove commented out code

remove commented out code

remove commented out code

remove commented out code

remove commented out code

remove commented out code

I'd rather not have commented out code - just delete

I'd rather not have commented out code - just delete

fix formatting

fix formatting

Always use StringBuffer for concatenation - a lot less expensive: directURL = new StringBuffer( ServerConfigurationService.getPortalUrl() ); directURL.append( serverUrl); etc return directURL.toStr...

Always use StringBuffer for concatenation - a lot less expensive:
directURL = new StringBuffer( ServerConfigurationService.getPortalUrl() );
directURL.append( serverUrl);
etc
return directURL.toString()

Use constant (see above)

Use constant (see above)

I see you're just repeating a pattern all over the code of hard-coding this constant again and again, but it's still bad practice https://crucible.sakaiproject.org/static/nsjxqv/2static/images/wiki...

I see you're just repeating a pattern all over the code of hard-coding this constant again and again, but it's still bad practice
Please add this as a constant to MnemeService, and be sure to differentiate it from the APPLICATION_ID – I'd call it "SAKAI_MNEME_TOOL_ID.

Date should be 2015

Date should be 2015

Merge pull request #1030 from pushyamig/SAK-29691

SAK-29691 Fixing the crashing of assignment feed

Merge pull request #1052 from axxter99/DASH-345

Dash-345

Merge pull request #1053 from axxter99/Dash-345-1

Dash-345 wicket 1.4.23