= Review (NS-23): FR 1497 (Button in NetarchiveSuite) =
|| Author || Søren ||
|| Moderator || Søren ||
|| State || Closed ||
== Objectives ==
{{{
Review of code that fixed FR 1497:
   trunk/src/dk/netarkivet/harvester/harvesting/DirectHeritrixController.java, r537, lines 193-201
   trunk/src/dk/netarkivet/harvester/harvesting/HeritrixController.java,  r537, lines 131-135
   trunk/src/dk/netarkivet/harvester/harvesting/HeritrixLauncher.java,  r537, lines 216-219
   trunk/src/dk/netarkivet/harvester/harvesting/JMXHeritrixController.java,  r537, lines  501-508
   trunk/src/dk/netarkivet/monitor/webinterface/JMXSummaryUtils.java,  r537, lines 234-235, 242
The primary changes:
Addition of getHarvestInformation() method to HeritrixController interface, and its implementation in DirectHeritrixController and JMXHeritrixController.
Use this method in a logmessage in HeritrixLauncher.doCrawlLoop()
In JMXSummaryUtils.generateMessage, any text found in a logmessage that starts with http are turned into a clickable link.
}}}
== Summary ==
{{{
SVC to follow-up
}}}
'''Total Time Used (Coding,Documentation,Review)''':

{{{
Time use (Coding,Documentation,Review)
KFC: 1
SVC: 0.25
JOLF: 0.25
}}}
'''General comments''':
|| '''Description''' || '''Classification''' || '''Status''' ||


=== Comments on file 'trunk/src/dk/netarkivet/harvester/harvesting/DirectHeritrixController.java', revision 537 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 193-200 || What if the harvester is not running??  It will still return "running inline", even if does not run. Also shouldn't "running inline" be a put into a constant? Followup: Add a TODO to make this method respond after how the Harvester really is doing, and not just respond "Running inline" || Cosmetic || OK ||
|| 222-227 || Fix this indentation. || Cosmetic || OK ||
|| 141-143 || No Javadoc on this function. || Cosmetic || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/harvester/harvesting/HeritrixController.java', revision 537 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 132 || "where to access" => the URL to a the GUI of running harvester || Cosmetic || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/monitor/webinterface/JMXSummaryUtils.java', revision 537 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 47-51, 60-61 || Javadoc these constants. || Cosmetic || OK ||
|| 46 || Missing dot in javadoc || Cosmetic || OK ||
|| 235 || This line is too long. Add comment to this line: All strings starting with "http:" or "https:" are replaced with proper HTML Anchor || Cosmetic || OK ||
|| 286-291 || Javadoc this. || Cosmetic || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/harvester/harvesting/JMXHeritrixController.java', revision 537 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 252-255, 288-291, 296, 301-303, 308, 318, 325-327, 346, 356, 390, 454 || These function does something else than just call the HeritrixController#cleanup() function. What? and why not describe it? || Cosmetic || OK ||
|| 71, 78-86 || Javadoc these constants and the log. And add TODO: Add feature request in the Heritrix project to make publicly available constants for these attributes in the Heritrix code. || Cosmetic || OK ||
|| 157-165 || Also Javadoc these constants. Why are these constants not publicly available in the Heritrix code. Add feature request in the Heritrix project || Cosmetic || OK ||
|| 507 || Remove empty line || Cosmetic || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/harvester/harvesting/HeritrixLauncher.java', revision 537 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 50-51 || Two empty lines next to each other. || Cosmetic || OK ||