MushroomObserver / mushroom-observer

A website for sharing observations of mushrooms.
https://mushroomobserver.org
MIT License
77 stars 25 forks source link

Error showing non-existent User #2080

Closed JoeCohen closed 3 months ago

JoeCohen commented 3 months ago

Showing a non-existent user throws an Error. To reproduce:

We probably should go to the user index with the following flash error. (I think -- but not 100% certain -- we used to do that.) runtime_object_not_found: "Sorry, the [type] you tried to display (id #[id]) does not exist. Someone may have deleted it or merged it into another."

JoeCohen commented 3 months ago

@nimmolo: Off the top of your head, do you think this one is related to recent changes to site & user stats? (Cf. #2078)

nimmolo commented 3 months ago

@JoeCohen — It's definitely related somehow. I notice from your PDF that the error is thrown in Checklist, which i also worked on. The raise error line there on line 39 is a Joe line — I was wondering about that. Maybe that should throw to the user index instead?

It seems like it shouldn't ever be hitting that line though, for a nonexistent user... What I think should happen is that it should hit find_user! in UsersController#show, which is basically my eager-loading version of find_or_goto_index. (In various controllers, I've been rewriting the guard line in the show action to perform the functionality of find_or_goto_index, but allow for eager-loading, using a model scope. Maybe there's some way to pass the scope to find_or_goto_index?)

The other question is, it seems like we should probably be creating a blank UserStats for every new user. Does that seem right?

JoeCohen commented 3 months ago

@nimmolo

  1. Nathan deserves credit for line 39, which irignally used fail instead of raise error. https://github.com/MushroomObserver/mushroom-observer/blame/113d1064d99f8e7bb75a911755324c4495c2fef2/app/classes/checklist.rb#L31. I just rubocopped it.
  2. I'm also puzzled why it hits that line; find_or_goto_index feels like it should display the index and cease processing create. But it doesn't. Instead it returns nil, with the result that @show_user is set to nil, and create goes on its merry way with a nil @show_user.
  3. A blank UserStats for new users seems right. Would it work instead (or should this happen additionally) to make the default UserStat#checklist an empty string, with null: false?
nimmolo commented 3 months ago

@JoeCohen I'm out the rest of the day, but:

  1. agree, it's very strange, i have a feeling it will not be too hard to debug though.
  2. I see what you mean, the checklist column has no default value. But I think maybe it should allow null, because if populated it should be a serialized hash. Hmm. Will think about it some more.