[MIG][11.0]hr_employee_age:Migration to 11.0. #458

Trivedi-Vacha-SerpentCS · 2018-07-03T12:00:17Z

[Mig][11.0]hr_employee_age:Migration to 11.0.

hr_employee_age/models/hr_employee.py

hieulucky111

Some minors adjustment, LGTM over all.

hr_employee_age/README.rst

hr_employee_age/views/hr_employee.xml

nikul-serpentcs

Add Unit test case too

JayVora-SerpentCS · 2018-09-29T20:12:47Z

Good to add unit tests

nikul-serpentcs

LGTM

pedrobaeza · 2018-10-01T08:29:30Z

Can you please squash a bit commit history and fix that merge commit (rebasing you will get this fixed)?

Trivedi-Vacha-SerpentCS · 2018-10-01T11:14:02Z

@pedrobaeza Squshed, Ready to merge

espo-tony

Please, tests should also verify the correctness of the result.

espo-tony · 2018-10-01T13:03:27Z

hr_employee_age/tests/test_hr_employee.py

+        })
+
+    def test_compute_age(self):
+        self.emp_root._compute_age()


This test only checks that the code runs correctly. It doesn't do any check about it providing the correct result.
Please, add some simple assertion on self.emp_root.age field.

@espo-tony here an only call _compute_age method, and one more thing we pass birthday field.

Ex. Today age is 28 but next year 29 that time assertion is False so we directly use this method
here we use manually assertEqual(self.emp_root.age, 28) but it's wrong to logically.

CC @JayVora-SerpentCS

@nikul-serpentcs Of course the assert needs to be dinamic: 28 shouldn't be hardcoded but computed within the test.

@espo-tony compute nothing to return, we have only one variable like self.emp_root.age then how to assert. and how to check another one.if you have any idea about that.

Something like:

age = relativedelta( fields.Date.from_string(fields.Date.today()), fields.Date.from_string(self.emp_root.birthday)).years assertEqual(self.emp_root.age, age)

Otherwise you can do something like this:

self.emp_root.birthday = fields.Date.from_string( fields.Date.today() - relativedelta(years=28)) self.emp_root._compute_age() assertEqual(self.emp_root.age, 28)

Or I guess you can find another couple of ways to achieve the same goal. The point is that the test as it's written is not proving the code is functionally correct because it doesn't do any assertion on its result.

@espo-tony Ok, In UT one more time Calculate age and after check assertion.

On the assert equal we are comparing two variables that are computed using the same instruction so the will obviously be equal. I know it is a simple function and probably works fine, but I think it would be better to use the second option proposed by @espo-tony or even using mock to control what fields.Date.today() returns and compare the age against what we expect.

@jarroyomorales nope, not ready ^

JayVora-SerpentCS · 2018-10-01T13:10:00Z

I realised lately yes, we need asserts!

Trivedi-Vacha-SerpentCS · 2018-10-03T05:27:00Z

@espo-tony Fixed Code, please add review
@pedrobaeza @JayVora-SerpentCS Squash done, Ready to Merge

rafaelbn · 2019-07-24T21:15:20Z

Hi @Trivedi-Vacha-SerpentCS ,

I'm rebuilding runbot for testing. In any case i saw travis is 🔴 so it's passing again the see what happens

Thanks!!!!

rafaelbn

Functional test 👍

Please review travis 🔴 ! before merging

JayVora-SerpentCS · 2019-07-29T08:22:56Z

Will do, thanks.

Trivedi-Vacha-SerpentCS · 2019-07-29T12:01:59Z

@rafaelbn Travis failed due to the issue in module hr_attendance_report_theoretical_time, this module is not in the branch of this PR.

etobella · 2019-07-29T12:29:23Z

Then rebase your branch and push force in order to test everything please

etobella · 2019-07-29T12:46:32Z

@Trivedi-Vacha-SerpentCS you should rebase not merge

jarroyomorales · 2019-08-27T14:02:14Z

Is it ready to merge??

alexey-pelykh · 2019-08-27T14:06:24Z

@jarroyomorales not all valid change-requests have beed addressed

jarroyomorales · 2019-09-26T11:47:51Z

@Trivedi-Vacha-SerpentCS can you attend #458 (comment) so we can discuss about merging it?

Trivedi-Vacha-SerpentCS · 2019-09-26T11:51:32Z

Didn't understand what exactly you are asking to change.

jarroyomorales · 2019-09-26T13:35:22Z

@Trivedi-Vacha-SerpentCS I thought the second option proposed by @espo-tony in #458 (comment) made more sense, but if you want to keep it like this forget about my comment, at the end is a simple calculation.

OCA-git-bot · 2019-09-26T21:42:21Z

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

alexey-pelykh · 2019-09-27T06:21:22Z

/ocabot merge

OCA-git-bot · 2019-09-27T06:21:32Z

On my way to merge this fine PR!
Prepared branch 11.0-ocabot-merge-pr-458-by-alexey-pelykh-bump-no, awaiting test results.

Signed-off-by alexey-pelykh

OCA-git-bot · 2019-09-27T08:59:19Z

It looks like something changed on 11.0 in the meantime.
Let me try again (no action is required from you).
Prepared branch 11.0-ocabot-merge-pr-458-by-alexey-pelykh-bump-no, awaiting test results.

Signed-off-by alexey-pelykh

OCA-git-bot · 2019-09-27T11:36:01Z

Congratulations, your PR was merged at 0e9f6c3. Thanks a lot for contributing to OCA. ❤️

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

JayVora-SerpentCS suggested changes Jul 3, 2018

View reviewed changes

hr_employee_age/models/hr_employee.py Outdated Show resolved Hide resolved

Trivedi-Vacha-SerpentCS force-pushed the 11.0-hr_employee_age branch from 4c1496a to 4e2ccee Compare July 4, 2018 05:36

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

Add delivery methods BSSFW-49 (OCA#458)

88e9a9a

hieulucky111 reviewed Jul 27, 2018

View reviewed changes

hr_employee_age/models/hr_employee.py Outdated Show resolved Hide resolved

hieulucky111 approved these changes Jul 27, 2018

View reviewed changes

nikul-serpentcs reviewed Sep 5, 2018

View reviewed changes

hr_employee_age/README.rst Outdated Show resolved Hide resolved

nikul-serpentcs reviewed Sep 5, 2018

View reviewed changes

hr_employee_age/views/hr_employee.xml Outdated Show resolved Hide resolved

nikul-serpentcs reviewed Sep 5, 2018

View reviewed changes

nikul-serpentcs approved these changes Oct 1, 2018

View reviewed changes

JayVora-SerpentCS approved these changes Oct 1, 2018

View reviewed changes

Trivedi-Vacha-SerpentCS force-pushed the 11.0-hr_employee_age branch from 972da5d to 0628197 Compare October 1, 2018 09:52

pedrobaeza added this to the 11.0 milestone Oct 1, 2018

espo-tony suggested changes Oct 1, 2018

View reviewed changes

Trivedi-Vacha-SerpentCS force-pushed the 11.0-hr_employee_age branch 2 times, most recently from a67ef81 to c6f2de8 Compare October 3, 2018 05:08

nikul-serpentcs mentioned this pull request Oct 26, 2018

[12.0][ADD] hr_employee_relatives: alternative to hr_family #489

Merged

This was referenced Dec 28, 2018

[11.0] [MIG] hr_employee_age: Migrated the module in v11. #515

Closed

Migration to version 11.0 #374

Closed

rafaelbn approved these changes Jul 24, 2019

View reviewed changes

saltonmassally and others added 8 commits July 29, 2019 18:50

[ADD] hr_employee_age

927519e

implementation update

0b4d138

[MIG] Make modules uninstallable

827bb4b

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

ba82b32

[MIG] Rename manifest files

6c85850

[MIG] 10.0 Porting hr_employee_age (OCA#293)

00d5b0e

[UPD] Update hr_employee_age.pot

e6890bd

[Mig][IMP]hr_employee_age:Migration to 11.0.

34e1adb

Trivedi-Vacha-SerpentCS force-pushed the 11.0-hr_employee_age branch from c6f2de8 to 34e1adb Compare July 29, 2019 13:21

JayVora-SerpentCS approved these changes Aug 8, 2019

View reviewed changes

espo-tony approved these changes Sep 26, 2019

View reviewed changes

OCA-git-bot added approved ready to merge labels Sep 26, 2019

alexey-pelykh approved these changes Sep 27, 2019

View reviewed changes

OCA-git-bot added a commit that referenced this pull request Sep 27, 2019

Merge PR #458 into 11.0

ed3ffad

Signed-off-by alexey-pelykh

OCA-git-bot added a commit that referenced this pull request Sep 27, 2019

Merge PR #458 into 11.0

f1982b6

Signed-off-by alexey-pelykh

OCA-git-bot merged commit 34e1adb into OCA:11.0 Sep 27, 2019

OCA-git-bot added the merged 🎉 label Sep 27, 2019

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

Mraimou pushed a commit to camptocamp/hr that referenced this pull request Feb 11, 2021

merge OCA#458 testing bso_delivery migration script

92afe04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MIG][11.0]hr_employee_age:Migration to 11.0. #458

[MIG][11.0]hr_employee_age:Migration to 11.0. #458

Trivedi-Vacha-SerpentCS commented Jul 3, 2018

hieulucky111 left a comment

nikul-serpentcs left a comment

JayVora-SerpentCS commented Sep 29, 2018

nikul-serpentcs left a comment

pedrobaeza commented Oct 1, 2018

Trivedi-Vacha-SerpentCS commented Oct 1, 2018

espo-tony left a comment

espo-tony Oct 1, 2018

nikul-serpentcs Oct 2, 2018

espo-tony Oct 2, 2018

nikul-serpentcs Oct 2, 2018

espo-tony Oct 2, 2018

nikul-serpentcs Oct 2, 2018

jarroyomorales Jul 23, 2019

alexey-pelykh Aug 27, 2019

JayVora-SerpentCS commented Oct 1, 2018 •

edited

Loading

Trivedi-Vacha-SerpentCS commented Oct 3, 2018

rafaelbn commented Jul 24, 2019

rafaelbn left a comment

JayVora-SerpentCS commented Jul 29, 2019

Trivedi-Vacha-SerpentCS commented Jul 29, 2019

etobella commented Jul 29, 2019

etobella commented Jul 29, 2019

jarroyomorales commented Aug 27, 2019

alexey-pelykh commented Aug 27, 2019

jarroyomorales commented Sep 26, 2019

Trivedi-Vacha-SerpentCS commented Sep 26, 2019

jarroyomorales commented Sep 26, 2019

OCA-git-bot commented Sep 26, 2019

alexey-pelykh commented Sep 27, 2019

OCA-git-bot commented Sep 27, 2019

OCA-git-bot commented Sep 27, 2019

OCA-git-bot commented Sep 27, 2019

[MIG][11.0]hr_employee_age:Migration to 11.0. #458

[MIG][11.0]hr_employee_age:Migration to 11.0. #458

Conversation

Trivedi-Vacha-SerpentCS commented Jul 3, 2018

hieulucky111 left a comment

Choose a reason for hiding this comment

nikul-serpentcs left a comment

Choose a reason for hiding this comment

JayVora-SerpentCS commented Sep 29, 2018

nikul-serpentcs left a comment

Choose a reason for hiding this comment

pedrobaeza commented Oct 1, 2018

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

Choose a reason for hiding this comment

JayVora-SerpentCS commented Oct 1, 2018 • edited Loading

Trivedi-Vacha-SerpentCS commented Oct 3, 2018

rafaelbn commented Jul 24, 2019

rafaelbn left a comment

Choose a reason for hiding this comment

JayVora-SerpentCS commented Jul 29, 2019

Trivedi-Vacha-SerpentCS commented Jul 29, 2019

etobella commented Jul 29, 2019

etobella commented Jul 29, 2019

jarroyomorales commented Aug 27, 2019

alexey-pelykh commented Aug 27, 2019

jarroyomorales commented Sep 26, 2019

Trivedi-Vacha-SerpentCS commented Sep 26, 2019

jarroyomorales commented Sep 26, 2019

OCA-git-bot commented Sep 26, 2019

alexey-pelykh commented Sep 27, 2019

OCA-git-bot commented Sep 27, 2019

OCA-git-bot commented Sep 27, 2019

OCA-git-bot commented Sep 27, 2019

JayVora-SerpentCS commented Oct 1, 2018 •

edited

Loading