-
Notifications
You must be signed in to change notification settings - Fork 114
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
Paging for history view #233
Conversation
data.forEach(function(resource){ | ||
resource.type = type; | ||
resource.modified_date = new Date(resource.modified_date); | ||
if (type === 'services') { |
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.
nit: maybe my lack of knowledge on JS, curious how easy/difficult it is to make this into a const
. My worry is that it's easy to mistype "services", "credentials", "blind_credentials", and would be easier to spot mis typing / errors if we used a variable.
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.
heh. yeah, it would be way easier to have it set as a const and I really wish it was. I'll followup later to make that happen.
} | ||
|
||
this.setLimit = function(limit) { | ||
_this.limit = limit; |
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.
curious, what does this do? wondering why we need a wrapper to do this.
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.
You can't access the scope from within a service, but we get the limit from the server side, which gets added to the scope at the top level app config: https://github.com/lyft/confidant/blob/master/confidant/public/modules/app.js#L24-L34
This change refactors the history view to support paging, and also to update the history list efficiently when a resource is reverted.
A new setting
HISTORY_PAGE_LIMIT
is added. By default it isn't set, keeping the original behavior of the endpoints, and the UI. Setting a limit will limit the number of results the revision history endpoints will return. They'll also return a next_page attribute along with the results, which can be passed in for the next set of results. This value is also exposed to the UI via theget_client_config endpoint
, and is accessible in the angular code via$scope.clientconfig.generated.history_page_limit
.The history UI no longer shows a combined history with checkboxes to filter in/out credentials, blind_credentials and services, but instead shows a navigation list for each. The reasoning for this is that loading in pages items would not show the interleaved history accurately, as each type is being fetched separately, and each may be edited at different frequencies. Splitting them out also makes it possible to fetch them individually through the "Load more" button.
The initial fetch of the history does a fetch of each type, but the default for the view is credential history.