issue 956 - exclude orphaned stories #971
Conversation
Thanks for the feedback @rudokemper. Pushed updates for both of these! |
|
@ice1080 You think you can rebase your branch so we dont have the conflicts? |
ice1080
force-pushed
the
956-exclude-orphaned-stories
branch
from
68abff7
to
f682903
Compare
|
@julinvictus rebase should now be complete 👍 |
| speakers: [speaker_1], | ||
| permission_level: :anonymous | ||
| ) | ||
| end |
There was a problem hiding this comment.
I'd rather utilize the existing story/place/speaker setup rather than adding new ones. I know it makes it easier to conceptually read "place_being_deleted" and the like, but it does slow down the tests considerably since we are forcefully creating and recreating all of these stories, places, and speakers for each test.
| expect(response.body).not_to include('This story should not be visible because it has no speaker') | ||
| expect(response.body).not_to include('This story should not be visible because it has no place') |
There was a problem hiding this comment.
This works; however, if you wanted to make use of the json_response helper, we can set this test up similar to the one you had in the api spec:
expect(json_response["stories"].length).to eq(0)There was a problem hiding this comment.
Hey @lauramosher, this test is actually not an api test, so the json_response doesn't parse the body since it's html instead of json. Let me know what you'd prefer, Rudo had asked me to create that test so that you had tests on the page contents instead of just the api tests.
There was a problem hiding this comment.
Ah, I see where I misunderstood. I noted that the json_response helper was included in this file and assumed we were testing a JSON endpoint.
Rudo is correct in that we need a test for this request as it's the one that had the issue. I believe Rudo was ensuring that we tested both the public API and the internal Home map story results since they are powered by two separate controllers.
The home request is powered by a partial that renders JSON (_home.json.jbuilder) and is completely separate from the public API endpoints available in /api/.
Like I said, this works, so I'm not super worried about changing it. If you're interested/wanted to, we could use rspec's type: :view attribute and test that this request renders the _home partial and we would then be able to assert on the JSON instead of grepping through HTML blobs.
|
My bad on submitting that without a comment. @ice1080 — thank you so much for your contribution! I apologize for the mixed signals here as there have been a lot of changes going on that have changed the landscape of the work you have been doing for us. This looks great and the code fix for the home rendering of the map is a great help! |
|
I'll get to work on these changes @lauramosher. Looks like someone did half the work of this story in some unrelated issue(s), which is why half my work and tests are no longer valid, which are most of your comments, since I created this PR over a month ago. |
|
@ice1080 I know. I am so sorry for the late review. |
|
@lauramosher pushed my changes. Please see my comments in the above conversations and let me know if you'd like anything else changed. Some of the conversation around this with Rudo was in the issue itself - #956 |
|
@ice1080 Thank you so much! Do you mind doing another rebase to clean up some of the unrelated changes in this PR? |
ice1080
force-pushed
the
956-exclude-orphaned-stories
branch
from
8ab9dbf
to
dfcdcfc
Compare
|
Hey @lauramosher , sorry for the late response to this. I believe it should be good now, let me know if that's not the case |
For issue #956