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

MSPB-253: Implement 'Always On, Busy, and No Answer' Call Forwarding Feature #493

Merged
merged 95 commits into from
Dec 15, 2023

Conversation

Isaac2600Hz
Copy link
Contributor

No description provided.

@Isaac2600Hz Isaac2600Hz requested a review from pcandia April 2, 2023 10:07
i18n/en-US.json Outdated
},
"acknowledge": {
"title": "Acknowledge Call",
"helpText": "Receive a prompt to accept forwarded calls. This prevents voicemails from being associated with the 'Forward To' number."
Copy link
Contributor

Choose a reason for hiding this comment

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

I wouldn't suggest using single quotes, instead you can do

Suggested change
"helpText": "Receive a prompt to accept forwarded calls. This prevents voicemails from being associated with the 'Forward To' number."
"helpText": "Receive a prompt to accept forwarded calls. This prevents voicemails from being associated with the \"Forward To\" number."

layoutTemplate.find('.feature-popup-title').each(function () {
var strategy = $(this).data('template');

if (strategy != 'off' || strategy != 'selective') {
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead i would do this:

Suggested change
if (strategy != 'off' || strategy != 'selective') {
if (strategy !== 'off' || strategy !== 'selective') {

Copy link
Contributor

@pcandia pcandia left a comment

Choose a reason for hiding this comment

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

I also have one question, why was the "Call Forwarding" option moved to the left of "Caller-ID number"? In the mockup I see it as the second option, which is how it was before:
image

Also, when testing the changes I saw a few issues, the first ones are that saving any option causes issues:
2023-04-02_19-07
2023-04-02_19-23

2023-04-02_20-22

Also, I would recommend checking the linter by running gulp lint --app=voip
You can see a bunch of suggestions which some are part of my review, please let me know if you have any issues running this or understanding what its asking you to do, some examples are these:
image

})

$template.find('.radio-state').on('change', function () {
var self = this,
Copy link
Contributor

Choose a reason for hiding this comment

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

you can remove this line as this is unused.


if (self.checked && self.defaultValue == 'voicemail') {
_.each(options, function (div) {
if (div.children[0].innerText != 'Forward direct calls only') {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is just a suggestion, instead of doing this, maybe you can add a class in the div instead of this.

Suggested change
if (div.children[0].innerText != 'Forward direct calls only') {
if ($divClassName.innerText !== 'Forward direct calls only') {

strategy = self.name.split('.')[0],
options = $template.find('.option[strategy=' + strategy + ']');

if (self.checked && self.defaultValue == 'phoneNumber') {
Copy link
Contributor

Choose a reason for hiding this comment

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

it's always best to use "===" operator to compare the values as it also checks the data type

Suggested change
if (self.checked && self.defaultValue == 'phoneNumber') {
if (self.checked && self.defaultValue === 'phoneNumber') {

_.each(options, function (div) {
$template.find(div).removeClass('disabled').find('input')
})
if (strategy == 'selective') {
Copy link
Contributor

Choose a reason for hiding this comment

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

same here

Suggested change
if (strategy == 'selective') {
if (strategy === 'selective') {

}
}

if (self.checked && self.defaultValue == 'voicemail') {
Copy link
Contributor

Choose a reason for hiding this comment

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

and here

Suggested change
if (self.checked && self.defaultValue == 'voicemail') {
if (self.checked && self.defaultValue === 'voicemail') {


<div class="remove-rule">
<label>
Remove rule
Copy link
Contributor

Choose a reason for hiding this comment

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

and here.

Comment on lines 130 to 132
{{#monsterCheckbox "large-checkbox"}}
<input type="checkbox" name="{{strategy}}.isEnabled" {{#if enabled}} checked="checked" {{/if}} />
{{/monsterCheckbox}}
Copy link
Contributor

Choose a reason for hiding this comment

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

Just the spacing here and in other places

Suggested change
{{#monsterCheckbox "large-checkbox"}}
<input type="checkbox" name="{{strategy}}.isEnabled" {{#if enabled}} checked="checked" {{/if}} />
{{/monsterCheckbox}}
{{#monsterCheckbox "large-checkbox"}}
<input type="checkbox" name="{{strategy}}.isEnabled" {{#if enabled}} checked="checked" {{/if}} />
{{/monsterCheckbox}}

Comment on lines 5 to 13
<th>
Day
</th>
<th>
Start Time
</th>
<th>
End Time
</th>
Copy link
Contributor

Choose a reason for hiding this comment

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

please do not hardcode values, instead we want to add them to the languages files

@@ -0,0 +1,24 @@
<div data-strategy="{{strategy}}">
<div class="title">
{{@root.i18n.users.callForwarding.strategies.common.options.title}}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
{{@root.i18n.users.callForwarding.strategies.common.options.title}}
{{i18n.users.callForwarding.strategies.common.options.title}}


<div class="voicemail-wrapper">
<div>
{{#monsterRadio @root.i18n.users.callForwarding.strategies.common.voicemail.label }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
{{#monsterRadio @root.i18n.users.callForwarding.strategies.common.voicemail.label }}
{{#monsterRadio i18n.users.callForwarding.strategies.common.voicemail.label }}

@ramandeepromana
Copy link
Contributor

I also have one question, why was the "Call Forwarding" option moved to the left of "Caller-ID number"? In the mockup I see it as the second option, which is how it was before: image

I believe that part was done by @joristirado and @Isaac2600Hz just worked on top of it (if I'm not wrong 😄)

@pcandia
Copy link
Contributor

pcandia commented Apr 3, 2023

@ramandeepromana yeah, i understand. It's an easy change and also we should always try to follow the mockups. So my comments were based on the mockups https://app.abstract.com/share/1b55ca8e-e43f-429c-969b-22f99f2491bc

Requested change/clean up as well as fixes for better match with mockups
Copy link
Contributor

@pcandia pcandia left a comment

Choose a reason for hiding this comment

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

Currently when clicking on "custom" the options for it is only displayed in the first option. Also, the "Additional options" are displayed even when it should be displayed for "Phone Number"
image

And after saving it, only the first row is displayed.

@Isaac2600Hz
Copy link
Contributor Author

Isaac2600Hz commented Sep 4, 2023

Currently when clicking on "custom" the options for it is only displayed in the first option. Also, the "Additional options" are displayed even when it should be displayed for "Phone Number" image

And after saving it, only the first row is displayed.

Fixed

Isaac2600Hz and others added 4 commits September 26, 2023 12:18
…d Ensure Backward Compatibility

- Removed the selective staggery implementation as it no longer aligns with the revised scope and the approaching QA merge deadline.
- Integrated updates to Issac's recent work, focusing on two main aspects:
  1. Upgraded UI logic to resolve issues that were causing functionality breakdowns, ensuring stability and alignment with the defined scope/mockups.
  2. Adapted the new call_forward object structure while ensuring compatibility with the old/no strategies structure, guaranteeing seamless functionality across different versions.
@ramandeepromana ramandeepromana self-assigned this Dec 13, 2023
@ramandeepromana ramandeepromana changed the title MSPB-253: Add call-forwarding app. MSPB-253: Implement 'Always On, Busy, and No Answer' Call Forwarding Feature Dec 13, 2023
Copy link
Contributor

@pcandia pcandia left a comment

Choose a reason for hiding this comment

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

This looks good to me. Also, after talking to Ramen, he explained to me that the design doesn't need to match the mockups as they are for comm.land, he also said that there won't be additional options for the "voicemail" selection because in Kazoo there is no forward to voicemail and they are manipulating user's default callflow.

ramandeepromana added a commit that referenced this pull request Dec 15, 2023
…rding Feature

Introduced enhancements to the Call Forwarding feature, integrating 'Always On, Busy, and No Answer' modes and changes to Call Failover.

1. Feature Extension: Expanded Call Forwarding functionality to include 'Always On', 'Busy', and 'No Answer'.

2. Decoupled Call Failover: Segregated 'Call Failover' into an independent feature module, for structural clarity in feature configuration and call management.

3. Object Structure Overhaul and Backward Compatibility: Implemented the new `call_forward` object schema and ensured backward compatibility with legacy/no-strategy structures.

Please refer original/master branch PR #493 for additional information.
@ramandeepromana ramandeepromana merged commit c43fbef into master Dec 15, 2023
1 check passed
@ramandeepromana ramandeepromana deleted the MSPB-253 branch December 15, 2023 16:58
ramandeepromana added a commit that referenced this pull request Dec 18, 2023
…rding Feature (#523)

Introduced enhancements to the Call Forwarding feature, integrating 'Always On, Busy, and No Answer' modes and changes to Call Failover.

1. Feature Extension: Expanded Call Forwarding functionality to include 'Always On', 'Busy', and 'No Answer'.

2. Decoupled Call Failover: Segregated 'Call Failover' into an independent feature module, for structural clarity in feature configuration and call management.

3. Object Structure Overhaul and Backward Compatibility: Implemented the new `call_forward` object schema and ensured backward compatibility with legacy/no-strategy structures.

Please refer original/master branch PR #493 for additional information.
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.

4 participants