[
https://issues.apache.org/jira/browse/OFBIZ-2205?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12688229#action_12688229
]
Ashish Vijaywargiya commented on OFBIZ-2205:
--------------------------------------------
Hello Abhinav,
The updated patch looks lot better from the initial one.
Yesterday I have spent some time in reviewing the content of your patch & Here
are few more comments
(I am sure you will not feel bad to see my comments again :-) ):
1) JobRequisition lookup should be improved. It is hard to remember the exact
jobRequisitionId so we should include other fields from JobRequisition entity.
Instead of "Begins With" we should prefer "Contains" for searching. Change this
everywhere in the patch.
2) EditApprovalStatus form should be improved to use the StatusValidChange. For
ex: Internal Job Posting status can't be changed from "Rejected" to "Approved".
Please ask question if it is confusing. You can search existing patterns to
understand the concept.
3) I think we should get rid of else part from all the service implementation
except Create Job Requisition.
The reason is that we are not explicitly passing the applicationId from the
html form.
+ <if-empty field="parameters.applicationId">
+ <make-next-seq-id value-field="newEntity"
seq-field-name="applicationId"/>
+ <else>
+ <set from-field="parameters.applicationId"
field="newEntity.applicationId"/>
+ </else>
+ </if-empty>
This case will not be applied on *Type entity based service implementation like
InterviewType & JobPostingType.
4) For changing the behaviour(optional="true" to optional="false" & vice versa)
of any field in the service definition we should use the <override> tag.
Change all the attribute tag into <override> tag and do the same for other
service definition.
+ <service name="updateJobRequisition" engine="simple"
default-entity-name="JobRequisition"
+ location="org/ofbiz/humanres/HumanResServices.xml"
invoke="updateJobRequisition" auth="true">
+ <description>Update Job Requisition</description>
+ <permission-service service-name="humanResManagerPermission"
main-action="UPDATE"/>
+ <auto-attributes mode="IN" include="pk" optional="false"/>
+ <auto-attributes mode="IN" include="nonpk" optional="true"/>
+ <attribute name="noOfResources" mode="IN" type="java.lang.Long"
optional="false"/>
+ <attribute name="durationMonths" mode="IN" type="java.lang.Long"
optional="false"/>
+ <attribute name="location" mode="IN" type="String" optional="false"/>
+ </service>
+
5) Do you know the purpose of default-map attribute in form tag ?
+ <form name="AddJobPostingType" type="single" target="createJobPostingType"
default-map-name="jobPostingType"
Please explain & if its not clear then let me know I will help you.
6) The following line should be changed to use the new pattern (see revision
756025 for more details) :
+ <field name="deleteLink" title="${uiLabelMap.CommonDelete}"
widget-style="buttontext">
+ <hyperlink
target="deleteInterviewType?interviewTypeId=${interviewTypeId}"
description="${uiLabelMap.CommonDelete}" also-hidden="false"/>
+ </field>
Please try to find the reason of this change and if you are unable to find the
reason then please let us know.
7) Do you know the purpose of default-field-type="hidden" in the following line
?
+ <auto-fields-service service-name="updateJobPostingType"
default-field-type="hidden"/>
Please explain & if its not clear then let me know I will help you.
8) You need not to fetch the "userLogin" object from session attribute in the
groovy files.
It is implicitly available.
+userLogin = session.getAttribute("userLogin");
9) We should change the groovy file name from "LoginHR.groovy" to something
more verbose.
Like ApproverPermissionCheck.groovy or ValidateApproverPermission.groovy or
IdentifyApproverPermission.groovy.
10) There is no need to add prefix or suffix as "genericValue". Just write the
name of entity following camelcase pattern.
For example : genericValueUserLoginSecurityGroup can be changed to
userLoginSecurityGroup.
This is again best practice and code can be reviewed in existing applications.
For fetching list you need to add only "s" as suffix after the name of entity
following camelcase pattern.
For example : UserLoginSecurityGroupList can be changed into
userLoginSecurityGroups
Although userLoginSecurityGroupList is also good to keep (Change "U" to "u").
General Note : Initially I was thinking to do all these changes but I thought
it would be difficult for you to review the changes.
I hope my comments will help you to overcome from such issues in future.
--
Ashish Vijaywargiya
> Implemented recruitment in HR module
> ------------------------------------
>
> Key: OFBIZ-2205
> URL: https://issues.apache.org/jira/browse/OFBIZ-2205
> Project: OFBiz
> Issue Type: New Feature
> Components: humanres
> Affects Versions: SVN trunk
> Environment: Windows XP Professional (5.1, Build 2600) Service Pack
> 2, Intel(R) Core(TM)2 CPU 4300 @ 1.80GHz (2 CPUs), 1014MB RAM, jdk1.5.0,
> apache-ant-1.7.0
> Reporter: Abhinav Vaid
> Assignee: Ashish Vijaywargiya
> Priority: Minor
> Fix For: SVN trunk
>
> Attachments: HR_Recruitment.patch, HR_Recruitment.patch,
> HR_Recruitment.patch, HR_Recruitment.patch
>
>
> In this patch we have included recruitment in the HR module.
> Recruitment performs tasks such as admin can create new job requisitions.
> He can update or delete new job requisitions.
> Now once the job requisition has been added, it is visible to all the
> employees.
> Then if interested employee wants to apply for the job requisition he sends
> it for approval to his superior.
> Superior from his login can check who all have applied for job requisition.
> He can update the status and same is reflected at employee's end.
> Here admin can also create new interview types and he can store the
> information of the diffrent interviews of employees.
> We have created Security groups:
> HUMANRES_APPROVER
> HUMANRES_EMPLOYEE
> We have created Security permissions:
> HUMANRES_APPROVE
> We have created Login Id's :
> demoadmin belongs to FULLADMIN security group
> demoapprover belongs to HUMANRES_APPROVER security group
> demoemployee belongs to HUMANRES_EMPLOYEE security group
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
