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] Hairdresser: Spliting the appointment #199

Open
wants to merge 10 commits into
base: saas-17.4
Choose a base branch
from

Conversation

jaeschwa
Copy link
Collaborator

Purpose: Hairdressers need a more flexible way to manage their appointments. Often, certain services, like a 3-hour brushing, only require their attention for a portion of the time, leaving gaps that could be utilized for other tasks.

Solution: The appointment system has been updated to allow splitting an appointment into two segments with a break in between. This allows the hairdresser to free up time for other tasks while ensuring the customer’s can now book in those break.

Implementation: Added a "break" field in the appointment type setup. Upon booking, the system now creates three appointments: a parent appointment that is sent to the customer and two child appointments representing the active time blocks needed by the hairdresser. These child appointments are linked and adapted to reflect the overall booking, allowing the 1.5-hour gap to be free for other tasks.

task-4100134

Help the hair salon to hanlde cases where a task can be break down in 2
parts. The initial appointment is break down in 2 new appointment
separeted by a break.

task-4100134
The working hours related to the contracts of the employe must be in the
same time zone as the appointment. Otherwise this leads to conflict in
the timeshifts availible. So now both are on Brussel timezone.

task-4100134
In the hair salon the reminder were applied to the child also however we
don't want to have reminder on the parent and on the child so the worker
does not have too much notifications. Reminder are kept in the parent
calendar.event.

task-4100134
Copy link
Collaborator

@vava-odoo vava-odoo left a comment

Choose a reason for hiding this comment

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

Hello @jaeschwa
Thanks for your work on this! It looks promising.
I still have quite a few comments on this. We may need to discuss it further at some point.
Cheers

hair_salon/data/appointment_type.xml Outdated Show resolved Hide resolved
hair_salon/data/appointment_type.xml Outdated Show resolved Hide resolved
hair_salon/data/appointment_type.xml Outdated Show resolved Hide resolved
hair_salon/data/appointment_type.xml Outdated Show resolved Hide resolved
hair_salon/demo/hr_employee.xml Outdated Show resolved Hide resolved
hair_salon/data/base_automation.xml Outdated Show resolved Hide resolved
hair_salon/data/ir_ui_view.xml Outdated Show resolved Hide resolved
hair_salon/data/ir_ui_view.xml Outdated Show resolved Hide resolved
@@ -0,0 +1,67 @@
<?xml version='1.0' encoding='UTF-8'?>
<odoo>
Copy link
Collaborator

Choose a reason for hiding this comment

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

you probably need to handle records (and loop), not record

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not necessary.

<field name="name">Delete Sub Appointment</field>
<field name="model_id" ref="calendar.model_calendar_event"/>
<field name="action_server_ids" eval="[(6, 0, [ref('delete_subpart_main_calendar_event_server_action')])]"/>
<field name="trigger">on_unlink</field>
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't see the logic with all these unlink. For example, I just tried to unlink the base appointment, none of the "[PART]" ones was archived...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You should delete them.

@jaeschwa jaeschwa force-pushed the saas-17.4-imp-hairdresser-scju branch from 709b7dc to 094bda5 Compare September 5, 2024 11:05
Copy link
Collaborator

@vava-odoo vava-odoo left a comment

Choose a reason for hiding this comment

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

Still a few comments. I think UX can be improved with a checkbox and maybe add constrains like end>=start

hair_salon/data/base_automation.xml Outdated Show resolved Hide resolved
hair_salon/__manifest__.py Outdated Show resolved Hide resolved
Comment on lines 53 to 54
<field name="x_appointment_part_2" string="Part 2" invisible="not x_appointment_part_2"/>
<field name="x_appointment_part_2" invisible="not x_appointment_part_2"/>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Two times? 👀 🤔
Fun fact is that runbot complains about it

<field name="model_id" ref="calendar.model_calendar_event"/>
<field name="action_server_ids" eval="[(6, 0, [ref('delete_subpart_calendar_event_server_action')])]"/>
<field name="trigger">on_create_or_write</field>
<field name="filter_domain">[("name", "not ilike", "PART 1"), ("name", "not ilike", "PART 2"), ("user_id", "!=", False)]</field>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Check should be on other fields (x_parent_id or x_appointment_part_2), not on name

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

How can we do this since it is for the creation? There is no parent nor appointment before this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That's the point. Those fields should be empty.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Then you will have a cascade effect.

<field name="model_id" ref="calendar.model_calendar_event"/>
<field name="action_server_ids" eval="[(6, 0, [ref('delete_subpart_main_calendar_event_server_action')])]"/>
<field name="trigger">on_unlink</field>
<field name="filter_domain">[("name", "ilike", "PART 1"), ("user_id", "!=", False)]</field>
Copy link
Collaborator

Choose a reason for hiding this comment

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

same

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I cannot find a condition that would work.
This is used to delete subpart, but what condition differs the PART 1 from the PART 2?

hair_salon/data/ir_ui_view.xml Outdated Show resolved Hide resolved
hair_salon/data/ir_ui_view.xml Outdated Show resolved Hide resolved
hair_salon/data/ir_ui_view.xml Outdated Show resolved Hide resolved
Comment on lines 31 to 42
event2_vals = {
'name': "[PART 1]" + record.name ,
'start': start_time,
'stop': start_time + datetime.timedelta(hours=x_break_start),
'allday': record.allday,
'user_id': record.user_id.id,
'x_parent_id': record.id,
'alarm_ids' : [],
'partner_ids': [record.partner_ids[0].id],
'appointment_type_id':record.appointment_type_id.id,

}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't it shorter to duplicate the original one and edit?
For example here the location is not provided

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

All theses fields would need to be changed except maybe the user_id and the appointment_type_id.
But we will need to duplicate so +1 line. What would be the advantage of shorter it? Because this is for format but could be written in 1 line.

<field name="trigger">on_unlink</field>
<field name="filter_domain">[("appointment_type_id", "!=", False)]</field>
</record>
<record id="delete_subpart_calendar_event" model="base.automation">
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if this whole logic wouldn't benefit from adding an on_delete strategy on the new m2o fields that you create (like cascade)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll need more explaination.

@jaeschwa jaeschwa force-pushed the saas-17.4-imp-hairdresser-scju branch from 094bda5 to 706f499 Compare September 6, 2024 07:40
Copy link
Collaborator

@vava-odoo vava-odoo left a comment

Choose a reason for hiding this comment

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

We're getting close to something

hair_salon/data/base_automation.xml Show resolved Hide resolved
<record id="delete_subpart_main_calendar_event_server_action" model="ir.actions.server">
<field name="child_ids" eval="[(6, 0, [ref('break_booking_server_action')])]"/>
<field name="code"><![CDATA[parent_record = env['calendar.event'].browse(record.x_parent_id)
parent_record.x_appointment_part_2.unlink()
Copy link
Collaborator

Choose a reason for hiding this comment

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

waht about part_1?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You can only delete first part tot delete both, otherwise I cannot find a conditions.

Comment on lines +21 to +26
<field name="code"><![CDATA[x_break_start = record.x_break_start
x_break_end = record.x_break_end
appointment_duration = record.duration

start_time = record.start
end_time = record.stop
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure it's worth to declare intermediate variables

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Maybe not useful. Do you want it to be removed?

'appointment_type_id':record.appointment_type_id.id,

}
part_1 = env['calendar.event'].create(event2_vals)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can't you create in batch?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Is it worth it for 2 records?

hair_salon/data/ir_actions_server.xml Outdated Show resolved Hide resolved
Comment on lines 53 to 56
<xpath expr="//field[@name='location']" position="before">
<field name="x_appointment_part_2" string="Part 2" invisible="not x_appointment_part_2"/>
<field name="x_parent_id" invisible="not x_parent_id"/>
</xpath>
Copy link
Collaborator

Choose a reason for hiding this comment

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

What about part_1?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We have to chose either how to avoid cascade effect?

Comment on lines +50 to +51
<field name="x_break_start" invisible="x_parent_id" widget="float_time"/>
<field name="x_break_end" invisible="x_parent_id" widget="float_time"/>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
<field name="x_break_start" invisible="x_parent_id" widget="float_time"/>
<field name="x_break_end" invisible="x_parent_id" widget="float_time"/>
<field name="x_break_start" invisible="x_parent_id or not x_break" widget="float_time"/>
<field name="x_break_end" invisible="x_parent_id or not x_break" widget="float_time"/>

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Breaking the view

hair_salon/data/ir_ui_view.xml Outdated Show resolved Hide resolved
@@ -0,0 +1,57 @@
<?xml version='1.0' encoding='UTF-8'?>
<odoo>
<record id="x_break_end_appointment_model" model="ir.model.fields">
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should maybe make sure that those float are reset to 0 if the user unchecks x_break. What do you think?
For that, I'd move the field to a compute (not readonly and stored), with the python dealing with this only case.

<field name="model_id" ref="calendar.model_calendar_event"/>
<field name="action_server_ids" eval="[(6, 0, [ref('delete_subpart_calendar_event_server_action')])]"/>
<field name="trigger">on_create_or_write</field>
<field name="filter_domain">[("name", "not ilike", "PART 1"), ("name", "not ilike", "PART 2"), ("user_id", "!=", False)]</field>
Copy link
Collaborator

Choose a reason for hiding this comment

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

That's the point. Those fields should be empty.

@jaeschwa jaeschwa force-pushed the saas-17.4-imp-hairdresser-scju branch from 706f499 to 8ac3fce Compare September 6, 2024 15:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants