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.

Sign up for GitHub

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

Jump to bottom

[14.0][IMP] hr_timesheet_activity_begin_end: missing form fields #577

Merged
merged 1 commit into from
Apr 18, 2023
Merged

[14.0][IMP] hr_timesheet_activity_begin_end: missing form fields #577

merged 1 commit into from
Apr 18, 2023

Conversation

dalonsod
Copy link
Contributor

time_start and time_stop fields were missing in timesheets form view. This PR adds them.

@dalonsod
Copy link
Contributor Author

@ernestotejeda @CRogos could you review? Thanks!

CRogos
CRogos suggested changes Mar 16, 2023
View reviewed changes
Copy link
Contributor

@CRogos CRogos left a comment

Choose a reason for hiding this comment

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

The code looks good, but there is similar change in the v16 PR.
b1785ef
I think we should use the same XML in both versions.

@dalonsod
Copy link
Contributor Author

Thanks @CRogos , I agree with you...
In v16 migration, could that improvement be placed in a separate commit, so it could be ported to v15?

Meanwhile I'll replicate the code in the same way as yours, but view name, that seeems to be wrong.

@dalonsod dalonsod mentioned this pull request Mar 16, 2023
Merged
@CRogos
Copy link
Contributor

CRogos commented Mar 16, 2023

Yes you are right. I'll change the name and commit it in a separate commit.

@CRogos CRogos mentioned this pull request Mar 16, 2023
Merged
@dalonsod dalonsod force-pushed the 14.0-imp-hr_timesheet_activity_begin_end-form branch from 262ed9a to d01f94d Compare March 16, 2023 10:51
@dalonsod
Copy link
Contributor Author

I think we should use the same XML in both versions.

@CRogos applied changes

CRogos
CRogos suggested changes Mar 16, 2023
View reviewed changes
Copy link
Contributor

@CRogos CRogos left a comment

Choose a reason for hiding this comment

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

I am so sorry. I wasn't aware that the date was located diffently in v14.
image
That doesn't look good in my opinion.

The same code in v16 leads to this result.
image

So maybe we should switch to your first xml version in v14 and v16?

@dalonsod dalonsod force-pushed the 14.0-imp-hr_timesheet_activity_begin_end-form branch from d01f94d to 724e421 Compare March 17, 2023 16:31
@dalonsod
Copy link
Contributor Author

Done!

imagen

CRogos
CRogos approved these changes Mar 20, 2023
View reviewed changes
Copy link
Contributor

@CRogos CRogos left a comment

Choose a reason for hiding this comment

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

Code and functional review LGTM

@dalonsod
Copy link
Contributor Author

@schhatbar-initos @davejames could you review? Thanks!

davejames
davejames approved these changes Apr 17, 2023
View reviewed changes
Copy link
Member

@davejames davejames left a comment

Choose a reason for hiding this comment

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

LGTM

@OCA-git-bot
Copy link
Contributor

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). 🤖

@dalonsod
Copy link
Contributor Author

@nimarosa is this ready to be merged? Thanks!

@nimarosa
Copy link

/ocabot merge patch

@OCA-git-bot
Copy link
Contributor

This PR looks fantastic, let's merge it!
Prepared branch 14.0-ocabot-merge-pr-577-by-nimarosa-bump-patch, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 52c08df into OCA:14.0 Apr 18, 2023
@OCA-git-bot
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CRogos CRogos CRogos approved these changes

@davejames davejames davejames approved these changes

@nimarosa nimarosa nimarosa approved these changes

Assignees
No one assigned
Labels
approved merged 🎉 ready to merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@dalonsod @CRogos @OCA-git-bot @nimarosa @davejames