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

Create job advert #4

Open
wants to merge 88 commits into
base: master
Choose a base branch
from
Open

Create job advert #4

wants to merge 88 commits into from

Conversation

mitrevski94
Copy link

@mitrevski94 mitrevski94 commented May 30, 2017

Added new subsection for creating job advertisements inside the Recruitment section. New migration, new form with validation, Resource Controller, Model, Repository and Interface.

Copy link
Member

@trajchevska trajchevska left a comment

Choose a reason for hiding this comment

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

Specific comments are left where needed. In general:

  1. Make sure the code format is PSR2 compliant. You can install this package in Sublime and build the files before committing.
  2. All functions should be properly documented, taking into account all input params and their type. You can use DocBlockr in Sublime.
  3. When adding other classes with "use", make sure they are needed in the current class.

$jobData = $request->all();
$jobData = $this->jobAdvertRepository->create($jobData);
$request->session()->flash('success', trans('app.pim.job_advert.store_success'));
return redirect()->route('recruitment.job_advert.edit', $jobData->id); }
Copy link
Member

Choose a reason for hiding this comment

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

Make sure the format of your code follows PSR2 standards. Check this package for Sublime that builds the code for you.

/**
* Store a newly created resource in storage.
*
* @param \Illuminate\Http\Request $request
Copy link
Member

Choose a reason for hiding this comment

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

Make sure the functions are documented properly, using the actual type of the input parameters and the response. Check other controllers for reference.

namespace App\Modules\Recruitment\Http\Controllers;

use Illuminate\Http\Request;
use App\Http\Controllers\Controller;
Copy link
Member

Choose a reason for hiding this comment

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

After finishing with the rest of the functions in this controller, check what of these "use" statements is not needed and remove them.

namespace App\Modules\Recruitment\Http\Requests;

use Illuminate\Foundation\Http\FormRequest;
use Illuminate\Validation\Rule;
Copy link
Member

Choose a reason for hiding this comment

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

You don't need Rule and Route here. Make sure you only include the classes you use.

{
$rules = [
'title' => ['required'],
'image' => ['required']
Copy link
Member

Choose a reason for hiding this comment

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

The image is not required, you can remove it. Please add the description as well as required field.


use DB;
use App\Repositories\EloquentRepository;
use App\User;
Copy link
Member

Choose a reason for hiding this comment

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

Remove the unnecessary use statements.

@@ -0,0 +1,109 @@
@extends('layouts.main')
@section('content')
Copy link
Member

Choose a reason for hiding this comment

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

In cases like this, when the file is copied from another module but not yet adapted, please remove irrelevant data and leave the basic structure only so it's not confusing.

@@ -25,6 +25,7 @@ public function register()
Settings\Repositories\SalaryComponentsRepository::class => [Settings\Repositories\Interfaces\SalaryComponentsRepositoryInterface::class],
Pim\Repositories\EmployeeRepository::class => [Pim\Repositories\Interfaces\EmployeeRepositoryInterface::class],
Pim\Repositories\CandidateRepository::class => [Pim\Repositories\Interfaces\CandidateRepositoryInterface::class],
Recruitment\Repositories\JobAdvertRepository::class => [Recruitment\Repositories\Interfaces\JobAdvertRepositoryInterface::class],
Copy link
Member

Choose a reason for hiding this comment

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

Please move this below together with the rest of the Recruitment bindings.

'update_success' => 'The job advertisement details were successfully updated.',
'delete_success' => 'The job advertisement was successfully removed.',
'additional' => 'Additional details',
'preferences' => [
Copy link
Member

Choose a reason for hiding this comment

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

The preferences part is forgotten here, right? Please check.

routes/web.php Outdated
@@ -166,6 +166,7 @@
'destroy' => 'candidates.destroy'
]]);


Copy link
Member

Choose a reason for hiding this comment

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

Please remove this extra new line.

mitrevski94 and others added 3 commits May 31, 2017 17:11
Add BrowserStack logo

Adjust BrowserStack URL - readme

change BrowserStack logo format

Update browserstack url

Update Readme

Add BrowserStack reference to readme

Update Readme
@trajchevska trajchevska closed this Jun 2, 2017
@trajchevska trajchevska reopened this Jun 2, 2017
mitrevski94 and others added 23 commits June 7, 2017 10:01
Replaced required rules on gender and birth date fields
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

4 participants