[MIG][11.0]hr_contract_multi_jobs: Migration to 11.0 #453

Trivedi-Vacha-SerpentCS · 2018-06-25T10:14:41Z

[MIG][11.0]hr_contract_multi_jobs: Migrated module in 11.0

[BSSFL-495] Commercial Invoice

nikul-serpentcs

Improve Code

nikul-serpentcs · 2018-10-01T07:48:00Z

hr_contract_multi_jobs/models/hr_contract.py

+            if contract.contract_job_ids:
+                main_jobs = contract.contract_job_ids.filtered('is_main_job')
+                if len(main_jobs) != 1:
+                    raise Warning(


@Trivedi-Vacha-SerpentCS Remove Warning use userError

nikul-serpentcs · 2018-10-01T07:50:34Z

hr_contract_multi_jobs/tests/test_hr_contract_multi_jobs.py

+        self.contract_id.write({'contract_job_ids':
+                                [(0, 0, {'job_id': self.job_id.id,
+                                         'is_main_job': True})]})
+        self.assertTrue(self.contract_id.job_id.id == self.job_id.id)


@Trivedi-Vacha-SerpentCS IMO assertTrue replace assertEqual.

Ex. self.assertEqual(self.contract_id.job_id.id, self.job_id.id)

nikul-serpentcs · 2018-10-01T07:50:50Z

hr_contract_multi_jobs/tests/test_hr_contract_multi_jobs.py

+                                         'is_main_job': True}),
+                                 (0, 0, {'job_id': self.job_2_id.id,
+                                         'is_main_job': False})]})
+        self.assertTrue(self.contract_id.job_id.id == self.job_id.id)


same as here

Fix field selection and french translation

nikul-serpentcs

Code Review LGTM

Trivedi-Vacha-SerpentCS · 2018-10-01T11:15:24Z

@pedrobaeza Commit squshed, Ready to merge

espo-tony

Just some lesser and cosmetic notes

espo-tony · 2018-10-01T11:18:58Z

hr_contract_multi_jobs/models/hr_contract.py

+        """
+        for contract in self:
+            main_job = contract.contract_job_ids.filtered('is_main_job'
+                                                          ) or False


You can remove or False because the filtered method will return an empty recordset if no record matches the condition. An empty recordset will be evaluated as False in the following conditional statement.

@espo-tony Is not need to remove, because after @Trivedi-Vacha-SerpentCS check if condition

espo-tony · 2018-10-01T11:19:24Z

hr_contract_multi_jobs/models/hr_contract.py

+class HrContract(models.Model):
+    _inherit = 'hr.contract'
+
+    contract_job_ids = fields.One2many('hr.contract.job',


Please, break the line just after ( in order to get a nicer and more readable indentation

espo-tony · 2018-10-01T11:23:36Z

hr_contract_multi_jobs/models/hr_contract_job.py

+    _description = 'Relational object between contract and job'
+
+    name = fields.Char(string='Job', related='job_id.name', index=True)
+    job_id = fields.Many2one('hr.job',


Like earlier, please, break the line just after (

espo-tony · 2018-10-01T11:23:42Z

hr_contract_multi_jobs/models/hr_contract_job.py

+                             string='Job',
+                             required=True,
+                             ondelete='cascade')
+    contract_id = fields.Many2one('hr.contract',


Like earlier, please, break the line just after (

espo-tony · 2018-10-01T11:23:52Z

hr_contract_multi_jobs/models/hr_job.py

+class HrJob(models.Model):
+    _inherit = 'hr.job'
+
+    contract_job_ids = fields.One2many('hr.contract.job',


Like earlier, please, break the line just after (

espo-tony · 2018-10-01T11:29:16Z

hr_contract_multi_jobs/views/hr_contract_view.xml

+            <field name="arch" type="xml">
+
+                <field name="job_id" position="replace">
+                    <field name="job_id" readonly="1"/>


Please, if you only wish to add a readonly attribute to this element, use XPath attributes functionality instead than remove. It should be something like:

<field name="job_id" position="attributes"> <attribute name='readonly'>1</attribute> </field>

pedrobaeza · 2018-10-01T11:52:11Z

And we need to remove the plural on the module name.

espo-tony · 2018-10-01T12:00:19Z

hr_contract_multi_jobs/models/hr_contract.py

+        """
+        for contract in self:
+            main_job = contract.contract_job_ids.filtered('is_main_job'
+                                                          )


Please, put ) on the same line than the rest of the statement it belongs

Trivedi-Vacha-SerpentCS · 2018-10-01T12:06:07Z

@pedrobaeza Module Renamed Done

espo-tony · 2018-10-01T12:20:25Z

hr_contract_multi_job/models/hr_contract.py

+        for contract in self:
+            main_job = contract.contract_job_ids.filtered('is_main_job')
+            if main_job:
+                main_job = main_job[0].job_id.id


According to the api.constraint just below, I don't think this if statement is required. That's because:

We are sure there is at least 1 main_job, which means if main_job is always True;

We are also sure there is exactly 1 main_job, therefore main_job is a recordset containing exactly 1 record, which means its fields can be directly accessed through dot-notation (no need to use [0] to access 1st and only element)

If you don't explicitely invoke a write method, you don't necessarily need to pass the ID (an integer), you can directly pass the record.

I'd rather write it as:

main_job = contract.contract_job_ids.filtered('is_main_job').job_id contract.job_id = main_job

Or even write the same thing in a single line (if you manage to stay within the max line length)

@espo-tony Here if not get main_job then ..?
if Your Conditions are False, so @Trivedi-Vacha-SerpentCS Used If a condition.

If Dev. Removed main_job in UT that time use our scenario.

@nikul-serpentcs I argue that the condition is never false, because in the code there is a constraint ensuring there is always 1 and only 1 main_job . Am I wrong?

espo-tony

Code Review LGTM

nikul-serpentcs · 2018-10-02T07:11:41Z

@pedrobaeza if it's Done then Move Forward

pedrobaeza added this to the 11.0 milestone Jun 25, 2018

pedrobaeza mentioned this pull request Jun 25, 2018

Migration to version 11.0 #374

Closed

71 tasks

Trivedi-Vacha-SerpentCS changed the title ~~11.0 hr contract multi jobs~~ [Mig][11.0]hr_contract_multi_jobs: Migrated module in 11.0 Jun 28, 2018

Trivedi-Vacha-SerpentCS changed the title ~~[Mig][11.0]hr_contract_multi_jobs: Migrated module in 11.0~~ [Mig][11.0]hr_contract_multi_jobs: Migration to 11.0 Jun 28, 2018

Trivedi-Vacha-SerpentCS force-pushed the 11.0-hr_contract_multi_jobs branch 2 times, most recently from 639267b to 006ce52 Compare July 3, 2018 05:43

Trivedi-Vacha-SerpentCS changed the title ~~[Mig][11.0]hr_contract_multi_jobs: Migration to 11.0~~ [MIG][11.0]hr_contract_multi_jobs: Migration to 11.0 Jul 3, 2018

sambarros pushed a commit to sambarros/hr that referenced this pull request Jul 26, 2018

Merge pull request OCA#453 from TelmoSenseFly/BSSFL-495

8286543

[BSSFL-495] Commercial Invoice

nikul-serpentcs reviewed Oct 1, 2018

View reviewed changes

feketemihai and others added 20 commits October 1, 2018 15:46

Port code to new API.

1b92632

Move icon.png to description folder.

1dcfd48

Update code to new API, update constraints code, add readme file.

e9c899c

Update translations.

20849e1

Partial remove tests.

8ff7c1b

Partial remove tests.

e85dcfc

Add tests for contract_multi_jobs.

15cb27e

Update readme.

97d3414

Fix travis errors.

186849d

Update readme.

4fb1b8a

Update code according to comments.

6dc06ab

Revert contrains in _check_one_main_job.

aeba927

Update tests to new API.

e6860a2

Fix type in authors.

3a73aea

[FIX] contract_hourly_rate

cbb6572

Fix field selection and french translation

[UPD] prefix versions with 8.0

122b748

[MIG] Make modules uninstallable

3441a49

[FIX] remove en.po that was erroneously created by transbot

6a46bf3

[MIG] Rename manifest files

e19ad34

remove obsolete .pot files [ci skip]

deb0b1a

Trivedi-Vacha-SerpentCS force-pushed the 11.0-hr_contract_multi_jobs branch from 006ce52 to 27e0a2a Compare October 1, 2018 10:18

Trivedi-Vacha-SerpentCS force-pushed the 11.0-hr_contract_multi_jobs branch from 27e0a2a to b6e7256 Compare October 1, 2018 10:38

nikul-serpentcs approved these changes Oct 1, 2018

View reviewed changes

espo-tony suggested changes Oct 1, 2018

View reviewed changes

espo-tony reviewed Oct 1, 2018

View reviewed changes

Trivedi-Vacha-SerpentCS force-pushed the 11.0-hr_contract_multi_jobs branch from 61be847 to 07b4c0d Compare October 1, 2018 12:04

espo-tony reviewed Oct 1, 2018

View reviewed changes

[MIG][11.0]hr_contract_multi_jobs:Migrated from 10.0 to 11.0.

d85f604

Trivedi-Vacha-SerpentCS force-pushed the 11.0-hr_contract_multi_jobs branch from 86af602 to d85f604 Compare October 1, 2018 12:42

espo-tony approved these changes Oct 1, 2018

View reviewed changes

pedrobaeza merged commit 536c6dc into OCA:11.0 Oct 2, 2018

Trivedi-Vacha-SerpentCS deleted the 11.0-hr_contract_multi_jobs branch September 4, 2020 10:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MIG][11.0]hr_contract_multi_jobs: Migration to 11.0 #453

[MIG][11.0]hr_contract_multi_jobs: Migration to 11.0 #453

Trivedi-Vacha-SerpentCS commented Jun 25, 2018 •

edited

Loading

nikul-serpentcs left a comment

nikul-serpentcs Oct 1, 2018

nikul-serpentcs Oct 1, 2018

nikul-serpentcs Oct 1, 2018

nikul-serpentcs left a comment

Trivedi-Vacha-SerpentCS commented Oct 1, 2018

espo-tony left a comment

espo-tony Oct 1, 2018

nikul-serpentcs Oct 1, 2018

espo-tony Oct 1, 2018

espo-tony Oct 1, 2018

espo-tony Oct 1, 2018

espo-tony Oct 1, 2018

espo-tony Oct 1, 2018

pedrobaeza commented Oct 1, 2018

espo-tony Oct 1, 2018

Trivedi-Vacha-SerpentCS commented Oct 1, 2018

espo-tony Oct 1, 2018

nikul-serpentcs Oct 1, 2018 •

edited

Loading

espo-tony Oct 1, 2018

espo-tony left a comment

nikul-serpentcs commented Oct 2, 2018

[MIG][11.0]hr_contract_multi_jobs: Migration to 11.0 #453

[MIG][11.0]hr_contract_multi_jobs: Migration to 11.0 #453

Conversation

Trivedi-Vacha-SerpentCS commented Jun 25, 2018 • edited Loading

nikul-serpentcs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikul-serpentcs left a comment

Choose a reason for hiding this comment

Trivedi-Vacha-SerpentCS commented Oct 1, 2018

espo-tony left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedrobaeza commented Oct 1, 2018

Choose a reason for hiding this comment

Trivedi-Vacha-SerpentCS commented Oct 1, 2018

Choose a reason for hiding this comment

nikul-serpentcs Oct 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

espo-tony left a comment

Choose a reason for hiding this comment

nikul-serpentcs commented Oct 2, 2018

Trivedi-Vacha-SerpentCS commented Jun 25, 2018 •

edited

Loading

nikul-serpentcs Oct 1, 2018 •

edited

Loading