Skip to content

XBlock Settings service configuration for support urls and timeout, JS vars cleanup/refactor#55

Merged
bryanlandia merged 18 commits intomasterfrom
bryan/help-url-from-settings
Sep 3, 2021
Merged

XBlock Settings service configuration for support urls and timeout, JS vars cleanup/refactor#55
bryanlandia merged 18 commits intomasterfrom
bryan/help-url-from-settings

Conversation

@bryanlandia
Copy link
Copy Markdown
Contributor

@bryanlandia bryanlandia commented Sep 1, 2021

Change description

Use the XBlock Settings service to support platform-level configuration of support urls and launch timeout seconds.
A bunch of refactoring and cleanup of spaghetti JS code for status and error messages
Settings default to those in use for Appsembler Tahoe (timeout at 120 seconds, support url /help)

Features are a response to customer request (ISC) to use custom feedback page for support url. Configurable timeout seconds just seemed useful, especially for development/testing.

Also drops Python 2.7 support since all customers will be on Juniper+... preps release 3.0.0.

Type of change

  • Bug fix (fixes an issue)
  • New feature (adds functionality)

Related issues

https://appsembler.atlassian.net/browse/RED-2278
https://appsembler.atlassian.net/browse/BLACK-245

Checklists

Development

  • Lint rules pass locally
  • Application changes have been tested thoroughly
  • Automated tests covering modified code pass

Security

  • Security impact of change has been considered
  • Code follows company security practices and guidelines

Code review

  • Pull request has a descriptive title and context useful to a reviewer. Screenshots or screencasts are attached as necessary
  • "Ready for review" label attached and reviewers assigned
  • Changes have been reviewed by at least one other contributor
  • Pull request linked to task tracker where applicable

@bryanlandia bryanlandia marked this pull request as ready for review September 2, 2021 03:14
estherjsuh
estherjsuh previously approved these changes Sep 2, 2021
Copy link
Copy Markdown
Contributor

@estherjsuh estherjsuh left a comment

Choose a reason for hiding this comment

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

LGTM!

briandant
briandant previously approved these changes Sep 2, 2021
Copy link
Copy Markdown
Contributor

@briandant briandant left a comment

Choose a reason for hiding this comment

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

I'm good with this. Thanks for these great changes. Just a couple questions that I'll leave to you to resolve, optionally change, and merge at will.

I'm stealing some of your patterns here for the universal lab launcher that I'm building.

On that topic, soon we'll have to come together to make some changes to this XBlock. We're going to enhance the API ...

'js_class': 'LaunchContainerXBlock'
}
}
DEFAULT_SUPPORT_URL = '/help'
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do we have customer docs to explain how this will work? "By default, your support URL will be /help. Please configure the URL at X location in your settings." I'm wondering about the case where a site does not have a support page. Perhaps having some mechanism to not post the support URL if it's blank?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks @briandant - This makes me think that the best thing is to actually put help text on the support_email field which explains that an email overrrides a support url, and tells what the support_url is set to. I'll add a commit with that to this PR.

@property
def timeout_secs(self):
lcsettings = self.get_xblock_settings()
return lcsettings.get('timeout_seconds', DEFAULT_LAUNCHER_TIMEOUT_SECONDS)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Did you test the experience when this timeout is shorter than the timeout configured in AVL. e.g., the timeout here is 60 seconds but the lab actually launches in 75 seconds.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

No, actually. I'll double check but the most harm that would be done is that a "Lab taking too long to load?" message would appear, then the lab would still load. I don't imagine we would ever configure a very short timeout or bother configuring this at the platform level, unless for a scenario like a conference where we just wanted to bump the default 120 seconds up a little bit temporarily due to high volume.

#launcher_submit:disabled, #launcher_submit[disabled] {
color: rgba(0, 0, 0, 0.8);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for cleaning up these styles 👍 💯

@briandant
Copy link
Copy Markdown
Contributor

While I've got you, @bryanlandia, could you please tell me your development workflow for this XBlock? I presume you run edX locally instead of merely the xblock-sdk. I need to get my edX environment setup, more generally speaking. Can you point me to some docs for a dev env?

@bryanlandia
Copy link
Copy Markdown
Contributor Author

While I've got you, @bryanlandia, could you please tell me your development workflow for this XBlock? I presume you run edX locally instead of merely the xblock-sdk. I need to get my edX environment setup, more generally speaking. Can you point me to some docs for a dev env?

True, I didn't use the XBlock-SDK! I tested locally using tox to run unit tests, and developed in my ISC Juniper Devstack and then ahem did some final testing and fixing on a preproduction server up on GCP using good ol ssh and runserver with a breakpoint 😬 .

Take a look at https://github.com/appsembler/sultan/, Ahmed's tool for GCP-based devstack setup. You can find a devstack image at https://console.cloud.google.com/compute/imagesDetail/projects/appsembler-devstack-30/global/images/devstack-juniper?project=appsembler-devstack-30 Ask Anders if you need access to that.

@bryanlandia bryanlandia dismissed stale reviews from briandant and estherjsuh via 2b73bd3 September 2, 2021 21:29
@bryanlandia
Copy link
Copy Markdown
Contributor Author

Updated help text for support email:

image

At some later point we could get clever and give the actual default for support url, but not high priority right now.

@bryanlandia
Copy link
Copy Markdown
Contributor Author

I re-requested reviews since the repo policy makes me. I just pushed one commit with the updated support email text.

@bryanlandia bryanlandia merged commit fc4103b into master Sep 3, 2021
@bryanlandia bryanlandia deleted the bryan/help-url-from-settings branch September 3, 2021 16:49
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.

3 participants