MarchosiasM / irlgamers

MIT License
0 stars 3 forks source link

Do we need smart component (constructor or lifecycle) on EventDetailPage.js? #45

Closed dimstar closed 6 years ago

dimstar commented 6 years ago

I encourage anyone who wants to take a look at this should. Here is the issue on how to reproduce:

  1. go to site root in browser 1.1 ex: http://localhost:8000/
  2. find an event you have not authored. make certain it has slots available.
  3. Click the event title to go to the event detail page 3.1 ex: http://localhost:8000/games/ultimate-werewolf-party-arooooooooo0000
  4. Click join 4.1 ISSUE: '0/5' (or however many the event has) slots doesn't reflect the joined user, click again and the number will increase.

It is my thinking that if we had a constructor or possibly lifecycle method we could set this initial state and allow it to propogate immediately without having to call the updateAttendee action function more than once.

dimstar commented 6 years ago

@rperez2021 @MarchosiasM Just wanted to call this out to you guys.

MarchosiasM commented 6 years ago

So, there's a number of issues wrapped into this one.

  1. The number of attendees doesn't update upon the initial click.
  2. An attendee is allowed to join more than once
  3. The events page doesn't display how many attendees are participating
  4. The events page will still show an event that is full

I suspect the first issue is a matter of the page not re-rendering. Which is odd because it does update upon the second click, but just one behind.

This issue is reproducible by leaving the event and returning. Even if the event is 8/10, if you click it won't update, but will add you as an attendee, but won't update until subsequent clicks. So perhaps there's a problem with the way we initially render the component.

The second issue I think may need two ways to manage it. FIrst of all a user who has already joined an event shouldn't see the join button. So perhaps in the rendering of the component we do a quick loop to check and see if the user is amongst the users attending and set a state variable to true and hide that button. Also put logic in the click that sets that to true. Solves that without back-end logic, but it probably wouldn't kill us to make sure that that doesn't get through even if the front end logic fails us and the API routes go through in spite of our best efforts.

Third issue is easy enough. Probably can go in as a minor effort when we try and make that main events display more robust.

Fourth issue is even easier to handle than issue three. Filtering out full events would be trivial.

MarchosiasM commented 6 years ago

What IS working, is someone CAN join, and when a component thinks that the event is full it will hide the join button, though I was under the impression that it'd render some string telling the user that the event is now full.

dimstar commented 6 years ago

2 - 4 I consider minor feature updates/bugs.

the big one is the ISSUE 1 you point out. This is because it has larger implications for how the component is built or how we might need to refactor.

MarchosiasM commented 6 years ago

I'll take a look later, but I suspect it's because we're ignoring a proper redux dispatch or call somewhere. All of those calls should make react rerender and it's not happening, so one of us dropped the ball, perhaps with the placeholder code I tossed in one of my lovely ternaries somewhere.

This is an error I suspect we'd weed out with proactive refactoring and avoiding anti-patterns that I've been ignoring in favor of just getting the gears moving.

dimstar commented 6 years ago

Fix: We don't need a smart component here! See this commit

@MarchosiasM found that the issue was not in the client but in the server server/controllers/even.controller.js. I was using a method which was returning the original document from mongodb. Event.findOneAndUpdate() Returns the original, un-updated mongo document from the collection.

The event document being returned to state from our apiCall() was always one 'state' behind the real time desired document we were tying to pass into application client state.