Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Offer letter functionality #691

Merged
merged 144 commits into from Oct 7, 2018
Merged

Conversation

pokhiii
Copy link
Member

@pokhiii pokhiii commented Oct 6, 2018

No description provided.

Abhishek and others added 30 commits August 18, 2018 12:53
* new migration offer letter

* uploading offer letter and storing path in database

* self review and self test functionality

* styleci fix
* billings can now be created from project stage

* invoice creation from project screen

* final changes

* cleanup

* removed ajax. will come back later
@@ -22,6 +22,21 @@ Vue.component('project-stage-component', require('./components/ProjectStageCompo
Vue.component('project-stage-billing-component', require('./components/ProjectStageBillingComponent.vue'));
Vue.component('applicant-round-action-component', require('./components/HR/ApplicantRoundActionComponent.vue'));

$(document).ready(() => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why this change?

Copy link
Member Author

@pokhiii pokhiii Oct 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mohitgusain this code should be put before document.getElementById('page_hr_applicant_edit')) code block since the alert close button stopped working.

@@ -14,7 +14,6 @@
<form action="{{route('invoices.update', $invoice)}}" method="POST" enctype="multipart/form-data" class="form-invoice">
@csrf
@method('PATCH')

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove extra line.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you mean add extra line. right?

@@ -183,7 +182,11 @@
if ($loop->last) {
if ($applicationRound->application->status == config('constants.hr.status.sent-for-approval.label')) {
$showFooter = true;
} elseif (in_array($applicationRound->round_status, [null, config('constants.hr.status.rejected.label')])) {
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Follow psr-4 formatting

*
* @return void
*/
public function __construct(Application $application, $subject, $body)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

type-hint all function params.

public function isRejected()
{
return $this->status == config('constants.hr.status.rejected.label');
}

public function saveOfferLetter(string $filePath)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can avoid using this since the functionality within is also similar. We can avoid wrapping straightforward things.

@@ -73,24 +77,67 @@ public function _update($attr)
case 'send-for-approval':
$fillable['round_status'] = 'confirmed';
$application->sendForApproval($attr['send_for_approval_person']);

ApplicationMeta::create([
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not good to call one model from another model. Ideally a model's job should be just to interact with the specific resource it's supposed to take care of.

All this application logic like creating meta, sending emails, notifications etc should reside in a controller.

Copy link
Member Author

@pokhiii pokhiii Oct 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have opened a new issue to track this #692

// The below env call needs to be changed to config after the default
// credentials bug in the Google API services is resolved.
$email = $attr['onboard_email'] . '@' . env('GOOGLE_CLIENT_HD');
$applicant->onboard($email, $attr['onboard_password'], [
'designation' => $attr['designation'],
]);

User::create([
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is supposed to happen after a certain event, i.e. when an applicant is onboarded. Ideally we should use Event/Listeners in Laravel. See if model observers can help here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -183,10 +182,15 @@
if ($loop->last) {
if ($applicationRound->application->status == config('constants.hr.status.sent-for-approval.label')) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can merge first two if statements using in_array

@php
$event = $item['event'];
@endphp
<b><u>{{ date(config('constants.display_date_format'), strtotime($item['date'])) }}</u></b><br>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems like you need this in all the three cases. Can this go out of the switch statement?

@@ -0,0 +1,31 @@
<div class="card mt-4">
<form action="{{ route('setting.hr.update') }}" method="POST">

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove extra lines

@@ -0,0 +1,31 @@
<div class="card mt-4">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rename this to approve

@@ -10,6 +10,7 @@
@include('settings.hr.applicant-auto-responder')
@include('settings.hr.applicant-interview-reminder')
@include('settings.hr.no-show')
@include('settings.hr.approved')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hr.approve

@@ -62,9 +62,15 @@

Route::prefix('applications')->namespace('Applications')->group(function () {
Route::resource('/evaluation', 'EvaluationController')->only(['show', 'update']);

Route::get('job/{application}/offer-letter', 'JobApplicationController@viewOfferLetter')->name('applications.job.offer-letter');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

did you test for all these resources? job, internship and volunteer? If not, please do the first round of self-testing

@rathorevaibhav rathorevaibhav merged commit db2a658 into develop Oct 7, 2018
@rathorevaibhav rathorevaibhav deleted the offer-letter-functionality branch October 7, 2018 05:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants