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

Incorporate Project Space Dialer through AWS CCP package #28089

Merged
merged 42 commits into from
Jul 21, 2020

Conversation

ctsims
Copy link
Member

@ctsims ctsims commented Jul 11, 2020

image

SUMMARY

Provides an AWS Connect CCP dialer inside of domains when the flag is enabled and configured.

The dialer can be triggered from within a form. For domains where the dialer is enabled, Markdown text in formplayer which produces a tel:XXXXX hyperlink is intercepted before rendering and replaced with a reference which will call the domain specific dialer if it is available. This hyperlink will spawn a new tab if no other dialer has been spawned, or will reuse the existing tab if a dialer was spawned previously from web apps.

image

image

The dialer can be configured per project space with an AWS Connect instance, and header text. The Dialer page icon reuses the Custom Project Space Icon if it is available.

image

NOTE - The code for the dialer (static HTML / CSS and JavaScript) is boilerplate from AWS. This is somewhat custom widget for now and I don't think it's worth rebuilding them for now v. building a cleaner experience.

FEATURE FLAG

Yes, this creates a new feature flag for the dialer

RISK ASSESSMENT / QA PLAN

We should likely run QA on formplayer in general, but the project team can take responsibility for testing the dialer functionality since it is a private, flagged behavior.

@ctsims ctsims added the product/feature-flag Change will only affect users who have a specific feature flag enabled label Jul 11, 2020
@ctsims ctsims requested a review from orangejenny as a code owner July 11, 2020 03:17
from corehq.apps.widget.models import DialerSettings

def domain_uses_dialer(domain):
try:
Copy link
Member Author

Choose a reason for hiding this comment

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

@orangejenny I'd like to make it possible for "enterprise spaces to check a box that says "Use Enterprise Dialer settings" which this method would then use to look up the chain at the "control" space to get its settings and mirror them back down. Is that consistent with how enterprise managed domains work?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes and no. You can look up the controlling domain's name with DomainPermissionsMirror.source_domain(domain) and then pull its settings, yes. But enterprise domains don't have any additional config. So your options, from low to high effort, are:

  1. Don't add a config at all
  2. Add a boolean field domain_uses_dialer to DomainPermissionsMirror and make it visible in django admin
  3. Add the boolean field and also add a checkbox to the DomainPermissionsMirrorView, which is currently a read-only UI.

I think any of these is fine. DomainPermissionsMirror is new enough and invisible enough that no one is using it at this point except the projects it was built for.

Copy link
Contributor

@orangejenny orangejenny left a comment

Choose a reason for hiding this comment

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

Our standard for new js files is that they're compatible with RequireJS. The two minified files and detectRTC.js look like they support AMD modules already, and then ccputil.js contains the entry point (initDialer), so you could make that your RequireJS main module. Overall, it'd look something like this:

hqDefine("widgets/js/ccputil", [
   "hqwebapp/js/initial_page_data",
    "widgets/js/detectRTC",
   "widgets/js/amazon-connect-min",
    "widgets/js/connect-streams-min",
], function (
   initialPageData,
   DetectRTC
   /* not sure if you need params for the other two files, depends on if the code references them modules directly */
) {
   /* current code, ideally excluding the dead functions like copyToClipboard */

    $(function () {
        initDialer(initialPageData.get('callout_number'), initialPageData.get('aws_instance_id'))
    });
});

And then instead of the inline js in web_app_dialer.html, you'd use requirejs_main and initial page data:

{# this goes near the top of the page, outside of any other block #}
{% requirejs_main "widgets/js/web_app_dialer" %}

{# these go inside widget_content or some other block #}
{% initial_page_data 'callout_number' callout_number %}
{% initial_page_data 'aws_instance_id' dialer_settings.aws_instance_id %}

corehq/apps/cloudcare/static/cloudcare/js/util.js Outdated Show resolved Hide resolved
{% block title %}Dialer{% endblock %}

{% block widget_content %}
<div id="divBanner" class="banner">
Copy link
Contributor

Choose a reason for hiding this comment

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

Naming convention is for ids and classes to-be-like-this.


from memoized import memoized


#TODO: Add toggle decorator
Copy link
Contributor

Choose a reason for hiding this comment

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

Are there any other permissions that should be checked here, like permission to use web apps?

Copy link
Member Author

Choose a reason for hiding this comment

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

The dialer doesn't actually load any meaningful context from web apps (just styles) so I think the current toggle set is fine. Will want to go more fine grained in the future if we actually allow the dialer to extend permissions to any AWS Connect stuff, (right now the user still needs their session credentials logged in anyway) or if we start allowing them to do things like backreference case ID's

corehq/apps/widget/views.py Outdated Show resolved Hide resolved
corehq/apps/widget/forms.py Outdated Show resolved Hide resolved
corehq/apps/widget/forms.py Outdated Show resolved Hide resolved
@@ -0,0 +1,209 @@
/**
Based on AWS boilerplate
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this file be committed to production? Looks like it's only referenced in checkNetwork which is never called. This seems like it would be useful to have as documentation, but not especially useful checked in.

@dimagi dimagi deleted a comment from stickler-ci Jul 14, 2020
@dimagi dimagi deleted a comment from stickler-ci Jul 14, 2020
@dimagi dimagi deleted a comment from stickler-ci Jul 14, 2020
@dimagi dimagi deleted a comment from stickler-ci Jul 14, 2020
@dimagi dimagi deleted a comment from stickler-ci Jul 14, 2020
@dimagi dimagi deleted a comment from stickler-ci Jul 15, 2020
@ctsims
Copy link
Member Author

ctsims commented Jul 15, 2020

@orangejenny Did a pass based on initial feedback:

  • JS Imports are now managed Require.js modules
  • I moved everything out of the new widget module and into the existing integration module (previously just housing the SimPrints configuration models), which I think most cleanly captures the external dependencies
  • Cleaned up some of the template code I'd copied from other spaces
  • I fixed all of the linter inputs for code that I actually wrote vs am copying-in from AWS. I'm still not particularly inclined to go scrub all of the boilerplate code much more, since I think it's highly diminishing returns to do that v. re-implement views, and I don't think either are worth the time on the first pass, since the code is fairly isolated.

I've put everything on staging and confirmed that it all works as expected.

@@ -90,7 +90,7 @@
url(r'^remote_link/', include('corehq.apps.linked_domain.urls')),
url(r'^translations/', include('corehq.apps.translations.urls')),
url(r'^submit_feedback/$', submit_feedback, name='submit_feedback'),
url(r'^widget/', include('corehq.apps.widget.urls')),
url(r'^integration/', include('corehq.apps.integration.urls')),
Copy link
Contributor

Choose a reason for hiding this comment

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

The integration urls are already included in domain.urls, which is correct for your new code since DialerSettingsView is domain-specific (I didn't notice on first review). I think you can just remove this and everything will continue to work, just with a different URL.

url(r'^integration/', include('corehq.apps.integration.urls')),

@orangejenny
Copy link
Contributor

@ctsims Thank you for updating. I had one comment about the urls, but once that's fixed, this'll be good to go.

I fixed all of the linter inputs for code that I actually wrote vs am copying-in from AWS. I'm still not particularly inclined to go scrub all of the boilerplate code much more, since I think it's highly diminishing returns to do that v. re-implement views, and I don't think either are worth the time on the first pass, since the code is fairly isolated.

This code definitely violates the principle of "A codebase should look like it was written by one person," and I don't love that, but I agree this is isolated and that there's little value to rewriting it. RequireJS was the major thing, since without those changes someone would need to migrate these files later. The remaining differences are stylistic (naming conventions, spacing) or are because the code doesn't use our common dependencies (jQuery, moment).

One remaining question: are translations relevant for this code? That would be a matter of reading through the js files and wrapping any user-facing text with a gettext.

@dimagi dimagi deleted a comment from stickler-ci Jul 15, 2020
@dimagi dimagi deleted a comment from stickler-ci Jul 20, 2020
@ctsims
Copy link
Member Author

ctsims commented Jul 20, 2020

@orangejenny Translations submitted so I think this is in its final form

@ctsims
Copy link
Member Author

ctsims commented Jul 20, 2020

@orangejenny pending feedback from biyeun on that question anyhow.

@dimagi dimagi deleted a comment from stickler-ci Jul 20, 2020
Copy link
Contributor

@orangejenny orangejenny left a comment

Choose a reason for hiding this comment

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

Looks good. I think #28168 will resolve the yarn question.

@ctsims ctsims merged commit dcc2aab into master Jul 21, 2020
@ctsims ctsims deleted the ctsims/call_center_widgets branch July 21, 2020 20:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
product/feature-flag Change will only affect users who have a specific feature flag enabled
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants