-
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
Save player names as the user changes them #57
Conversation
What if the user starts typing a player name before making any hero selections? We want to save "zioncross" as the first player for that comp, but the only thing tying a player to a composition is the player_selections table, which requires a map segment and a hero selection. No longer!
Tests are passing:
|
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.
This is a great start @cheshire137! I'm marking as approved as I don't want to hold you up if you are happy with this direction, but I left some thoughts and suggestions. There appears to be a bug in the position assignment for new CompositionPlayer
records at the moment.
@@ -0,0 +1,33 @@ | |||
require 'rails_helper' | |||
|
|||
RSpec.describe CompositionPlayer, type: :model do |
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.
Is there a reason you prefer this style, rather than describe CompositionPlayer do
?
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.
This is what RSpec generated when I did rails g model CompositionPlayer
. I don't have a preference really.
hero_id = player_selection.hero_id | ||
player_name = player_selection.player.name | ||
result[hero_id][player_name] ||= [] | ||
result[hero_id][player_name] << player_selection.map_segment_id | ||
end | ||
|
||
result | ||
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.
In get_map_segment_ids
, I think we're doing more work than we need to. If I'm following this correctly, it looks like the JSON builder views for CompositionsController
are only interested in the heroes for which a player has been created. If that's the case, we should be able to reduce
the composition.player_selections
into the result
hash:
composition.player_selections.includes(:player).reduce({}) do |result, player_selection|
hero_id = player_selection.hero_id
player_name = player_selection.player.name
result[hero_id] ||= {}
result[hero_id][player_name] ||= []
result[hero_id][player_name] << player_selection.map_segment_id
result
end
If there's another reason we're requiring all Heroes to be populated first in the result hash, please ignore this. I'm not seeing the reason for that though based on the return value from the JSON request.
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.
If there's another reason we're requiring all Heroes to be populated first in the result hash
For the convenience of the JSON views, so we don't have much logic in the views. When I tried your code, I got a undefined method '[]' for nil:NilClass
error on last_composition.json.jbuilder:25
I believe because @map_segment_ids[hero.id]
is nil so we can't grab @map_segment_ids[hero.id][player.name]
from it. We could do a check in the view "is there a hash at hero.id in map_segment_ids?", but that's logic I'd rather have elsewhere, like the controller.
player_name = player_selection.player_hero.player.name | ||
result[hero_id][player_name] = player_selection.map_segment_id | ||
hero_id = player_selection.hero_id | ||
player_name = player_selection.player.name |
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.
Might be nice to add a helper method for player_name
on PlayerSelection
in the same spirit as hero_id
.
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.
If we end up using this more than once, I'll do it!
def set_position | ||
return unless composition | ||
|
||
self.position ||= composition.composition_players.count |
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.
I'm not sure we want to memoize here. This seems to have adverse affects on the position calculated for new players added to a composition. When memoized, the CompositionPlayer
records created for me always have a position of 0
. Was there a specific goal you had in mind with memoizing 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.
The goal wasn't memoization here but rather to set a default value only if the caller didn't explicitly pass a position. I want the caller to be able to override the position, and that's what this allows: if position
is already non-nil, it doesn't get set to the default value of how many other players are in this composition.
the CompositionPlayer records created for me always have a position of 0
Were you using the same composition for each new CompositionPlayer you created? If not, I'd expect it to always be 0.
There's a model test, sets position
, that shows the position defaults to something >0 as more players are added the composition.
errors.add(:composition, 'has maximum number of players already.') | ||
end | ||
end | ||
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.
It would be lovely to add some tests for CompositionPlayer
🙇
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.
end | ||
|
||
true | ||
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.
I feel like this is pretty complex to read. There is a mix of declarative and imperative code here that makes the conditional branching a bit much to load into the brain (or maybe that's just how I feel on a Saturday) 😛 ?
I think one thing that would help in this context is to consider treating players
as a select list rather than a text field in the team composition react component. Then we could give the user an option for create player
, which would allow us to take care of player record creation independently of team composition record creation. I think that also would be a nice feature to have regardless for users, so they always have a select list of their previously created players
when they switch to a new map to create a new composition. What do you think?
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.
I feel like this is pretty complex to read.
I agree! I feel like this is an improvement over the logic being in the controller, but I think we can continue to improve this model.
so they always have a select list of their previously created players
I agree! The idea had come up before of autocompleting player names based on past compositions, or when you start a new composition you get text fields already filled with your past player names. Just moving to select menus sounds easier, though. I'll file an issue!
Fixes #51.
I discovered a problem with our logic for storing a
player_hero_id
onplayer_selections
: it doesn't let us update the hero selection for a given player in a composition at a particular map segment. So I split the key intoplayer_id
andhero_id
and put a unique index onmap_segment_id
+composition_id
+player_id
so we disallow having the same player on multiple heroes for the same point in a given composition. Discussed with @rewinfrey to confirm my thinking.Another gap in our data model was we didn't allow for the team organizer to set a player name without first having made a hero selection for that player. So I could enter all my players' names and reload the page, and none of them would be persisted. I added a
composition_players
table to tie players in a particular order to the composition.This replaces the throttle-debounce npm package with react-debounce-input since it's built to work with React and I was able to get it working with the normal
value
andonChange
properties you should use on aninput
.This also refactors CompositionsController#save so it makes use of a new CompositionSaver model. This pulls a lot of logic out of the controller and makes things easier to read.
Here's the new functionality; note the page reload in the middle and how the data fills back in: