I see Terran is looking at this, but I saw thought I would offer some feedback about the general structure of the code.
With some notable exceptions, the OpenILS::Application Perl module namespace is generally reserved for modules (and module groups) that provide an OpenSRF application implementation. I would recommend the contents of the OpenILS::Application::EResourceLinkClick package be moved into the OpenILS::WWW namespace, and not inherit from OpenILS::Application, since it's not meant to register OpenSRF methods. In fact, as it doesn't have any overlapping sub names with the parallel OpenILS::WWW::EResourceLinkClick module, it could be absorbed there quite nicely, I think.
A couple schema notes:
- The IDL says "clicked_at" is required, but it has a DEFAULT. This may be debatable, still, but I would lean towards not marking anything that has a database default.
- The course column on action.eresource_link_click_course is NOT NULL and is meant to point to a course id, but does not have an FKEY. I would recommend the reverse of that -- allow it to be NULL, add the FKEY, and if we want to be able to remove the course, set the FKEY to ON UPDATE CASCADE ON DELETE SET NULL.
I see Terran is looking at this, but I saw thought I would offer some feedback about the general structure of the code.
With some notable exceptions, the OpenILS: :Application Perl module namespace is generally reserved for modules (and module groups) that provide an OpenSRF application implementation. I would recommend the contents of the OpenILS: :Application: :EResourceLinkC lick package be moved into the OpenILS::WWW namespace, and not inherit from OpenILS: :Application, since it's not meant to register OpenSRF methods. In fact, as it doesn't have any overlapping sub names with the parallel OpenILS: :WWW::EResource LinkClick module, it could be absorbed there quite nicely, I think.
A couple schema notes:
- The IDL says "clicked_at" is required, but it has a DEFAULT. This may be debatable, still, but I would lean towards not marking anything that has a database default. eresource_ link_click_ course is NOT NULL and is meant to point to a course id, but does not have an FKEY. I would recommend the reverse of that -- allow it to be NULL, add the FKEY, and if we want to be able to remove the course, set the FKEY to ON UPDATE CASCADE ON DELETE SET NULL.
- The course column on action.
Otherwise, it all looks good from a code read.