This repository has been archived by the owner on Sep 12, 2022. It is now read-only.
Support viewing older requests #751
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cdosborn
force-pushed
the
support-older-requests
branch
from
61e4146
to
dc70a86
Compare
For collections of models we support `fetchModels` and `getAll`, for individual models we have `fetchModel` and `get`. However the latter methods don't work like you might hope. Problem: 1 If you call `get(modelId)` that checks if the model exists and then fetches the first page of the models from the server. If `modelId` isn't in the first page, then `get` won't work. Problem: 2 `fetchModel` actually fetches the modelId from the server, so theres no concern about pagination, but it sticks its result `this.models`. If this.models doesn't exist, then you'll get a runtime error. So `fetchModel` only works if you implicitly have called `getAll` or `fetchModels` first. Not nice. Solution to: 1 I created a `getModel` analog to `get` which actually fetches the model. I couldn't replace `get` because the current behavior is used in many places. Solution to: 2 I fixed `fetchedModels` in a backwards compatible way to prevent the runtime error. When it fetches a model it adds it to the model cache (like it used to) and it adds it to `this.fetchedModels`, which is a cache for model instances. This fixes the implicit need to call store.getAll() as mentioned.
There are a few caveats, I'll comment on the diff of this commit
When a resource request gets updated to approved/closed/denied, we no longer
remove it from the store, we show all requests of all status types now
diff --git a/troposphere/static/js/actions/AdminResourceRequestActions.js b/troposphere/static/js/actions/AdminResourceRequestActions.js
index 10dc13abd..f31263844 100644
--- a/troposphere/static/js/actions/AdminResourceRequestActions.js
+++ b/troposphere/static/js/actions/AdminResourceRequestActions.js
@@ -14,9 +14,6 @@ export default {
Utils.dispatch(AdminResourceConstants.UPDATE, {
model: request
});
- Utils.dispatch(AdminResourceConstants.REMOVE, {
- model: request
- });
});
return promise;
}
We want to make permalink work for admin resource requests. Normally we fetch
all resource request (AdminResourceRequestStore.getAll()) and lookup the
request id in the url from those requests. However, getAll only returns the
first page, sometimes the id is not in the url, so we sometimes fetch the
specific request.
diff --git a/troposphere/static/js/components/admin/ResourceMaster.jsx b/troposphere/static/js/components/admin/ResourceMaster.jsx
index d594c2643..111c9c2f7 100644
@@ -96,57 +94,48 @@ const ResourceMaster = React.createClass({
+ // Lookup selectedRequest by requestId, or make a separate request for
+ // the model specifically
+ let selectedRequest =
+ (requests.get(requestId) || stores.AdminResourceRequestStore.getModel(requestId));
We sort resource requests first by status then by creation date. The api
returns results sorted by creation date, that's implicit, not great.
diff --git a/troposphere/static/js/components/admin/ResourceRequest/ResourceRequestNav.jsx b/troposphere/static/js/components/admin/ResourceRequest/ResourceRequestNav.jsx
index 6eb23462b..72992a2f0 100644
--- a/troposphere/static/js/components/admin/ResourceRequest/ResourceRequestNav.jsx
+++ b/troposphere/static/js/components/admin/ResourceRequest/ResourceRequestNav.jsx
@@ -1,6 +1,13 @@
import React from "react";
import Backbone from "backbone";
+function sortByStatus(request) {
+ if (request.get('status').name == "pending") {
+ return 1;
+ }
+ return 2;
+}
...
@@ -51,7 +69,12 @@ export default React.createClass({
return (
<ul style={list}>
- { requests.map(this.renderListItem) }
+ {
+ // Requests are assumed to be sorted by start date, we then
+ // further sort to render the pending requests first (sortBy
+ // is stable)
+ requests.sortBy(sortByStatus).map(this.renderListItem)
+ }
When an admin views a resource request that has already been approved/closed/denied, require interaction to prevent accidental changing of resources. Not a huge fan of this approach, think we can redesign this view to be simpler, more intuitive, but the primary goal is to make the view more usable.
This fixes the not so nice experience of approving/denying reloading the app.
cdosborn
force-pushed
the
support-older-requests
branch
from
dc70a86
to
b7c39c0
Compare
|
Quick note, #750 has been merged now. |
cdosborn
force-pushed
the
support-older-requests
branch
from
b7c39c0
to
3f8fed6
Compare
|
Thanks, rebased onto latest master |
lenards
reviewed
Feb 20, 2018
| @@ -51,7 +50,7 @@ const ResourceMaster = React.createClass({ | |||
|
|
|||
| onSelect(request) { | |||
| // Navigate to a request at the top of the list | |||
| browserHistory.push(`/application/admin/resource-requests/${request.id}`); | |||
| this.props.router.push(`/admin/resource-requests/${request.id}`); | |||
There was a problem hiding this comment.
Connor knows this - but for folks reading and observing ... or, for inquiring minds - this change leverages that withRouter knows the basename for Troposphere is /application
|
✅ merged: cyverse/atmosphere#575 |
|
🎉 |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Depends on:
Problem: No way to view older requests.
Features:
Checklist before merging Pull Requests