Voicemail Fast Forward and Rewind #5662

noahmehl · 2019-04-04T18:28:56Z

This is a first stab at integrating the uuid_fileman seek command in relation to call flows voicemail module to add the feature of fast forward and rewind of playback of voicemails.

I also want to note that in our testing, even if we can get kazoo to issue the correct FS command: uuid_fileman $uuid seek:+/- value, the voicemail doesn't actually seek. In my cursory testing, even issuing the command via fs_cli doesn't work either. I haven't had a chance to test via vanilla FS build/configs.

extremerotary · 2019-04-11T15:46:36Z

@jamesaimonetti This would be pretty cool to add!

jamesaimonetti

First pass, some small things to clean up. Overall good stuff!

jamesaimonetti · 2019-04-11T17:06:01Z

applications/callflow/src/module/cf_voicemail.erl

    ,{'prompt', <<"vm-message_menu">>}
    ].

+play_prompt(Message, true=_AllowFfRw) ->
+    {'play', Message, ?VM_MESSAGE_TERMINATORS};


If I customize keys in the callflow vm system_config to something other than 5 and 8, this will terminate playback instead of ff/rw.

Also please tickie all atoms: true should be 'true'.

@jamesaimonetti latest code has been updated for this, please take a look :)

jamesaimonetti · 2019-04-11T17:07:10Z

applications/callflow/src/module/cf_voicemail.erl

            forward_message(H, Box, Call),
            {_, NMessage} = kvm_message:set_folder(?VM_FOLDER_SAVED, H, AccountId),
            _ = kapps_call_command:prompt(<<"vm-saved">>, Call),
            play_messages(T, [NMessage|PrevMessages], Count, Box, Call);
+        {ok, rewind} ->


tickie (won't comment on all places that need it though).

@jamesaimonetti latest code has been updated for this, please take a look :)

jamesaimonetti · 2019-04-11T17:08:28Z

applications/callflow/src/module/cf_voicemail.erl

            forward_message(H, Box, Call),
            {_, NMessage} = kvm_message:set_folder(?VM_FOLDER_SAVED, H, AccountId),
            _ = kapps_call_command:prompt(<<"vm-saved">>, Call),
            play_messages(T, [NMessage|PrevMessages], Count, Box, Call);
+        {ok, rewind} ->
+            lager:notice("caller chose to rewind 10 sec of the message"),


Is this really 'notice'-level worthy? I would just use info for these unless you have a compelling reason for the higher level.

@jamesaimonetti latest code has been updated for this, please take a look :)

jamesaimonetti · 2019-04-11T17:09:50Z

applications/callflow/src/module/cf_voicemail.erl

+allow_ff_rw(MailboxJObj) ->
+    case ?DEFAULT_ALLOW_FF_RW of
+        'true' -> 'true';
+        'false' -> kzd_voicemail_box:pin_required(MailboxJObj)


Why does 'pin_required' matter for whether ff/rewind is allowed?

@jamesaimonetti latest code has been updated for this, please take a look :)

jamesaimonetti · 2019-04-11T17:11:34Z

core/kazoo_amqp/src/api/kapi_dialplan.erl

@@ -30,6 +30,7 @@
        ,execute_extension/1, execute_extension_v/1
        ,break/1, break_v/1
        ,play/1, play_v/1, playstop/1, playstop_v/1
+        ,playseek/1, playseek_v/1 %TODO


No longer todo right? LOL

@jamesaimonetti latest code has been updated for this, please take a look :)

jamesaimonetti · 2019-04-11T17:12:26Z

core/kazoo_amqp/src/api/kapi_dialplan.hrl

+                              ,{<<"Application-Name">>, <<"playseek">>}
+                              ,{<<"Insert-At">>, <<"now">>}
+                              ]).
+-define(PLAY_SEEK_REQ_TYPES, []).


I would like Duration to have an fun is_integer/1 type validator.

@jamesaimonetti latest code has been updated for this, please take a look :)

jamesaimonetti · 2019-04-11T17:13:17Z

core/kazoo_call/src/kapps_call_command.erl

+    ok;
+seek(Direction, Duration, Call) ->
+    NoopId = noop_id(),
+                                                %Commands = [kz_json:from_list([{<<"Application-Name">>, <<"noop">>}


Remove commented code

@jamesaimonetti latest code has been updated for this, please take a look :)

jamesaimonetti · 2019-04-11T17:13:49Z

core/kazoo_call/src/kapps_call_command.erl

+-spec seek_command(atom(), kz_term:api_pos_integer()) -> kz_json:object().
+seek_command(Direction, Duration) ->
+    kz_json:from_list([{<<"Application-Name">>, <<"playseek">>}
+                      ,{<<"Direction">>,Direction}


Space after the comma and before Direction. Same for Duration below

@jamesaimonetti latest code has been updated for this, please take a look :)

jamesaimonetti · 2019-04-11T17:15:53Z

core/kazoo_call/src/kapps_call_command.erl

@@ -2446,7 +2511,8 @@ do_collect_digits(#wcc_collect_digits{max_digits=MaxDigits
                    do_collect_digits(Collect#wcc_collect_digits{after_timeout=kz_time:decr_timeout(After, Start)});
                {'ok', Digit} ->
                    %% DTMF received, collect and start interdigit timeout
-                    Digits =:= <<>>
+                    FlushOnDigit


We've been moving away from having the right-most arg not returning boolean() when using andalso/orelse.

I would move this to a function maybe_flush_queue(FlushOnDigit, Digits, Call) with a definition like

maybe_flush_queue('true', <<>>, Call) -> flush(Call); maybe_flush_queue(_FlushOnDigit, _Digits, _Call) -> 'ok'.

@jamesaimonetti latest code has been updated for this, please take a look :)

jamesaimonetti · 2019-04-11T17:18:22Z

applications/callflow/src/module/cf_voicemail.erl

            forward_message(H, Box, Call),
            {_, NMessage} = kvm_message:set_folder(?VM_FOLDER_SAVED, H, AccountId),
            _ = kapps_call_command:prompt(<<"vm-saved">>, Call),
            play_messages(T, [NMessage|PrevMessages], Count, Box, Call);
+        {ok, rewind} ->
+            lager:notice("caller chose to rewind 10 sec of the message"),
+            _ = kapps_call_command:seek(rewind, 10000, Call),


I think the duration should be figured out for the mailbox when creating the mailbox record instead of hard-coded.

I would check the VM box, the owner_id's User doc, then the system_config doc for voicemail for this setting.

@jamesaimonetti latest code has been updated for this, please take a look :)

noahmehl · 2019-04-20T15:07:26Z

I've opened a bug in the Freeswitch Jira:

https://freeswitch.org/jira/browse/FS-11796

jamesaimonetti · 2019-04-29T19:26:23Z

applications/callflow/src/module/cf_voicemail.erl

@@ -41,7 +41,16 @@
 -define(KEY_DELETE_AFTER_NOTIFY, <<"delete_after_notify">>).
 -define(KEY_SAVE_AFTER_NOTIFY, <<"save_after_notify">>).
 -define(KEY_FORCE_REQUIRE_PIN, <<"force_require_pin">>).
+-define(KEY_ALLOW_FF_RW, <<"allow_ff_rw">>).
+-define(KEY_SEEK_DURATION, <<"seek_duration">>).


Generally speaking we try to put units of time in the names: seek_duration_ms in this case to ease reading.

@jamesaimonetti latest code has been updated for this, please take a look :)

jamesaimonetti · 2019-04-29T19:26:44Z

applications/callflow/src/module/cf_voicemail.erl

 -define(MAX_INVALID_PIN_LOOPS, 3).
+-define(DEFAULT_SEEK_DURATION, 10000).


In a similar vein, there are time related macros in kz_types.hrl to use to ease reading: 10 * ?MILLISECONDS_IN_SECOND would compile to 10000 but reads a little easier.

@jamesaimonetti latest code has been updated for this, please take a look :)

jamesaimonetti · 2019-04-29T19:31:17Z

applications/callflow/src/module/cf_voicemail.erl

@@ -1629,6 +1688,20 @@ should_require_pin(MailboxJObj) ->
        'false' -> kzd_voicemail_box:pin_required(MailboxJObj)
    end.

+-spec allow_ff_rw(kz_json:object()) -> boolean().
+allow_ff_rw(MailboxJObj) ->
+    case ?DEFAULT_ALLOW_FF_RW of


I think the logic is not quite right here. The kapps_config flag should be used to allow the voicemail box's setting to be used (and not as an allowed-by-default setting like it is this way). I would rewrite this to:

case ?DEFAULT_ALLOW_FF_RW of 'true' -> kzd_voicemail_box:allow_ff_rw(MailboxJObj); 'false' -> 'false' end

So if kapps_config returns false the ff/rw feature is not available on this system, full stop (retaining backwards compat). If the system operator turns that config flag to true then the voicemail box's setting is used (defaulting to false to preserve existing functionality).

Does that make sense?

@jamesaimonetti There was some confusion here. We actually only want to have to enable the feature at the kapps_config level. We removed any VMBox level config.

@jamesaimonetti We have updated as requested.

extremerotary · 2019-05-08T16:30:09Z

Hey @lazedo ,
Do you think you'll have some time to check out the freeswitch bug that was discovered during this programming?
https://freeswitch.org/jira/browse/FS-11796

I think this fast-forward and rewind feature is pretty sweet, and we need your help to finish it off!

noahmehl · 2019-05-09T00:58:21Z

@lazedo @extremerotary @jamesaimonetti Great news! We found someone to provide a patch for this. I have submitted it to the Jira issue and created a PR for the Freeswitch repo:

https://freeswitch.org/stash/projects/FS/repos/freeswitch/pull-requests/1725 (master)

Also, I have a v1.8 version here:
https://freeswitch.org/stash/users/noahmehl_reper.io/repos/freeswitch/browse?at=refs%2Fheads%2Fv1.8

noahmehl · 2019-05-14T17:05:07Z

FF:RW.graffle.pdf

This is what we propose for logic on whether the feature is enabled.

jamesaimonetti · 2019-05-14T23:45:58Z

@noahmehl i wouldn't even worry about account config. As I said in IRC:

is_ff_rw_enabled(VMBox) ->
    case kapps_config:get(?CONFIG_CAT, <<"is_voicemail_ff_rw_enabled">>, 'false')
        andalso kzd_vmboxes:is_ff_rw_enabled(VMBox, 'false').

First check ensures the system operator has opted-in and second check ensures the vmbox owner wants to use the feature (and presumably has tested that there are no conflicts in DTMF mapping).

As stated in IRC as well, a separate PR should be created to categorize DTMF keys and check for conflicting keys within a category when creating VM boxes via API.

noahmehl · 2019-05-15T11:40:08Z

@jamesaimonetti I have updated the logic document:

FF:RW - 1.0.2.pdf

We will work on this update.

noahmehl · 2019-05-15T11:53:21Z

Great news! @lazedo @jamesaimonetti @extremerotary

The Freeswitch PR was merged: https://freeswitch.org/stash/projects/FS/repos/freeswitch/pull-requests/1725

sergey-safarov · 2019-05-15T13:07:17Z

applications/callflow/src/module/cf_voicemail.erl

@@ -133,8 +153,10 @@
              ,prev = <<"4">> :: kz_term:ne_binary()
              ,next = <<"6">> :: kz_term:ne_binary()
              ,delete = <<"7">> :: kz_term:ne_binary()
+              ,rewind = <<"5">> :: kz_term:ne_binary()
+              ,fastforward = <<"8">> :: kz_term:ne_binary()


How about to use this menu

,keep = <<"1">> :: kz_term:ne_binary() ,replay = <<"2">> :: kz_term:ne_binary() ,forward = <<"3">> :: kz_term:ne_binary() ,prev = <<"4">> :: kz_term:ne_binary() ,callback = <<"5">> :: kz_term:ne_binary() ,next = <<"6">> :: kz_term:ne_binary() ,rewind = <<"7">> :: kz_term:ne_binary() ,delete = <<"8">> :: kz_term:ne_binary() ,fastforward = <<"9">> :: kz_term:ne_binary()

Callback feature on PR #5267

I think the biggest problem with that is that it would change the default behavior for every deployed system and user (because you changed delete).

I think the point is that we should not change any existing keys, and since key mapping is available per mailbox, any operator could apply the mapping you suggest at any time.

From my view this change is acceptable for kazoo 5.x.x branch.

@jamesaimonetti or @lazedo can you please comment?

jamesaimonetti

Rebase to address conflicts plus a couple more notes

jamesaimonetti · 2019-05-16T23:07:46Z

core/kazoo_call/src/kapps_call_command.erl

+
+-spec seek(atom(), kz_term:api_pos_integer(), kapps_call:call()) -> kapps_api_std_return().
+seek(_Direction, 0, _Call) ->
+    'ok';


This 'ok' is not a kapps_api_std_return(). Perhaps b_seek should pattern-match the 0 and return immediately rather than call wait_for_noop with 'ok' in the second parameter?

Alternatively you could return 'undefined' instead. Then the return type would be kz_term:api_ne_binary()

Siims 'ok' is kapps_api_std_return()

-type kapps_api_std_return() :: kapps_api_error() | {'ok', kz_json:object() | kz_term:ne_binary() } | 'ok'.

@jamesaimonetti I think we're a little confused on why it's not a kapps_api_std_return(). I imagine that it's because this is an OK from freeswitch?

@jamesaimonetti turns out b_seek/3 wasn't needed, and we removed it. Does this help?

This appears to still return 'ok' and b_seek still uses that return value (unless I'm missing something?).

jamesaimonetti · 2019-05-16T23:10:33Z

applications/callflow/src/module/cf_voicemail.erl

            forward_message(H, Box, Call),
            {_, NMessage} = kvm_message:set_folder(?VM_FOLDER_SAVED, H, AccountId),
            _ = kapps_call_command:prompt(<<"vm-saved">>, Call),
            play_messages(T, [NMessage|PrevMessages], Count, Box, Call);
+        {ok, 'rewind'} ->


Tickie ok atoms

lazedo · 2019-05-17T15:00:50Z

@noahmehl please squash your commits and push. thanks.
note that you also have several errors maybe due to a failed rebase.

src/ecallmgr_util.erl:119: head mismatch
src/ecallmgr_util.erl:13: function send_cmd/5 undefined
src/ecallmgr_util.erl:95: function send_cmd/5 undefined
src/ecallmgr_util.erl:97: spec for undefined function send_cmd/5
src/ecallmgr_util.erl:194: function cmd_is_empty/1 is unused
../../make/kz.mk:81: recipe for target 'ebin/ecallmgr.app' failed
make[2]: *** [ebin/ecallmgr.app] Error 1

noahmehl · 2019-05-21T17:03:41Z

@jamesaimonetti I have created a Jira issue for the populate_keys work:

https://2600hz.atlassian.net/browse/KAZOO-6102

noahmehl · 2019-06-02T19:27:16Z

@jamesaimonetti @lazedo we believe that everything has been addressed. Also, I just rebased from master. Can we get this merged?

sergey-safarov · 2019-06-03T07:33:09Z

applications/callflow/src/module/cf_voicemail.erl

            forward_message(H, Box, Call),
            {_, NMessage} = kvm_message:set_folder(?VM_FOLDER_SAVED, H, AccountId),
            _ = kapps_call_command:prompt(<<"vm-saved">>, Call),
            play_messages(T, [NMessage|PrevMessages], Count, Box, Call);
+        {'ok', 'rewind'} ->
+            lager:info("caller chose to rewind 10 sec of the message"),


Here and some strings bottom rewind/forward duration is configurable.
Please update log message.
Think value may removed from log message.

I removed the values.

maybe need use

lager:info("caller chose to rewind message"),

Sorry, my bad. all corect

sergey-safarov · 2019-06-03T08:11:09Z

applications/callflow/src/module/cf_voicemail.erl

+-spec is_ff_rw_enabled(kz_json:object()) -> boolean().
+is_ff_rw_enabled(MailboxJObj) ->
+    case ?IS_FF_RW_ENABLED of
+        'true' -> kzd_voicemail_box:is_ff_rw_enabled(MailboxJObj);


Looks as kzd_voicemail_box module is deprecated. And need use kzd_vmboxes in this PR.
@jamesaimonetti, @lazedo could you look.

They need to be merged into one module; that can be taken care of at a future time.

But new functionality should go in kzd_vmboxes, that is the module that tracks the schema.

jamesaimonetti · 2019-06-03T19:33:41Z

core/kazoo_call/src/kapps_call_command.erl

@@ -2373,6 +2410,7 @@ b_privacy(Mode, Call) ->
                            ,call :: kapps_call:call()
                            ,digits_collected = <<>> :: binary()
                            ,after_timeout = ?MILLISECONDS_IN_DAY :: pos_integer()
+                            ,flush_on_digit = true


Tickie atom

jamesaimonetti · 2019-06-03T19:33:57Z

applications/callflow/src/module/cf_voicemail.erl

@@ -165,6 +181,8 @@
                 ,transcribe_voicemail = 'false' :: boolean()
                 ,notifications :: kz_term:api_object()
                 ,after_notify_action = 'nothing' :: 'nothing' | 'delete' | 'save'
+                 ,is_ff_rw_enabled = false :: boolean()


jamesaimonetti · 2019-06-03T19:34:14Z

applications/callflow/src/module/cf_voicemail.erl

@@ -1057,6 +1102,8 @@ message_menu(Prompt, #mailbox{keys=#keys{replay=Replay
                                          ,kapps_call_command:default_collect_timeout()
                                          ,Interdigit
                                          ,NoopId
+                                          ,[<<"#">>]
+                                          ,false


jamesaimonetti · 2019-06-03T19:36:02Z

core/kazoo_documents/src/kzd_voicemail_box.erl

@@ -17,6 +17,8 @@
        ,pin/1, pin/2
        ,mailbox_number/1, mailbox_number/2
        ,pin_required/1, pin_required/2
+        ,is_ff_rw_enabled/1, is_ff_rw_enabled/2


Should not duplicate this between kzd_vmboxes and kzd_voicemail_boxes. Please focus on kzd_vmboxes as this is the module that tracks the schema.

@jamesaimonetti I believe we have resolved this and the other tickie items. Can you take a look?

jamesaimonetti · 2019-06-06T21:13:25Z

Merged! Yay it happened lol

jamesaimonetti requested changes Apr 11, 2019

View reviewed changes

jamesaimonetti requested changes Apr 29, 2019

View reviewed changes

sergey-safarov reviewed May 15, 2019

View reviewed changes

jamesaimonetti requested changes May 16, 2019

View reviewed changes

noahmehl force-pushed the voicemail_ff_rw branch 2 times, most recently from 7a8b67b to a5e50f7 Compare May 17, 2019 14:45

noahmehl force-pushed the voicemail_ff_rw branch 3 times, most recently from f812cd3 to 9c82b44 Compare May 28, 2019 17:26

noahmehl force-pushed the voicemail_ff_rw branch 2 times, most recently from d10183d to 441a161 Compare June 2, 2019 19:26

sergey-safarov reviewed Jun 3, 2019

View reviewed changes

noahmehl force-pushed the voicemail_ff_rw branch from 441a161 to 8a38634 Compare June 3, 2019 13:16

jamesaimonetti reviewed Jun 3, 2019

View reviewed changes

noahmehl force-pushed the voicemail_ff_rw branch from a504475 to 7362205 Compare June 4, 2019 17:49

Added voicemail fast forward and rewind

3150272

noahmehl force-pushed the voicemail_ff_rw branch from 7362205 to 3150272 Compare June 6, 2019 15:19

jamesaimonetti pushed a commit that referenced this pull request Jun 6, 2019

Added voicemail fast forward and rewind (#5662)

c9dc911

jamesaimonetti added a commit that referenced this pull request Jun 6, 2019

strict enforcement of seek command (#5662)

908534b

jamesaimonetti approved these changes Jun 6, 2019

View reviewed changes

jamesaimonetti closed this Jun 6, 2019

noahmehl mentioned this pull request Jun 9, 2019

Voicemail Fast Forward and Rewind - Rebased on 4.3 #5845

Closed

itlevel3 pushed a commit to sipengines/kazoo that referenced this pull request Oct 16, 2019

Added voicemail fast forward and rewind (2600hz#5662)

0affe04

itlevel3 pushed a commit to sipengines/kazoo that referenced this pull request Oct 16, 2019

strict enforcement of seek command (2600hz#5662)

4a41cb8

		-define(MAX_INVALID_PIN_LOOPS, 3).
		-define(DEFAULT_SEEK_DURATION, 10000).

Voicemail Fast Forward and Rewind #5662

Voicemail Fast Forward and Rewind #5662

Conversation

noahmehl commented Apr 4, 2019

extremerotary commented Apr 11, 2019

jamesaimonetti 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

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

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

noahmehl commented Apr 20, 2019

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

extremerotary commented May 8, 2019

noahmehl commented May 9, 2019 • edited

noahmehl commented May 14, 2019

jamesaimonetti commented May 14, 2019

noahmehl commented May 15, 2019

noahmehl commented May 15, 2019

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

jamesaimonetti left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

T0ha May 17, 2019 • edited

Choose a reason for hiding this comment

noahmehl May 17, 2019 • edited

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

lazedo commented May 17, 2019

noahmehl commented May 21, 2019

noahmehl commented Jun 2, 2019

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamesaimonetti commented Jun 6, 2019

noahmehl commented May 9, 2019 •

edited

T0ha May 17, 2019 •

edited

noahmehl May 17, 2019 •

edited