-
Notifications
You must be signed in to change notification settings - Fork 250
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
Resolves #1017, checks _isLocked in router #1018
Conversation
This prevents routing to a locked page/menu via the URL, or menu link which has not yet been updated to support _isLocked. Also, Adapt.findById() has been updated to return a warning when the _id value passed in does not exist in the mapping.
This PR should go in once #1013 is merged. |
I feel very strongly that this is something that needs to be configurable (a bit like the Being able to bypass the locking via URL is indispensable for testing/development purposes. If this were merged in as-is it would add quite a lot of faff to development/testing time for us (having to constantly switch locking on/off) - whilst adding very little in the way of benefit since 99% of our courses are presented to the user in a popup window with no address bar, leaving them unable to navigate via the URL anyway. |
This enforces the _isLocked property in the router when set to true.
@moloko, I've added a |
|
||
if (!currentModel) { | ||
return; | ||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unnecessary else block
- Removed unnceccessary ELSE - Allowed re-routing on an invalid _id - Defaulted navigation to course homepage if Adapt.location._previousId is unset
👍 |
I've just tried this on the current version of the framework (together with @tomgreenfield's changes from #1013) and, when I run the course, it triggers the bookmarking dialog together with the following message in the console:
Clicking 'Yes' in the bookmarking dialog takes me to http://localhost:9001/#/id/undefined If I disable spoor via config.json, it does not do this. Equally it doesn't do this if spoor is enabled but I'm using the current (master) version of adapt.js - so it's likely to be the amend in there not playing nicely with the offline_API_wrapper.js in spoor - which is returning I suspect a bit of defensive programming needs to be added to |
Hi @moloko, thanks for testing, however after looking at it this is more a problem with the Bookmarking extension itself storing the string "undefined" in the Also, it appears somebody at least caught the error thrown by core here: A more correct solution would be to also check the model returned by |
fair point, checking for the string |
I think if we're to add parameter validation like this we should probably do it for all functions and create a separate ticket for that. The function already returns early if the collection type can't be determined, as would happen with an invalid id/index. |
+1 |
@brian-learningpool - Please take a look at
These check would become useless as What do you think? |
Hi @hrajotia, I think the second link is fine as the |
questionView L483 💭 |
@@ -481,7 +481,7 @@ define(function(require) { | |||
try { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you remove the try catch also?
+1 |
2 similar comments
+1 |
+1 |
This prevents routing to a locked page/menu via the URL, or menu link which has not yet been updated to support _isLocked.
Also, Adapt.findById() has been updated to return a warning when the _id value passed in does not exist in the mapping.