Hey Milo,
Thanks for tackling this set of issues and for doing good work. Being able to deligate work and have it come back is a good feeling :)
I've read the code and pulled the graphql_additions branch in order to run the queries. This is a good first draft but there are a few tweaks that need to be made.
I'll format this as a numbered checklist so that when discussing a particular point it can easily be referenced / fixed.
We should settle on some conventions for naming our variables, etc., so that individual developer code is indistiguishable from one to the next. I've added a section to the ReadMe document for establishing this. We can add to it as ideas occur. Once we've settled some of these things we can do a quick refactor of existing code.
- [ ]
Object variables called new_object or this_object vs straight up object. Ex:
new_dog = Dog.new
this_dog = Dog.find_by_name("Otis")
#vs
dog = Dog.new
My recommondation is to take our cues from rails convention:
# GET /dogs/new
def new
@dog = Dog.new
end
def set_dog
@dog = Dog.find(params[:id])
end
# GET /dogs
def index
@dogs = Dog.all
end
... using instance variables only if necessary (creating a private getter / setter for example).
Let me know what you think. I don't have a particulary strong opinion on what convention we settle on, as long as we settle on something.
Request standardized naming convention for object variables.
- [ ]
Right now there are few _id fields being represented as Int types. This works, at least in the case of auto incrementing ID's, however, GraphQL has an ID standard type meant for this purpose. See here and here for a little more info on the topic.
Request switching these types over to match with GraphQL's standard.
- [ ]
With the exception of sign_in_user.rb return values differ from file to file. I like the way you've been doing yours as it reduces the reduncy of having to declare a bunch of fields in a mutation (see the current create_circle.rb for an example of this). With that in mind, I'm suggesting the following standard:
field :model, Types::Model::ModelType, null: false
def resolve
model = Model.new( ... )
# ...
return { model: model }
end
Two things of note:
- There is only a single field, one that matches the base return object.
- There is an explict return statement. I know this is not the "Ruby Way", but I believe that explicit return statements are clearer and more readable. Once again, I'm not highly opinionated, but we should settle on a way of writing our functions so that they are consistent across the code base.
Request standardized field and return values.
- [ ]
We should settle on a consisten MOA when it comes to declaring attribute values. Ex:
name = args[:input][:name]
user = User.create!(
name: name
)
#vs
user = User.create!(
name: args[:input][:name]
)
I've done both in this code base, so I probably need to refactor. (Check create_user.rb vs update_care_recipient.rb) for an example.
I'm not strongly opinionated. I think the trade off is between between more lines or longer lines. I find both pretty readable. The one drawback I see in the first option (name = args[:input][:name]), is that we are setting up a variable to be used only once. That seems wasteful. My personal thoughts are to only do this if a line is getting long and unwieldy. What's your take?
Request standardized mass assignment practice.
- [ ]
When creating associations between related tables there a few options. So far we've been using two:
BioEntry.new(circle_id: 1)
BioEntry.new(circle: current_circle)
My thoughts on the matter. Because we are not using strong parameters, we should eventually establish a whitelist for which attributes are eligable for mass assignment. Here is an article on the topic.
In my opinion, circle_id is the more error prone of the two as it the only requires an integer to be set.
To create a valid association using the object assignment attribute, a valid object must already be handy.
BioEntry.new(circle: 1)
# => ActiveRecord::AssociationTypeMismatch
Request standardized mass assignment attributes.
- [ ]
There are few inconsistant tab and spacing issues across some of these files (mutation_type.rb and query_type.rb). I'm not sure how this snuck through the linter. I've manually adjusted them in github already, but that may need to be redone due to refactored code getting pushed up on the same branch.
As well, the resolve function in delete_bio_entry.rb is difficult to read. Would recommend some empty lines to break it down into paragraphs (more on that function below, however).
There is some commented out code in delete_bio_entry.rb. I think it's safe to just remove that entirely.
Request file cleanup.
- [ ]
As a matter of readability I'd like to keep the order of fields presented in the same order, file to file. In particular, I'd like to keep critical data, like id at the top.
Request that field order for GraphQL Types be presented in the same order they appear in the schema, with any relevent id fields being listed at the top.
Currently created_by is not a foreign key. This opens up the possibilty of orphaned records. As well, the desired functionality:
BioEntry.create(
created_by: current_user
)
...does not yet exist.
Request that the relevant migration files be updated to establish created_by as a foreign key:
add_foreign_key :bio_entries, :users, column: :created_by
- [ ]
The field name bio is confusing given that 'bio' refers to a collection of 'bio_entries' (bio is to bio_entry as tasks is to task). Request that this be renamed bio_entry.
- [ ]
Currently, the bio_entries table is making use of postgreSQL's array functionality. However, each item in that array should also be a foreign key as it represents a User primary key.
The desired functionality would be something like the following:
bio_entry.seen_by << current_user
bio_entry.seen_by # => return an array of <User Objects>
Unfortunately, this is not possible, using a postgres array.
Request that the seen_by array be removed entirely
A new issue will be created specifying the need and specs for a join table.
A seen_by method can then be written to retrieve the appropriate user objects from this table.
That way, we can take notice of that bio_entry's category and disable any tasks for the current_user/category until they acknowledge that they've seen the new bio_update (and been added to the above array).
Additionally as it currently sits, seen_by is being set equal to the current_user object directly so it isn't taking advantage of the array in the first place.
- [ ]
With the exception of description and circleId, the only thing being returned is an error message:
mutation {
createBioEntry(
bioEntry: {
category: food
description: "Needs almonds in his soup"
}
){
bio {
seenBy {
firstName
}
}
}
}
{
"data": {
"createBioEntry": {
"bio": null
}
},
"errors": [
{
"message": "Cannot return null for non-nullable field BioEntryType.seenBy"
}
]
}
This same error occurs for all other fields that are supposedly available:
mutation {
createBioEntry(
bioEntry: {
category: food
description: "Needs almonds in his soup"
}
){
bio {
createdAt
}
}
}
{
"data": {
"createBioEntry": {
"bio": null
}
},
"errors": [
{
"message": "Cannot return null for non-nullable field BioEntryType.createdAt"
}
]
}
Request full funcionality be made available.
- [ ]
If the server output is monitored while issuing the createBioEntry mutation, (or use the rails console to search for added data), it can be seen that the database does not get touched. This is due to BioEntry.new() being used as opposed to create.
Request resolve function be updated to persist provided data.
- [ ]
Although this is technically available, it is a utility method resulting from the the association between the bio_entries table and the circles table. It's meant for backend use only. The frontend doesn't need the option of accessing this information as it already has access to the uuid of the current circle.
Request removal of circle_id field from bio_entry type
- [ ]
I like your implmentation of this function. However, despite the fact that the graphql-ruby docs recommend treating error messages as data, the front end operates with the following functionality:
if loading
#...
else if error
#...
else
#...
end
Given this, it makes sense to provide an error in the form of raise "Error Message Here" rather than the result piece of data.
I do think this is a good way to implement a successful deletion message though.
def resolve(args)
bio_entry = BioEntry.find_by_id(args[:id])
raise "No Bio Entry found with this ID" if bio_entry.nil?
bio_entry.delete.reload
raise "Failed to delete Bio Entry" if bio_entry
return {message: "Bio Entry successfully deleted."}
end
Request errors be raise for failures and refactor for readability
- [ ]
Given the fact that no data is being altered, this should be a query instead of a mutation. This could then potentially be refactored as follows:
def show_bio_entry(args)
bio_entry = BioEntry.find_by_id(args[:id])
raise "No Bio Entry found with this ID" if bio_entry.nil?
return {bio_entry: bio_entry}
end
Request relocation to queries_type.rb and refactor
- [ ]
As this is also a request for data that doesn't alter anything, it should go in the queries section.
Request this be moved to queries_type.rb
- [ ]
I can't get it to work in it's current state:
mutation {
showBio {
bio {
description
}
}
}
{
"error": {
"message": " Failed to implement ShowBioPayload.bio, tried:\n\n - `#<Class:0x00007f3b9dd110c0>#bio`, which did not exist
...
Request the following functionality:
query {
showBio {
bioEntries {
id
description
category
createdBy
createdAt
updatedAt
seenBy
}
}
}
If I had to guess what the problem/solution is, my guess would be that it is the return value of the resolve function.
The field name, (in this case bio) needs to refer to a method on whatever is being returned. Right now, an array of BioEntry objects is being returned. To make this work, I think that only the current circle should be returned.
def resolve
return context[:current_circle]
end
Then call the field bio_entries (referring to the bio_entries method on Circle). This should return a list of bio entries (I think).
I know this seems like a big list of fixes, but I want to be make sure that you know your work is appreciated. Thanks for your efforts. :)