Uploaded image for project: 'CiviHR'
  1. CiviHR
  2. HR-302

Absence Request - Code Review

    Details

    • Type: Task
    • Status: Done/Fixed
    • Priority: Minor
    • Resolution: Won't Do
    • Affects Version/s: HR-1.2
    • Fix Version/s: HR-1.2
    • Component/s: None
    • Labels:
      None
    • Sprint:
      Sprint 13a

      Description

      NOTES ON CRM/HRAbsence/Form/Request.tpl

      • L26 - this comment is wrong as it was copy/pasted from another template. In future I suggest not copy/pasting files as it leads to lots of problems.
      • L27 - unused variable
      • L75 - we are moving toward using $ as the local variable instead of cj as documented in https://wiki.civicrm.org/confluence/display/CRMDOC/Javascript+Reference#JavascriptReference-Enclosingyourcode
      • L84 - No boilerplate comments please. But more real comments in your code would be nice.
      • L125 - please do not create global functions outside of your closure
      • L190 - why 2 script tags?
      • L191 - please move global variables inside the closure
      • L197 - fix closure per above
      • L206, 265, 293, etc - This is weird. Please use simple object notation instead
      • Generally, the use of ids as selectors can lead to bugs as duplicate ids in the dom cause wacky collisions. Our form renderer really needs to stop putting such generic ids onto fields. But until that happens you can mitigate the problem like so:
        https://github.com/civicrm/civicrm-core/blob/1beb268a63e70974a25de6f08812575f7f11d778/templates/CRM/Event/Form/Participant.tpl#L340
        Place this at the beginning of your closure and always pass $form as the second param to jQuery whenever selecting any element. I also suggest selecting form elements by name instead of id but that's just my opinion.

      NOTES ON CRM/HRAbsence/Form/Request.php

      • L395-405 Function signature does not match params. What IDE are you using? It should tell you this automatically.
      • There are a couple other functions missing any docblock.
      • L582 wrong # of params passed to setStatus
      • L575, 583 etc.- In general we try to avoid issuing redirects directly, instead use CRM_Core_Session::singleton()->pushUserContext and let the form controller handle it.

      NOTES ON css
      I notice that you are using !important in a few places. In general I think this should a last resort since it is a pain to override. Using a more specific selector ought to be sufficient.

        Attachments

          Activity

            People

            • Assignee:
              colemanw Coleman Watts
              Reporter:
              colemanw Coleman Watts
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: