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

[FIX] sale_timesheet: project task invoice wizard daterange #140874

Conversation

dhda-odoo
Copy link
Contributor

@dhda-odoo dhda-odoo commented Nov 3, 2023

saas-16.3

Steps To reproduce:

  • install project, sales, and timesheets
  • open the project module and select any project created from SO
  • click on any task
  • click on the Sales Order smart button
  • to set the end date hover beside the start date

Issue:

  • end date should not be optional to select.

Cause:

  • the unification of datetime, daterange, and date happened in task 3121497 where daterange widget has the end date by default optional.

Solution:

  • I have added attrs and Timesheets Period label to have the same behavior as it was in saas-16.2.

task-3506482

@robodoo
Copy link
Contributor

robodoo commented Nov 3, 2023

Pull request status dashboard

@C3POdoo C3POdoo requested a review from a team November 3, 2023 09:07
@C3POdoo C3POdoo added the RD research & development, internal work label Nov 3, 2023
@ppr-odoo ppr-odoo self-requested a review November 3, 2023 12:27
@ppr-odoo ppr-odoo marked this pull request as draft November 3, 2023 12:27
@@ -15,11 +15,14 @@
name="timesheet_invoice_date_range"
attrs="{'invisible': ['|', ('invoicing_timesheet_enabled', '=', False), ('advance_payment_method', '!=', 'delivered')]}"
>
<label for="date_start_invoice_timesheet" string="Timesheets Period" attrs="{'invisible': [ '|', ('invoicing_timesheet_enabled', '=', False), ('advance_payment_method', '!=', 'delivered')]}"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to update the pot file otherwise translation is break.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, you are correct

@dhda-odoo dhda-odoo force-pushed the saas-16.3-project-task-invoice-wizard-daterange-dhda branch 2 times, most recently from 7212f09 to a2b5e91 Compare November 7, 2023 11:46
Copy link
Contributor

@pkyriakou pkyriakou left a comment

Choose a reason for hiding this comment

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

Thank you @dhda-odoo for your work 💯
I left one comment 😁

<field
name="date_start_invoice_timesheet"
widget="daterange"
attrs="{'required': ['|', ('date_start_invoice_timesheet', '!=', False), ('date_end_invoice_timesheet', '!=', False)]}"
Copy link
Contributor

Choose a reason for hiding this comment

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

is this really needed ? can't you just do this ?

Suggested change
attrs="{'required': ['|', ('date_start_invoice_timesheet', '!=', False), ('date_end_invoice_timesheet', '!=', False)]}"
required="1"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pkyriakou Yes, we do need it to ensure the same behavior as in version 16.2.

Copy link
Contributor

Choose a reason for hiding this comment

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

@pkyriakou we cannot because the daterange should be only required when there is at least one date set. If the both fields are empty then the daterange should not be required. 🙂

I agree it is a bit strange to write the required condition like this but it does not seem we have another way to do that. 😅

@ppr-odoo ppr-odoo force-pushed the saas-16.3-project-task-invoice-wizard-daterange-dhda branch from a2b5e91 to 058ad6e Compare November 17, 2023 14:38
Copy link
Contributor

@pkyriakou pkyriakou left a comment

Choose a reason for hiding this comment

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

Thank you @dhda-odoo 💯
LGTM 🟢 🥳

@dhda-odoo dhda-odoo force-pushed the saas-16.3-project-task-invoice-wizard-daterange-dhda branch 2 times, most recently from ec34bd2 to 607d64f Compare November 24, 2023 06:00
@xavierbol xavierbol force-pushed the saas-16.3-project-task-invoice-wizard-daterange-dhda branch from 607d64f to c557199 Compare December 5, 2023 09:55
Copy link
Contributor

@xavierbol xavierbol left a comment

Choose a reason for hiding this comment

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

Thanks for your work 🙂

<field
name="date_start_invoice_timesheet"
widget="daterange"
attrs="{'required': ['|', ('date_start_invoice_timesheet', '!=', False), ('date_end_invoice_timesheet', '!=', False)]}"
Copy link
Contributor

Choose a reason for hiding this comment

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

@pkyriakou we cannot because the daterange should be only required when there is at least one date set. If the both fields are empty then the daterange should not be required. 🙂

I agree it is a bit strange to write the required condition like this but it does not seem we have another way to do that. 😅

@@ -15,13 +15,23 @@
name="timesheet_invoice_date_range"
attrs="{'invisible': ['|', ('invoicing_timesheet_enabled', '=', False), ('advance_payment_method', '!=', 'delivered')]}"
>
<label for="date_start_invoice_timesheet" string="Timesheets Period"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

Could just be a string in the field

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@xavierbol no that is not working.
2023-12-05_16-19

Copy link
Contributor

Choose a reason for hiding this comment

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

it works, it is just because you added nolabel="1"... 😕

I will fix it

@xavierbol xavierbol marked this pull request as ready for review December 5, 2023 09:55
@xavierbol
Copy link
Contributor

I applied my comment and rebased your branch. 🙂

robodoo r+

@C3POdoo C3POdoo requested a review from a team December 5, 2023 09:58
@xavierbol
Copy link
Contributor

robodoo r-

saas-16.3

Steps To reproduce:
 - install project, sales, and timesheets
 -  open the project module and select any project created from SO
 - click on any task
 - click on the Sales Order smart button
 - to set the end date hover beside the start date

Issue:
 - end date should not be optional to select.

Cause:
 - the unification of datetime, daterange, and date happened in task 3121497
   where daterange widget has the end date by default optional.

Solution:
 - I have added attrs and Timesheets Period label to have the same behavior as it was in saas-16.2.

task-3506482
@xavierbol xavierbol force-pushed the saas-16.3-project-task-invoice-wizard-daterange-dhda branch from c557199 to 2223fbd Compare December 5, 2023 15:47
@xavierbol
Copy link
Contributor

robodoo r+

robodoo pushed a commit that referenced this pull request Dec 5, 2023
saas-16.3

Steps To reproduce:
 - install project, sales, and timesheets
 -  open the project module and select any project created from SO
 - click on any task
 - click on the Sales Order smart button
 - to set the end date hover beside the start date

Issue:
 - end date should not be optional to select.

Cause:
 - the unification of datetime, daterange, and date happened in task 3121497
   where daterange widget has the end date by default optional.

Solution:
 - I have added attrs and Timesheets Period label to have the same behavior as it was in saas-16.2.

task-3506482

closes #140874

Signed-off-by: Xavier Bol (xbo) <xbo@odoo.com>
@robodoo robodoo closed this Dec 5, 2023
@fw-bot fw-bot deleted the saas-16.3-project-task-invoice-wizard-daterange-dhda branch December 19, 2023 18:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RD research & development, internal work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants