-
Notifications
You must be signed in to change notification settings - Fork 2
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
feat(journal-crud): Basic Journal CRUD #57
feat(journal-crud): Basic Journal CRUD #57
Conversation
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.
Overall PR looks good. Just a few nits and then it'll be good to go 🚀
required: false, | ||
}, | ||
|
||
policies: { |
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.
Hey, @d-e-v-esh we had also planned to have a policyType
field right?
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.
Shall I rename policies
to polityType
?
b79812c
to
de00a34
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.
Shall I rename
policies
topolicyType
?
@d-e-v-esh Not really. policyType
would be a property of policies
The policies
object will have the following properties 👇🏻
policies {
title,
firstYear,
lastYear,
policyType
}
Out of which, only lastYear
property can be named as required:false
and rest as required: true
Also I noticed that you discarded the lastYear
property from policies
whereas I intended to say that we can leave as it is, without the required: true
property. Please revert this change as well.
Lmk in-case of any queries.
This commit contains the following: - Updated Journal model and typeDefs - getJournalByISSN Query - deleteJournal Mutation - updateJournal Mutation
There are actually more fields that are needed to be added under policies {
isDataAvailabilityStatementPublished: Boolean;
isDataShared: Boolean;
isDataPeerReviewed: Boolean;
enforced: Boolean;
enforcementEvidence: String;
} After we merge user authentication into the main |
de00a34
to
a378e92
Compare
I updated it. Let me know if there are any other changes. |
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.
LGTM 🎉
#54
This PR builds on the
journalResolver
and adds more functionalities. Now we can do basic CRUD operations with the journal entity. More will be added on top of it in forthcoming PRs.Changes
model
andtypeDefs
getJournalByISSN
QuerydeleteJournal
MutationupdateJournal
MutationFlags
createdAt
,updatedAt
andcreatedBy
do not auto-populate yet. This will be added in future PRs.