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

Resolves #1017, checks _isLocked in router #1018

Merged
merged 7 commits into from
Apr 27, 2016
Merged

Resolves #1017, checks _isLocked in router #1018

merged 7 commits into from
Apr 27, 2016

Conversation

brian-learningpool
Copy link
Member

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 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.
@brian-learningpool
Copy link
Member Author

This PR should go in once #1013 is merged.

@moloko
Copy link
Contributor

moloko commented Apr 14, 2016

I feel very strongly that this is something that needs to be configurable (a bit like the _force config option for start pages) - with it switched off by default.

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.
@brian-learningpool
Copy link
Member Author

@moloko, I've added a _forceRouteLocking attribute to the config model, defaulted to false.


if (!currentModel) {
return;
} else {
Copy link
Member

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
@himanshu1618
Copy link
Contributor

👍

@moloko
Copy link
Contributor

moloko commented Apr 20, 2016

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:

Adapt.findById() unable to find collection type for id: undefined

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 undefined for the lesson_location

I suspect a bit of defensive programming needs to be added to Adapt.findById so that it will return if if finds that the id argument is set to anything like undefined, "undefined", null or "null"

@moloko moloko added this to the v2.0.9 milestone Apr 21, 2016
@brian-learningpool
Copy link
Member Author

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 restoredLocationID, as opposed to an empty string or a null value. I don't think we should be doing string comparisons to check for null/undefined values.

See here:
https://github.com/adaptlearning/adapt-contrib-bookmarking/blob/master/js/adapt-contrib-bookmarking.js#L39

Also, it appears somebody at least caught the error thrown by core here:
https://github.com/adaptlearning/adapt-contrib-bookmarking/blob/master/js/adapt-contrib-bookmarking.js#L53

A more correct solution would be to also check the model returned by Adapt.findById() is not null or undefined.

@moloko
Copy link
Contributor

moloko commented Apr 21, 2016

fair point, checking for the string "undefined" is probably out of scope for this code (I'm just used to the days when IE would use "undefined" by default so got into the habit of checking for this as well) - but I still think it could do with exiting if id is null or undefined

@brian-learningpool
Copy link
Member Author

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.

@moloko
Copy link
Contributor

moloko commented Apr 21, 2016

+1

@himanshu1618
Copy link
Contributor

himanshu1618 commented Apr 22, 2016

@brian-learningpool - Please take a look at

These check would become useless as Adapt.findById might not be throwing any error but now just returning undefined instead.

What do you think?

@brian-learningpool
Copy link
Member Author

Hi @hrajotia, I think the second link is fine as the continue is exiting the for loop. I've updated the code you highlighted anyway as the empty catch in the first example was bugging me.

@oliverfoster
Copy link
Member

oliverfoster commented Apr 22, 2016

questionView L483 💭

@@ -481,7 +481,7 @@ define(function(require) {
try {
Copy link
Member

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?

@himanshu1618
Copy link
Contributor

+1

2 similar comments
@dennis-learningpool
Copy link
Member

+1

@moloko
Copy link
Contributor

moloko commented Apr 27, 2016

+1

@moloko moloko merged commit 0bbe41f into master Apr 27, 2016
@moloko moloko deleted the issue/1017 branch April 27, 2016 15:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants