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

isEnabled: false doesnt disable the plugin #95

Closed
tomallpress opened this issue Aug 9, 2024 · 18 comments · Fixed by #98
Closed

isEnabled: false doesnt disable the plugin #95

tomallpress opened this issue Aug 9, 2024 · 18 comments · Fixed by #98

Comments

@tomallpress
Copy link

The readme states that isEnabled controls whether the plugin is enabled or not, but it seems to actually control whether the icon appears in the nav bar. If isEnabled is set to false, the cog doesn't appear, but you can still use the shortcuts to show the cog, and then use it, suggesting it's not actually disabled.

Suggest we could either:

  • re-word the readme so it says isEnabled just shows/hides the icon, and clarifying it doesn't actually disable the plugin.

or

  • maybe there should be two seperate properties: isEnabled to enable/disable the plugin (for consistency with other plugins), and then a seperate control to show/hide the icon?

What do people think?

@swashbuck
Copy link
Contributor

@tomallpress Thanks for raising this. I think _isEnabled should completely disable all things Dev Tools. Setting it to false should give you peace of mind that the plugin is safely disabled and won't potentially interfere with anything else. I wasn't even aware that you could toggle the cog icon, which makes me wonder what else is secretly enabled when _isEnabled is false.

What is the use case for allowing Dev Tools to be toggled on and off? Should we only allow the toggle if _isEnabled is initially set to true?

@oliverfoster
Copy link
Member

We had it like this so that we could hide devtools, to give the course to QA or the client, without uninstalling the devtools plugin. Devtools would still be accessible for those who knew and wouldn't require complete uninstallation in the development period. The plugin was then removed before sending the course to the client.

What workflow is the property breaking that necessitates a change to this behaviour?

@swashbuck
Copy link
Contributor

What workflow is the property breaking that necessitates a change to this behaviour?

We usually publish courses with Dev Tools installed but set to _isEnabled: false. This adds an extra step to publishing courses if we need to remember to completely uninstall the plugin from courses before publishing. Also, as much as it is discouraged, often a PM or QA person will zip up courses for client delivery. This would be one more thing that we need to add to a checklist which, frankly, are not always followed anyway.

@oliverfoster
Copy link
Member

oliverfoster commented Aug 20, 2024

This file shows all of the ways to enable devtools when it is _isEnabled: false:
https://github.com/cgkineo/adapt-devtools/blob/master/js/enable.js

Typing kcheat on a keyboard:

if (buffer.substr(blen - ('kcheat').length, ('kcheat').length) === 'kcheat') {
if (!Adapt.devtools.get('_isEnabled')) enable();
}

Pressing 5 with the mouse button pressed:

if (isMouseDown && c === '5' && !Adapt.devtools.get('_isEnabled')) enable();

Running kcheat() in the console:

window.kcheat = () => {
buffer = 'kcheat';
processBuffer();
};

Visiting the url #kcheat:

Router.route('kcheat', 'kcheat', () => {
if (window.kcheat) window.kcheat();
});

Touch screen, top left and top right tap hold:

adapt-devtools/js/enable.js

Lines 110 to 112 in b8b77ca

if (topLeftTapHold && topRightTapHold) {
if (window.kcheat) window.kcheat();
}

It is a very deliberate design choice, not an accident, that _isEnabled: false only hides the cog.

@chris-steele will need to give input

@swashbuck
Copy link
Contributor

swashbuck commented Aug 20, 2024

It is a very deliberate design choice, not an accident, that _isEnabled: false only hides the cog.

If we don't want to remove this functionality, I like Tom's idea of creating a new option to just hide the button. Maybe call this _isButtonEnabled and it would work like _isEnabled does now. Then, _isEnabled would completely disable the plugin. That way, we can safely deliver a course by setting both _isButtonEnabled and _isEnabled to false.

maybe there should be two seperate properties: isEnabled to enable/disable the plugin (for consistency with other plugins), and then a seperate control to show/hide the icon?

@chris-steele
Copy link
Contributor

I'd like to see the functionality remain available via the 'secret' incantation as it is often very useful to invoke when testing from the front door, svn or even courses running on client systems.

Perhaps the change can be that if _isEnabled:false the button/cog will not appear at all and require the user to unhide it via browser developer tools? This would cover the scenario of a user accidentally enabling the functionality. I don't recall any user ever doing this in the 10 years this plugin has been around, if that can be of some peace of mind.

@oliverfoster
Copy link
Member

oliverfoster commented Aug 21, 2024

Perhaps the change can be that if _isEnabled:false the button/cog will not appear at all and require the user to unhide it via browser developer tools?

What do you think of @tomallpress suggestion that _isButtonEnabled could perform that function of disabling the button and _isEnabled would actually turn off the plugin? Is that going to make devtools substantially less useful in the wild?

it is often very useful to invoke when testing from the front door, svn or even courses running on client systems.

It sounds as though the ability to hide it and have it enableable is a reasonably useful and well used use-case?

From my perspective, I think the result seem almost identical. The person releasing would have to know to change _isButtonEnabled to false instead of _isEnabled to allow Chris' use-case, which still runs into Brad's problem - that people don't know which properties to disable.

I think I'd prefer Tom's other suggestion:

re-word the readme so it says isEnabled just shows/hides the icon, and clarifying it doesn't actually disable the plugin.

Then everyone is clearer and we can carry on as usual?

@chris-steele
Copy link
Contributor

chris-steele commented Aug 21, 2024

Is that going to make devtools substantially less useful in the wild?

If disabling the plugin means that it cannot be re-enabled at runtime, yes, it would be far less useful to me.

It sounds as though the ability to hide it and have it enableable is a reasonably useful and well used use-case?

From my point of view, again, absolutely. Today I have 'invoked' devtools several times.

Just seen your latest comment, yes, I'd vote for the readme change.

@joe-replin
Copy link
Contributor

joe-replin commented Aug 21, 2024

Also think about what may happen with a project months or years down the line when something needs to be resurrected. All of our devs & QA are trained to enable / disable devTools on final release. Even LD's need it when reviewing current and old content. It's so much easier for any Kineo staff member to be able to flip it on at any time. More than just technical people use this and I think we need to keep non-technical & non-developers into consideration on this because many find value in it.

  • There should be an option to completely disable it with a simple boolean.
  • I'm on board for an additional option just to disable the button.

@swashbuck
Copy link
Contributor

This could be a crazy conspiracy theory, but we had an issue with a handful of learners finishing a course in 5 minutes when it should have taken them 30+ minutes. The audience for this course is highly technical, so it's possible they realized they could enable Dev Tools and cheat their way through the course.

@oliverfoster
Copy link
Member

oliverfoster commented Aug 21, 2024

It is possible to complete any scorm course through the console without using devtools.
The only way to know if they used devtools is to collect data.

@chris-steele
Copy link
Contributor

5 minutes? If you know what to look for it should take seconds 😆

@danielghost
Copy link

Sounds like a readme change is agreed for _isEnabled, but did we also want to ensure the button doesn't appear as per Chris' comment?

Perhaps the change can be that if _isEnabled:false the button/cog will not appear at all and require the user to unhide it via browser developer tools? This would cover the scenario of a user accidentally enabling the functionality. I don't recall any user ever doing this in the 10 years this plugin has been around, if that can be of some peace of mind.

As this is a public repo, someone could look up the shortcuts if they really wanted to, or just inspect the code at runtime, at which point they probably would just use the SCORM API as mentioned above to complete anyway. For all these reasons I don't think it is critical to prevent this personally.

@swashbuck
Copy link
Contributor

I still want to flag that we have had some learners somehow cheat their way through courses in 5 minutes. If it's determined that learners have figured out how to use the "hidden" Dev Tools and the client finds out, we would have egg on our faces.

@chris-steele
Copy link
Contributor

I think it has been said already, but it is easy to cheat - any user with an ounce of technical ability can find the SCORM adapter or could read the component JSON and see how to answer test questions etc. We can't stop someone who is determined to cheat/circumvent controls.

@oliverfoster
Copy link
Member

We don't have any single json property which stops a component from being compiled into the course.

Regardless of _isEnabled true/false, all plugin code is compiled into the output.

Deleting a plugin from the course or using config.json:build.includes = [] to list the included plugins (as the AAT does), are the only ways of stopping a plugin from compiling into the output.

@oliverfoster
Copy link
Member

oliverfoster commented Jan 7, 2025

The navigation cog button does not appear in the DOM when _isEnabled: false and cannot be re-enabled with display: none overrides, see this code:

function initNavigationView() {
if (!Adapt.devtools.get('_isEnabled')) return;
if (navigationView) navigationView.remove();
navigationView = new DevtoolsNavigationView();
}

I have taken @tomallpress 's suggestion and reworded the schema and readme to explain what _isEnabled does more fully.

Pull request: #95

NOTE: in order to prevent the use of shortcuts to enable devtools, devtools must be uninstalled / removed from the project using either config.json:build.includes or by deleting the plugin or by on the AAT, removing it from the project.

@oliverfoster oliverfoster self-assigned this Jan 7, 2025
Copy link

🎉 This issue has been resolved in version 3.7.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants