Skip to content
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

Fix bug in content loading for story queries #25

Merged
merged 1 commit into from
Jun 13, 2021
Merged

Conversation

e-wai
Copy link
Collaborator

@e-wai e-wai commented Jun 12, 2021

Notion ticket link

Fully load content on storyByID

Implementation description

  • I don't know why this fixes anything I'm sorry

Steps to test

  1. Set up your DBs, make sure that you have at least one story
  2. Test query:
{
  storyById(id: 2) {
    id
    title
    contents {
      id
      storyId
      lineIndex
      content
    }
  }
  stories {
    id
    contents {
      id
      storyId
      lineIndex
      content
    }
  }
}
  1. Make sure contents field is not null on either! Either empty array "contents": [] or something like
"contents": [
  {
    "id": 1,
    "storyId": "1",
    "lineIndex": 0,
    "content": "Line 1"
  },
  {
    "id": 2,
    "storyId": "1",
    "lineIndex": 1,
    "content": "Line 2"
  }
]

What should reviewers focus on?

Checklist

  • My PR name is descriptive and in imperative tense
  • My commit messages are descriptive and in imperative tense. My commits are atomic and trivial commits are squashed or fixup'd into non-trivial commits
  • For backend changes, I have run the appropriate linters: docker exec -it <backend-container-id> /bin/bash -c "black . && isort --profile black ."
  • I have requested a review from the PL, as well as other devs who have background knowledge on this PR or who will be building on top of this PR

@e-wai e-wai requested a review from gaoxk June 12, 2021 07:35
Copy link
Contributor

@gaoxk gaoxk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aha! Interesting, here's why to_dict was having trouble. We need to pass it include_relationships=True to get the contents.
We should still err on the side of using to_dict to make sure we're never returning more information than necessary. Returning the model object directly runs that risk.

@e-wai
Copy link
Collaborator Author

e-wai commented Jun 12, 2021

Hmm it's still not working when I toggle include_relationships to True

@gaoxk
Copy link
Contributor

gaoxk commented Jun 13, 2021

🤔 are you sure? It works locally for me
changed lines are

return [result.to_dict(include_relationships=True) for result in Story.query.all()]
...
return story.to_dict(include_relationships=True)

@e-wai
Copy link
Collaborator Author

e-wai commented Jun 13, 2021

Ah, I was making changes to StoryContent instead of Story. Should be fixed!

Copy link
Contributor

@gaoxk gaoxk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@e-wai e-wai merged commit b773321 into main Jun 13, 2021
@e-wai e-wai deleted the bug/story-content branch June 13, 2021 22:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants