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.

Sign up for GitHub

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

Jump to bottom

add back sessionDate to api call. #285

Merged
merged 2 commits into from Oct 23, 2017
Merged

add back sessionDate to api call. #285

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a90e774
add back sessionDate to api call.
jselden Oct 20, 2017
e68cd03
add back sessionDate to api call. made code review change
jselden Oct 23, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 8 additions & 2 deletions src/core/api/user-permissions.js
View file
Open in desktop
@@ -1,3 +1,4 @@
import moment from 'moment';
import ngModule from '../module';

const AvUserPermissionsResourceFactory = function(AvApiResource) {
Expand All @@ -11,7 +12,11 @@ const AvUserPermissionsResourceFactory = function(AvApiResource) {
version: '/v1',
name: '/axi-user-permissions'
});

/**
* sessionDate used by api to determine if server side cache is out of date.
* i.e if user cache on server is older then sessionDate it will repull user information.
*/
this.sessionDate = moment().toISOString();
Copy link
Collaborator

Choose a reason for hiding this comment

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

This will cache the sessionDate to when the instance of AvUserPermissionsResourceFactory is instantiated. All of the calls afterwards would have the same sessionDate. Is this what is wanted or should it create a new sessionDate value when the calls is made (or eventually expire)?
A problem I could see happening is if this sessionDate is old (the user has been on the page for a little while) and this is older than the cache on the server this will trigger the server to not repull even though the cache on the server was out-of-date. And if that is the case, you would be better off caching the inital results in memory on the client (not saying that is a good idea, just saying that it would avoid unneeded network calls since you could determine that the server would return the same cached results).

Also, new Date().toJSON() does the same thing and is native to the browser.
moment tends to get overused and while it is nice for parsing, comparing, and fancy formatting and is already included in the project, it would be nice to only use it when the browser doesn't already natively support the feature.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The behavior is as desired that only bust cache on server potentially on initial load. Subsequent calls would hit cache. I agree that is essentially same as caching on client, but currently not sure good way to change back in logic without significant risk. This change was how it was working before in 1.x version due to issues with switching region and server not having the must current region for user. I will make change to new Date()

}

afterQuery(response) {
Expand All @@ -22,8 +27,9 @@ const AvUserPermissionsResourceFactory = function(AvApiResource) {

return this.query({
params: {
region,
permissionId: permissionIds,
region
sessionDate: this.sessionDate
}
});

Expand Down