freeCodeCamp / league-for-good

An open source sports league management tool
BSD 3-Clause "New" or "Revised" License
163 stars 106 forks source link

Adding a new player to a league does not update the player list #80

Open makkoli opened 6 years ago

makkoli commented 6 years ago

Players tab > Manually adding a player to league

After successful insertion, the roster does not update with the new player. It is being added to the list of unapproved player registrations. If the admin manually adds a player it should be immediately added to the list of all players.

afranco07 commented 6 years ago

I want to try and work on this

afranco07 commented 6 years ago

Hey I have this pretty much done, however I can't get the state to update the team roster without refreshing the page. Any help on which reducer to use for getting the latest team/league info?

IsaacRaymond commented 6 years ago

@afranco07 can you show what you have for this fix? I'm not an expert, but from my experience with react, proper changes to the state should automatically call the component to re-render

edit: Sorry, re-read your post. You're looking to change the state, not to get the component to re-render. I'll look and see if I can find which reducer is needed!

afranco07 commented 6 years ago

@IRGames yea I'm having trouble updating the state of the roster. There is a reducer UPDATE_TEAM under teams.js in the reducers folder, but I'm not sure if it is working correctly. And like I said, refreshing the page updates the state correctly

Here is some code I wrote attempting to update state. This is in the file createPlayer.js in this location client/src/actions/players/ in the creatPlayer(form, dispatch, props) function, starts on line 31. I may be missing something:

if ( team && team.teamId) {
    const leagueID = data.leagueId;
    axios.get(`${ROOT_URL}/league/fetch/${leagueID}`)
        .then(leagueData => {
            dispatch({
                type: EDIT_LEAGUE,
                payload: {
                    [leagueID]: { ...leagueData.data }
                }
            });
        });
}
IsaacRaymond commented 6 years ago

When you try adding a player, does it also "break" the app, like in these pictures? image After clicking "Add Player" image

IsaacRaymond commented 6 years ago

I'm really new to this app and fairly new to React, in general, but here's what I've got after studying this:

The type of state change that you're dispatching changes the entire league. I've tried to figure out why it causes the above glitch, but the reason hasn't come to me yet. I'm sure it has something to do with the fact that we're changing the entire state of the league when we're accessing a child of the league.

The variable we need to change in the state is "data.pending." Since we're the admin, this person isn't pending admin approval. It should be set to "true" instead of the default value of "false".

I've added one new line of code beneath line 20 of createPlayer.js: data.pending = false; I know a reducer is only supposed to change the state, nothing extra. Is there a special convention about actions? Am I doing something wrong by adding this line of code?

If you add that one line, the problem is fixed and the player is automatically approved.

afranco07 commented 6 years ago

@IRGames yes you are correct about the data.pending = false; and I already have some logic that checks if you are the admin for setting that. The problem is I need to somehow update the roster to include the new player you just created (without refreshing the page, just the state). If I go to the team roster, the player is not shown. However if I refresh the page then the player does show up in the roster. So there must be a way to get the state to update the roster, I just haven't found/figured it out yet.

Also it doesn't break like in the pictures you have above. There is an error that gets printed to the console but I don't exactly remember what it said. I'll be able to get that error for you tomorrow if you want.

I did get that error in the pictures before while working on something else. It happened when I was sending the wrong payload into the Reducer. Double check the data you're sending in

IsaacRaymond commented 6 years ago

Ok! If you could, I'd like to see the error. When my app crashed on my end, no error appeared.

I noticed one of the "To Dos" for the app was to "Finish writing out roles" (admin, coach, etc). When you get a chance, could you show me where you added the logic to check for admin? I was wondering if/how the app is currently set up to check a user's role. Thanks for talking things out with me here!

afranco07 commented 6 years ago

@IRGames I new to react and redux as well so this might not be the best way to do it but, here's a little snippet on how I am getting the users role. I get the current users email from redux state and find them under the staff array in the redux state (I save it into user variable). Then I check if the users role is Admin using the ternary if statement.

function mapStateToProps(state, ownProps) {
    // Getting the users role
    const email = state.auth.user.email;
    const staff = state.settings.staff;
    const user = staff.find(staffPerson => staffPerson.email === email);
    // cut out some code ...
    return {
        // removed some other code ...
        isAdmin: user.role === 'Administrator' ? true : false
    };
}

Then under createPlayer.js I have the line like you mentioned earlier:

player.pending = isAdmin ? false : true;

And the error I get is TypeError: can't assign to property "status" on "<insert long id number here>": not an object and it refers to the file teamData.js Line156:5

IsaacRaymond commented 6 years ago

@afranco07 Thanks for the reply! When you've been looking at the app, have you seen any other places where a user's role has been checked? I see that the Administrator, General Manager and Coach roles have been defined in roles.config.js, but I don't see any code that involves their implementation.

I ask because of the following "To Do" in writing out the roles: Finish writing out roles:

Administrator - has access to all tabs; has access to all permissions

Manager - has access to teams, players, seasons tab; access to all permissions in those tabs

Coach - has access to teams tab; has permissions view team players, input team scores after games

If we're going to check to see if there's an administrator like you've done above, we'll want to stay consistent and use the same format for checking a user's role throughout the app. Sounds like a big task. Should we ask someone at freeCodeCamp if the two of us can go ahead and start working on this implementation?

makkoli commented 6 years ago

@IRGames @afranco07 Hey guys, taking on the role task is a big ask. Significant parts of the app are in need of some refactoring to get it to work. You can join us on our slack at fcc-leagueforgood.slack.com. No one has been really using it, but I'm usually on slack everyday if you want to talk more about this.

afranco07 commented 6 years ago

@makkoli how do we sign up for the slack?

makkoli commented 6 years ago

@afranco07 Yeah, I actually forgot I need to invite you to the workspace. If you give me your email I can send you an invite.

makkoli commented 6 years ago

@afranco07 Sent.

IsaacRaymond commented 6 years ago

@makkoli isaac.w.raymond@gmail.com
thanks!

makkoli commented 6 years ago

@IRGames sent

paulywill commented 6 years ago

@makkoli Can this issue be closed?

afranco07 commented 6 years ago

@paulywill This issue has been fixed?

paulywill commented 6 years ago

@afranco07 It's working now. May have been a PR quite some time back.