Reader | Add loading screen to welcome gate #2718
Conversation
mmlumba
changed the title
mmlumba
changed the title
mmlumba
changed the title
|
|
||
| export class CaseSelectLoadingScreen extends React.Component { | ||
| componentDidMount = () => { | ||
| // We append an unneeded query param to avoid caching the json object. If we get thrown |
There was a problem hiding this comment.
Out of scope for this PR, but @mdbenjam it seems like we should fix this in Rails, not here. Shouldn't Rails be smart enough to do caching on a per-request-type basis? Are we doing something wrong that it caches the JSON response for the HTML request type?
There was a problem hiding this comment.
It's not a Rails problem, it's a browser problem. I'm almost certain this request doesn't even make it to Rails.
There was a problem hiding this comment.
I'm not sure, but I'm a bit skeptical of that. The browser only caches things when Rails tells it that it can. If the request doesn't even make it to Rails, that's because Rails sent a cache header that was too aggressive.
| routedCaseSelect = () => { | ||
| return <CaseSelectLoadingScreen | ||
| assignments={this.props.assignments}> | ||
| <div> |
There was a problem hiding this comment.
Why do we need this div?
|
I haven't done a full review, but this basically looks good. Nice work! |
| // to a page outside of the SPA and then hit back, we want the cached version of this | ||
| // page to be the HTML page, not the JSON object. | ||
| if (this.props.assignments) { | ||
| this.props.onInitialDataLoadingFail(false); |
There was a problem hiding this comment.
this initialDataLoadingFalse parameter was designed for the other loading page. I think we should make another variable and action to change it.
There was a problem hiding this comment.
I can add another variable/action for this loading page, but why would we want to make it different for the loading pages?
| <PageRoute | ||
| exact | ||
| title="Assignments | Caseflow Reader" | ||
| <Route |
There was a problem hiding this comment.
I think you should leave an exact here. It works because this is the second route hit, but if we ever reorder them it could cause a problem.
| import { mount } from 'enzyme'; | ||
| import _ from 'lodash'; | ||
|
|
||
| import { CaseSelectLoadingScreen } from '../../../app/reader/CaseSelectLoadingScreen'; |
There was a problem hiding this comment.
Nice tests, it would be really cool to mock out the API call and make sure we call the actions:
this.props.onReceiveAssignments
this.props.onInitialDataLoadingFail
There was a problem hiding this comment.
I am hesitant to do mocking because I've seen those tests become very brittle and low-ROI in the past: https://medium.com/@nickheiner/testing-javascript-8c8efe8434e.
I would rather we start with a simple capybara test, and only add mocking here if we feel like there are things that cannot effectively be tested from capybara.
There was a problem hiding this comment.
Didn't we agree that we can mock out at the xhr level? My understanding from this ticket: #2333 is that we'd be moving in this direction for Reader to try it out.
I see what you're saying in your post about brittle-ness, but I think our team wastes a lot of time on capybara tests. They take a long time to run and they break on a regular basis. We definitely have enzyme tests that mock out too much stuff (I'm guilty of that), and I've seen the brittleness of those tests. But moving to only mocking out the actual API call seems like a reasonable place to be.
Finally, for this particular case I don't think there is a good way to test this in a feature test. First, there are two paths, one involving a successful result from the server, and one involving a failure. You can't test both without mocking out something anyway. Second, how long the loading screen is up, is a pretty non-deterministic thing. How can we reliably test that the loading spinner appears and then the page loads. What if it loads in .3 seconds? Will Capybara be able to verify that?
There was a problem hiding this comment.
Ok, I'm fine with doing mocking here in that case. I agree that we had taken a stance that some mocking was ok. And I agree that the timing makes this difficult.
Some of the mocking that I was criticizing in that blog post is mocking of things that are totally internal to the module. Mocking out what require returns is very bad. However, asserting that functions passed to props are called at certain times is part of the public API of the code, and thus testing it is more reasonable.
This adds the lovely loading spinner to the Welcome Gate.
Connects #2618