-
Notifications
You must be signed in to change notification settings - Fork 16
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
Add support for news articles to GraphQL endpoint #3008
Conversation
4fdee73
to
82c4a18
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No real issues with the code itself here, but a few more thoughts/questions on the generic/specific debate that it would probably be good to discuss alongside the simplification PR when we mob
@@ -4,10 +4,11 @@ module Types | |||
class QueryType < Types::BaseObject | |||
field :edition, EditionTypeOrSubtype, description: "An edition or one of its subtypes" do | |||
argument :base_path, String | |||
argument :content_store, String, required: false, default_value: "live" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: interface
- Will
content_store
be the most obvious name for this argument for frontend devs/in client code (versus e.g.state
orpublishing_state
)? - Do you know what the difference is between
content_store
,state
, andphase
(and why iscontent_store
the correct one to use here?)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, we're gonna have to face this decision at some point! I wonder if there's already a Content Store-free name for this concept in use?
class NewsArticleType < Types::EditionType | ||
def self.document_types = %w[ | ||
government_response | ||
news_story | ||
press_release | ||
world_news_story | ||
] | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thought: approach
Given the lack of type-specific fields, I'm curious if there's a benefit to this versus just falling back to the generic EditionType
app/graphql/types/edition_type.rb
Outdated
field :available_translations, [Translation] | ||
|
||
def available_translations | ||
Presenters::Queries::AvailableTranslations.by_edition(object) | ||
.translations.fetch(:available_translations, []) | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thought: genericness
Is this link type available/present on all editions/types? If not, I've wondered before about a mixin approach for shared links that aren't quite generic enough to appear everywhere (e.g. include AvailableTranslationsLinks
in the relevant type classes). This feels related to the conversation over on the other PR!
Similar thoughts on the next commit - where/if to draw the line between generic and specific/how precise to be with our typing
At the moment, GraphQL queries only return the live edition of a document. This change allows users to request either the draft or live version of the document.
We are implementing news articles, which have many document types. Therefore we need to allow a GraphQL type to be found from multiple document types.
This creates a news article type, which inherits from the Edition type. There are no fields specific to this type, so everything can be inherited from the generic edition.
This link is used by multiple types of edition, so we should move it to the edition type, to reduce the need for duplicating the same in each type.
These are used on most editions, so can exist in a shared space.
82c4a18
to
d4cc096
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yndajas, I've made the changes that you suggested and left your other comments unresolved. (I've linked to this pull request in the notes for our mobbing session, mentioning the relevance of the discussion here.)
This allows us to retrieve news articles using a GraphQL query.
Trello card