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

[IMP]hr_recruitment:Redesigned hr_job kanban view cards, created Link… #47707

Open
wants to merge 3 commits into
base: master
from

Conversation

@tde-banana-odoo
Copy link
Contributor

tde-banana-odoo commented Mar 16, 2020

…din default source for jobs, improved applicant tree view, remove left panel from kanban view , made some fixes to email template of recruitement

Description of the issue/feature this PR addresses:

Current behavior before PR:

Desired behavior after PR is merged:

--
I confirm I have signed the CLA and read the PR guidelines at www.odoo.com/submit-pr

…din default source for jobs, improved applicant tree view, remove left panel from kanban view , made some fixes to email template of recruitement
Copy link
Contributor

awa-odoo left a comment

It's looking better than the first version but we are not there quite yet.

It's ok for this time but next time try to avoid creating a new PR, as we like to keep a nice history of the comments.

Your commit message needs to be reworked to follow guidelines (reminder: https://www.odoo.com/documentation/13.0/reference/guidelines.html#git).

Example of a clean commit message by one of your fellow newcomers: 6443282
(Side note 1: Except it's missing the task id reference at the bottom).
(Side note 2: And all lines should not go further than 80 characters to keep it easily readable)

Also, important note, your runbot build is failing, making it impossible to test your task.
Can you check what's wrong and fix it?

Thanks for your work and keep trying! It's normal to get a lot of comments on your first tasks.

</t>
<div name="kanban_boxes" class="row o_recruitment_kanban_boxes">
<div class="o_recruitment_kanban_box o_kanban_primary_bottom bottom_block" style="padding-left:8px;">
<div class="col-6"></div>
<div class="col-6">
<a type="action" name="%(action_hr_job_sources)d">Share Trackers</a>
<a type="object" name="%(action_hr_job_sources_func)d">Link Trackers</a>

This comment has been minimized.

Copy link
@awa-odoo

awa-odoo Mar 17, 2020

Contributor

You should try to keep the existing action instead of redefining a function for that link.
I guess you redefined a function to be able to customize the context?

@@ -44,7 +45,7 @@
<a class="o_kanban_manage_toggle_button" href="#"><i class="fa fa-ellipsis-v" role="img" aria-label="Manage" title="Manage"/></a>
</div>
</div>
<div class="container o_kanban_card_content">
<div t-att-class="'container o_kanban_card_content o-sample_data'">

This comment has been minimized.

Copy link
@awa-odoo

awa-odoo Mar 17, 2020

Contributor

Why t-att-class?

t-att-class is to be able to evaluate variable while building the classes, here you are specifying regular classes so you don't need it.

This comment has been minimized.

Copy link
@lextjes

lextjes Mar 31, 2020

I took the t-att-class from a kanban view where they where using a graph in Kanban card and then forget check if a i needed it I'm gonna erase it

@@ -54,7 +55,7 @@
</div>
<div class="col-6">
<field name="new_application_count"/> New Applications <br/>
<field name="no_of_recruitment"/> To Recruit
<a type="object" name="action_view_job"><field name="no_of_recruitment"/> To Recruit</a>

This comment has been minimized.

Copy link
@awa-odoo

awa-odoo Mar 17, 2020

Contributor

You can remove this action for now until we figure it out with the PO, I don't think we should keep it.

<xpath expr="//group" position="after">
<searchpanel>
<field name="company_id" groups="base.group_multi_company" icon="fa-building"/>
<field name="user_id" icon="fa-users"/>
</searchpanel>
</xpath>
Comment on lines -105 to -110

This comment has been minimized.

Copy link
@awa-odoo

awa-odoo Mar 17, 2020

Contributor

Why did you remove that? I don't see anything about it in the pad specs (maybe I missed it).

<record id="hr_applicant_list_view" model="ir.ui.view">
<field name="name">Applications List</field>
<field name="model">hr.applicant</field>
Comment on lines +23 to +25

This comment has been minimized.

Copy link
@awa-odoo

This comment has been minimized.

Copy link
@awa-odoo

awa-odoo Mar 17, 2020

Contributor

Also, why are you creating a new view instead of modifying the existing one?

@@ -96,6 +120,7 @@
<field name="legend_blocked" invisible="1"/>
<field name="legend_done" invisible="1"/>
<div class="oe_title">
<span class="o_form_label"><field name="name" readonly="1" nolabel="1"/></span>

This comment has been minimized.

Copy link
@awa-odoo

awa-odoo Mar 17, 2020

Contributor

I don't think specifying the same field multiple times is a good idea.
Why do you need this bit of code ?

@@ -386,7 +411,7 @@
<field name="res_model">hr.recruitment.source</field>
<field name="view_mode">tree</field>
<field name="domain">[('job_id', '=', active_id)]</field>
<field name="context">{'default_job_id': active_id}</field>
<field name="context">{'default_job_id': active_id,'default_source_id':3}</field>

This comment has been minimized.

Copy link
@awa-odoo

awa-odoo Mar 17, 2020

Contributor

Never hardcode IDs.
IDs are auto-incremented and are never guaranteed to be given to a specific record.
It means if we had another record before the one you want in the demo data, it will take the "3" ID and your development will be broken.

You can use an "eval" and "ref" to find the matching default source you want and achieve this.
I let you search a bit on that, there are examples in the code base.

@@ -856,7 +881,7 @@ action = act
<field name="name">hr.recruitment.source.tree</field>
<field name="model">hr.recruitment.source</field>
<field name="arch" type="xml">
<tree string="Sources of Applicants" editable="top" class="o_recruitment_list">
<tree string="Sources of Applicants" editable="top" class="o_recruitment_list width_class">

This comment has been minimized.

Copy link
@awa-odoo

awa-odoo Mar 17, 2020

Contributor

As specified in the task pad, let's not do that.
We will let the JS framework optimize the column width.

new_source = self.env['hr.recruitment.source'].create({
'source_id': 6,
'job_id': res.id
})
Comment on lines +135 to +138

This comment has been minimized.

Copy link
@awa-odoo

awa-odoo Mar 17, 2020

Contributor

What is this code for?
We can add default source in views but we should not force the source for every created jobs.

@@ -134,3 +164,31 @@ def edit_dialog(self):
'type': 'ir.actions.act_window',
'target': 'inline'
}
kanban_dashboard_graph = fields.Text(compute='_kanban_dashboard_graph')

This comment has been minimized.

Copy link
@awa-odoo

awa-odoo Mar 17, 2020

Contributor

Fields should always have a name, even if they're not displayed in views.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.