app: add panel to list UPS instances #1255

jonathaningram · 2017-10-09T02:02:27Z

Edit: fixes #1001

This panel appears under the "Services" panel:

There's no CRUD for this one so far: just listing out the UPSI that are bound to the app and those that are in the space:

It's worth noting that the second section is not "unbound UPSI" - it's showing all UPSI in the space that are not bound to the app. In fact, it might be showing UPSI that are bound to another app. It would be nice to make this a bit better, but we're limited by what the CF API returns (it doesn't tell us if the UPSI is bound). So without doing some transformations ourselves, I found the best way to do this quickly was to just label the second section as "Others in the space".

I imagine it's useful to know the GUID of the UPSI too - for now I've just put it in the title attribute in order to avoid cluttering the UI with GUIDs.

jcscottiii · 2017-10-12T20:12:34Z

static_src/components/upsi/upsi_panel.js

+    );
+  }
+
+  renderOther() {


renderOther is kind of generic.
change renderOther to something like renderUnbound

How about renderOthersInSpace? It's not quite "unbound" per:

It's worth noting that the second section is not "unbound UPSI" - it's showing all UPSI in the space that are not bound to the app. In fact, it might be showing UPSI that are bound to another app. It would be nice to make this a bit better, but we're limited by what the CF API returns (it doesn't tell us if the UPSI is bound). So without doing some transformations ourselves, I found the best way to do this quickly was to just label the second section as "Others in the space".

jcscottiii

@jonathaningram This looks good.
Could you add some unit tests for the upsi_actions and upsi_panel?

jonathaningram · 2017-10-16T10:23:24Z

@jcscottiii ready for review again.

jcscottiii · 2017-10-17T20:07:53Z

static_src/actions/env_actions.js

@@ -1,6 +1,6 @@
 import AppDispatcher from '../dispatcher';
 import { envActionTypes } from '../constants';
-import cfApi from '../util/cf_api.js';


is there a reason your removed the .js?

@jcscottiii yep, we don't need the .js suffix for importing modules. Same reason that the two lines above don't have the suffix, e.g. import AppDispatcher from '../dispatcher';.
Related: we shouldn't need to be naming files with .jsx and subsequently having to specify that prefix when importing those modules either. It's not really that useful. Even the Airbnb guide doesn't use the .jsx suffix for importing: https://github.com/airbnb/javascript/tree/master/react#naming

Also related: we're not following their guideline for naming components. E.g. we are currently using import ReservationCard from './reservation_card.jsx'; and we should be using import ReservationCard from './ReservationCard'; if we follow Airbnb's guide (and what's most commonly used in React projects).

jcscottiii

@jonathaningram you don't need to rethrow the error. In Redux, you create a matching error handling action.
Now, if the actions aren't doing appropriate things, that's a different problem.

jonathaningram · 2017-10-17T21:43:42Z

@jonathaningram you don't need to rethrow the error. In Redux, you create a matching error handling action.
Now, if the actions aren't doing appropriate things, that's a different problem.

@jcscottiii see Dan Abramov's comments on this:

The first one is the best reference, and I'll paste in the most important paragraphs here:

Problem:

Even something as simple as an undefined variable gets swallowed using .catch which makes for very hard development workflow.

Solution:

I can update the code to not rethrow, but change to using the second argument to .then instead of using .catch. I was only rethrowing because I wanted to show the error and wanted to change the current behaviour as little as possible. This was/is essentially a stepping stone.

Related: that this problem is slightly improved in React 16, so we should look at upgrading in the near future.

By .catching these promises (and others that are not yet fixed), this is actually masking any error that is thrown, not just things that arise from the promise in question failing. This causes a lot of issues. For example, the test in app_action.spec.js that is changed in this commit was not working as intended because the cfAPI.fetchApp call was never being stubbed - and the .catch handler was masking the fact that the cfAPI was actually trying to make a real call. Many other errors are masked because of these .catch handlers which makes development much harder.

jcscottiii · 2017-10-18T13:36:16Z

@jonathaningram let's leave it for react 16 (just create an issue about it). since react 16 will kind of take care of those weird cases like undefined variables, i would like to keep the actions handling all other errors.

jonathaningram · 2017-10-18T21:23:03Z

@jcscottiii ok reverted the error handling change. Ready for review.

jcscottiii

just a question but LGTM

jcscottiii · 2017-10-19T17:06:37Z

static_src/actions/upsi_actions.js

+  return Promise.resolve({ items });
+};
+
+fetchAllSuccess.exportTest = true;


what are these (this line and the other exportTest) needed for?

I'll remove it for now. It's an idea I was toying with.

Rationale: testing JS modules often means you need to export certain things so that the test file can import them to test. Unlike in Go where it can test both exported and unexported members, with JS modules, we're forced to export all the things if we want to test them. However, I don't want callers to use these. E.g. fetchAllSuccess should never be directly called. So I toyed with the idea of setting exportTest = true as an indicator that it shouldn't be used. Of course, to actually enforce it we would need tooling: e.g. maybe eslint can error when it's imported and used. Also even though the bandwidth would be minimal, we could also use a babel plugin to strip them out. I didn't really look for existing solutions to this - I just made this up as I was writing the test.

Anyway, I'll just remove it for now, but it's something that we could revisit later out of interest.

OK @jcscottiii removed.

jonathaningram · 2017-10-24T09:42:39Z

@jcscottiii let me know if there's anything pending with this? I feel like we're good to go but I may have missed a change request.

jonathaningram force-pushed the ji-app-upsi-panel-2 branch from 9a38ffe to 0c81cd8 Compare October 9, 2017 05:27

jcscottiii reviewed Oct 12, 2017

View reviewed changes

jcscottiii suggested changes Oct 12, 2017

View reviewed changes

jonathaningram force-pushed the ji-app-upsi-panel-2 branch 2 times, most recently from 1e5fcd4 to 4cd8b22 Compare October 16, 2017 08:52

jcscottiii reviewed Oct 17, 2017

View reviewed changes

jcscottiii suggested changes Oct 17, 2017

View reviewed changes

jonathaningram and others added 7 commits October 18, 2017 16:14

app: add panel to list UPS instances

aeb581c

upsi: rename method to renderOthersInSpace

8f83ac3

upsi: add test for UPSIPanel

7eb3685

upsi: add test for actions

d0c82ce

test: stop asserting if spy is not called

8be71c7

Fix missing export in ElasticLineItem

ba78f50

jonathaningram force-pushed the ji-app-upsi-panel-2 branch from 3e61df8 to ba78f50 Compare October 18, 2017 05:16

jonathaningram mentioned this pull request Oct 18, 2017

Upgrade frontend to React 16 #1263

Closed

Revert rethrowing errors in actions

39ac4b0

jonathaningram force-pushed the ji-app-upsi-panel-2 branch from 56bc506 to 39ac4b0 Compare October 18, 2017 21:22

jcscottiii reviewed Oct 19, 2017

View reviewed changes

Remove exportTest experiment for now

f8a871f

jcscottiii approved these changes Nov 4, 2017

View reviewed changes

jcscottiii merged commit 95bda81 into cloud-gov:master Nov 4, 2017

jonathaningram deleted the ji-app-upsi-panel-2 branch November 5, 2017 22:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

app: add panel to list UPS instances #1255

app: add panel to list UPS instances #1255

jonathaningram commented Oct 9, 2017 •

edited

Loading

jcscottiii Oct 12, 2017

jonathaningram Oct 12, 2017

jcscottiii left a comment

jonathaningram commented Oct 16, 2017

jcscottiii Oct 17, 2017

jonathaningram Oct 17, 2017

jcscottiii left a comment

jonathaningram commented Oct 17, 2017

jcscottiii commented Oct 18, 2017

jonathaningram commented Oct 18, 2017

jcscottiii left a comment

jcscottiii Oct 19, 2017

jonathaningram Oct 19, 2017

jonathaningram Oct 19, 2017

jonathaningram commented Oct 24, 2017

app: add panel to list UPS instances #1255

app: add panel to list UPS instances #1255

Conversation

jonathaningram commented Oct 9, 2017 • edited Loading

jcscottiii Oct 12, 2017

Choose a reason for hiding this comment

jonathaningram Oct 12, 2017

Choose a reason for hiding this comment

jcscottiii left a comment

Choose a reason for hiding this comment

jonathaningram commented Oct 16, 2017

jcscottiii Oct 17, 2017

Choose a reason for hiding this comment

jonathaningram Oct 17, 2017

Choose a reason for hiding this comment

jcscottiii left a comment

Choose a reason for hiding this comment

jonathaningram commented Oct 17, 2017

jcscottiii commented Oct 18, 2017

jonathaningram commented Oct 18, 2017

jcscottiii left a comment

Choose a reason for hiding this comment

jcscottiii Oct 19, 2017

Choose a reason for hiding this comment

jonathaningram Oct 19, 2017

Choose a reason for hiding this comment

jonathaningram Oct 19, 2017

Choose a reason for hiding this comment

jonathaningram commented Oct 24, 2017

jonathaningram commented Oct 9, 2017 •

edited

Loading