-
Notifications
You must be signed in to change notification settings - Fork 24.4k
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] sms, sms_*: sms composer and templates #32625
Conversation
5d55dce
to
4b4f38b
Compare
bfc9839
to
834d5d3
Compare
ccf82e4
to
bea169e
Compare
218254b
to
ad2b8b9
Compare
@pimodoo I changed calendar_sms and hr_presence to work with SMS templates |
8ab5076
to
874ef22
Compare
874ef22
to
394a7ae
Compare
33dbf95
to
12bbe4f
Compare
2aeac0e
to
cb057b6
Compare
record.write({ | ||
'state': 'error', | ||
'error_code': 'insufficient_credit' | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to reduce a bit the code, you could make something like this :
for record in self:
error = 'missing_number' if not record.number else False
if not error:
number = record._get_sanitized_number()
if not number:
error = 'wrong_number_format'
else:
try:
self.env['sms.api']._send_sms([number], record.content)
except InsufficientCreditError:
error = 'insufficient_credit'
record.write({
'state': 'error' if error else 'sent',
'error_code': error
})
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And I would prefer to name 'record' into 'sms', so we directly know what we are talking about :)
Same for all the other methods
The aim of this commit is to: 1) Add templates for SMS: - Users can now create template for SMS Text messages similar to mail templates. - Templates can be linked with models, especially to create contextual action to quickly send SMS. 2) Improve the SMS composer: - Integrate the template system. - Allow to use it for mass SMS. 3) Handle SMS failures: - SMS is log in the chatter with a fa-comment icon next to the date. - The icon can be grey if all the SMS have correctly been sent. - The icon can be red if at least one SMS have not correctly been sent. - As mail, SMS failures are created if a SMS fails. The failure appears in the inbox. - When clicking on the red icon, a new modal is displayed, which is used to re-send SMS, discard SMS failures or buy more SMS credits. Task ID: 1922163
39e47b5
to
cf91e6e
Compare
@dbeguin I made the change you suggested and rebased the branch :) Thanks for having a look ! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some remarks and questions
record.write({ | ||
'state': 'error', | ||
'error_code': 'insufficient_credit' | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And I would prefer to name 'record' into 'sms', so we directly know what we are talking about :)
Same for all the other methods
return recipients | ||
|
||
@api.multi | ||
def _get_sanitized_number(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could be an opportunity to use that in voip enterprise stuff to avoid duplicated code (in a separated prepare commit with sms sanitization stuffs)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also, as for 'email_normalized' we could think of a 'number_sanitized' field, so we can directly use it in _send method
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok for use it in enterprise, it was intended :)
But the field sanitized, I don't know... We should do one for mobile, one for phone, one for work mobile on employees, one for ... ;-)
to the mail.message | ||
:param body: Note to log in the chatter. | ||
:param sms_ids: IDs of the sms.sms records | ||
:return: ID of the mail.message created |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not the id that is returned but the message itself (even if main message_post docstring is incorrect). So I suggest to rename 'message_id' into 'message' (and maybe adapt the original docstring [in separated commit?])
also, is it not possible to integrate this into message_post directly ? so that the caller calls only message_post and the message post link itself the sms_ids to the message ? (giving sms_ids in kwargs)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, what do you prefer ? override message_post ?
'account_token': account.account_token, | ||
'messages': messages | ||
} | ||
endpoint = self.env['ir.config_parameter'].sudo().get_param('sms.endpoint', DEFAULT_ENDPOINT) + '/iap/sms/1/send' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This '/iap/sms/1/send' is a route in odoo ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's in IAP
@@ -12,9 +12,37 @@ class SmsApi(models.AbstractModel): | |||
_name = 'sms.api' | |||
_description = 'SMS API' | |||
|
|||
@api.model | |||
def _send_multi_sms(self, messages): | |||
""" Send SMS using IAP in batch mode |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be the role of this method to prepare the parameters. I would give sms.sms instead of messages list of dict as sms.sms model contains everything you need.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was for the possibility to only send sms, without creating sms.sms
if self.env.context.get('default_composition_mode') != 'mass_sms' \ | ||
and not self.env.context.get('default_recipient_ids'): | ||
recipients = self.env['sms.sms']._get_sms_recipients(active_model, records.id) | ||
missing_numbers = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing_numbers = [recipient['partner_id'].display_name for recipient in recipients if not recipient['number']]
The aim of this commit is to:
Task ID: 1922163
--
I confirm I have signed the CLA and read the PR guidelines at www.odoo.com/submit-pr