= Review (NS-143): FR1116: Global Crawler Traps =
|| Author || Colin ||
|| Moderator || Colin ||
|| State || Closed ||
== Objectives ==
{{{
A component to allow lists of crawler traps to be appended to all scheduled jobs. There are
three sub-components
i. Backend: datamodel + DAO
ii. Business logic: (small) modifications to class Job which actually add the global crawler traps to all new Jobs
iii. GUI: a single page with corresponding backend for carrying out all the necessary operations for managing lists of global crawler traps  - create, read, update, delete and activate/deactivate.
}}}
'''General comments''':
|| '''Description''' || '''Classification''' || '''Status''' ||
|| Append the english versions of the new properties to the other language files (Translations_de.properties, Translations_fr.properties, Translations_it.properties) || Cosmetic || OK ||
|| Remove @author and @since tags in all the code-files. We don't use these tags in NetarchiveSuite || Cosmetic || OK ||


|| Time Used svc: 1md csr: 12md || Cosmetic || NOTOK ||

=== Comments on file 'trunk/src/dk/netarkivet/harvester/webinterface/TrapActionEnum.java', revision 1226 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| General || Wrong revision shown; add SVN property svn:keywords with value (URL Revision Author Date Id) || Cosmetic || OK ||
|| 39-64 || Add javadoc before each enum value || Cosmetic || OK ||
=== Comments on file 'trunk/webpages/HarvestDefinition/traplist-download-element.jspf', revision 1239 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| General || Add NetarchiveSuite header-file || Cosmetic || OK ||
|| 24 || long line; make newline before 'value=..' || Cosmetic || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/harvester/datamodel/GlobalCrawlerTrapListDAO.java', revision 1226 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| General || Wrong revision shown; add SVN property svn:keywords with value (URL Revision Author Date Id) || Cosmetic || OK ||
|| 45 || Complete the @return statement || Cosmetic || OK ||
|| 51 || Complete the @return statement || Cosmetic || OK ||
|| 58 || Complete the @return statement || Cosmetic || OK ||
|| 75-80 || Missing javadoc for these methods || Cosmetic || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/harvester/webinterface/TrapCreateOrUpdateAction.java', revision 1227 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| General || Wrong revision shown; add SVN property svn:keywords with value (URL Revision Author Date Id) || Cosmetic || OK ||
|| 108-111, 113-114, 117-125 || Shouldn't line 108-125 be put into a try .. catch so that any database errors are caught? || Cosmetic || OK ||
|| 115 || This could be written more concisely like: if (fileName != null && !fileName.isEmpty()) || Cosmetic || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/harvester/datamodel/Job.java', revision 1226 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 394 || Missing javadoc for this method || Cosmetic || OK ||
|| 401 || Remove redundant line || Cosmetic || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/harvester/datamodel/GlobalCrawlerTrapList.java', revision 1226 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| General || Wrong revision shown; add SVN property svn:keywords with value (URL Revision Author Date Id) || Cosmetic || OK ||
|| 56 || say which table it is unique in. || Cosmetic || OK ||
|| 58-60 || Missing javadoc || Cosmetic || OK ||
|| 62-64 || Missing javadoc  Missing argument validation (not null and not empty string) || Cosmetic || OK ||
|| 67 || Missing '.' at end of line. || NA || OK ||
|| 69 || say which table it is unique in. || Cosmetic || OK ||
|| 132-133 || Missing '.' at end of line. || Cosmetic || OK ||
|| 136 || Missing argument validation of argument 'is' || Cosmetic || OK ||
|| 149-151 || Missing javadoc || Cosmetic || OK ||
|| 153 || Missing javadoc || NA || OK ||
|| 157 || Missing javadoc || NA || OK ||
|| 161 || Missing javadoc Missing argument validation || Cosmetic || OK ||
|| 165 || Missing javadoc || Cosmetic || OK ||
|| 169 || Missing javadoc Missing argument validation || Cosmetic || OK ||
|| 173 || Missing javadoc || Cosmetic || OK ||
|| 177-179 || Missing javadoc || Cosmetic || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/harvester/webinterface/TrapReadAction.java', revision 1227 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| General || Wrong revision shown; add SVN property svn:keywords with value (URL Revision Author Date Id) || Cosmetic || OK ||
|| 42-43 || Remove @author and @since tags. We don't use these tags in NetarchiveSuite || Cosmetic || OK ||
|| 58 || Breaks checkstyle rule: move "+" at end of line to next line. || Cosmetic || OK ||
|| 64 || Insert whitespace around ":" || Cosmetic || OK ||
|| 65 || replace (trap+"\n") with (trap + "\n") || Cosmetic || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/harvester/datamodel/MySQLSpecifics.java', revision 1238 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| General || Remove long lines in methods createGlobalcrawlerTrapLists() and createGlobalcrawlerTrapExpressions() || Cosmetic || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/harvester/datamodel/GlobalCrawlerTrapListDBDAO.java', revision 1226 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| General || Wrong revision shown; add SVN property svn:keywords with value (URL Revision Author Date Id) || Cosmetic ||OK ||
|| 110 || Missing javadoc || Cosmetic || OK ||
|| 113 || Missing javadoc; add new line before method || Cosmetic || OK ||
|| 127 || Move "+" to next line || Cosmetic || OK ||
|| 129 || Is it really necessary to log here? The exception is rethrown just after the logentry || Cosmetic || OK ||
|| 144 || Add missing javadoc || Cosmetic || OK ||
|| 149 || add newline || Cosmetic || OK ||
|| 168 || Add missing javadoc Remove long line || Cosmetic || OK ||
|| 214-219 || Add javadoc || Cosmetic || OK ||
|| 226 || Add comment that here we delete all entries from global_crawler_trap_lists where .... || Cosmetic || OK ||
|| 229 || Add comment that here we delete all entries from global_crawler_trap_expressions where .... || Cosmetic || OK ||
|| 244 || Add javadoc || Cosmetic || OK ||
|| 249 || Add missing argument validation || Cosmetic || OK ||
|| 254 || Add comment to what is done here || Cosmetic || OK ||
|| 260 || Add comment to what is done here || Cosmetic || OK ||
|| 263 || Add comment to what is done here || Cosmetic || OK ||
|| 271 || Move "+" to next line || Cosmetic || OK ||
|| 281-286 || Add javadoc || Cosmetic || OK ||
|| 296 || Move "+" to next line || Cosmetic || OK ||
|| 312 || Move "+" to next line || Cosmetic || OK ||
=== Comments on file 'trunk/webpages/HarvestDefinition/traplist_createorupdate_element.jspf', revision 1239 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| General || Add NetarchiveSuite fileheader || Cosmetic || OK ||
=== Comments on file 'trunk/webpages/HarvestDefinition/traplist_activation_element.jspf', revision 1238 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| General || Add NetarchiveSuite fileheader || Cosmetic || OK ||
=== Comments on file 'trunk/webpages/HarvestDefinition/Definitions-edit-global-crawler-traps.jsp', revision 1239 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| General || Missing standard NetarchiveSuite header-file || Cosmetic || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/harvester/datamodel/DerbySpecifics.java', revision 1233 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| General || Remove long lines in methods createGlobalcrawlerTrapLists() and createGlobalcrawlerTrapExpressions() || Cosmetic || OK ||
|| 320 || Is the case not wrong here? || Cosmetic || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/harvester/webinterface/TrapAction.java', revision 1226 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| General || Wrong revision shown; add SVN property svn:keywords with value (URL Revision Author Date Id) || Cosmetic || OK ||
|| 70, 77 || Replace "".equals(requestType) with requestType.isEmpty() || Cosmetic || OK ||
=== Comments on file 'trunk/webpages/HarvestDefinition/traplist_delete_element.jspf', revision 1238 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| General || Add NetarchiveSuite fileheader || Cosmetic || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/harvester/webinterface/TrapActivationAction.java', revision 1227 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| General || Wrong revision shown; add SVN property svn:keywords with value (URL Revision Author Date Id) || Cosmetic || OK ||
|| 57 || Shouldn't we put some of this code inside a try..catch, so that we can intercept any database errors? || Cosmetic || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/harvester/webinterface/Constants.java', revision 1227 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 86-87 || add missing "." || Cosmetic || OK ||
|| 89-99 || Consider adding separate javadoc for each constant || Cosmetic ||Rejected||
=== Comments on file 'trunk/src/dk/netarkivet/harvester/webinterface/TrapDeleteAction.java', revision 1209 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| General || Wrong revision shown; add SVN property svn:keywords with value (URL Revision Author Date Id) || Cosmetic || OK ||
|| 44-45 || Put into a try-catch, intercept any SQL-exceptions, and forward an error the user || Cosmetic || OK ||