Skip to content
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

Improve notifications about users joining a game #130

Open
jongrim opened this issue Jun 30, 2023 · 5 comments · May be fixed by #268
Open

Improve notifications about users joining a game #130

jongrim opened this issue Jun 30, 2023 · 5 comments · May be fixed by #268
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@jongrim
Copy link
Contributor

jongrim commented Jun 30, 2023

Is your feature request related to a problem? Please describe.
The notifications when someone joins one of your games, it lacks enough detail to understand what session was joined. This can lead to what look like duplicate notifications if someone joins multiple sessions.

Describe the solution you'd like
The notification should include information about the session joined

Describe alternatives you've considered
None

Additional context
Example of two legitimate notifications that look the same, though the user joined two different sessions
Screenshot 2023-06-30 at 4 32 14 PM

@jongrim jongrim added enhancement New feature or request good first issue Good for newcomers labels Jun 30, 2023
@mikeminutillo
Copy link

I had a look at this. The biggest challenge I can see is converting to the game owner's timezone, which we don't have on the server. Would it make sense to pass the actual timestamps in the custom_fields property and render them client side?

Not sure how that would fit in with the email part. I haven't found that yet :)

I will try and get the app running in a dev environment and give this approach a go.

@jongrim
Copy link
Contributor Author

jongrim commented Feb 23, 2024

Thanks for looking at this! I don't think we need the game owner's timezone - what were you seeing that led you down that path?

My thought for these was that the notification should include the session number that the user joined. So something like "H Guy joined session 2 of your game, game_name".

That string is built in notifyGameCreator in the processRsvp.ts file. Currently, the main handling function has the specific session being joined, but it's not aware of what session that is in the sequence. I think the change needed is to load all sessions for the game rather than the single one, and then we should be able to improve the string by adding language about which session was joined. Does that make sense? Any other ideas?

While here, I think it'd be useful to also go ahead and attach the joiner's user ID to the notification. I want to expand the ability to view other's profiles in the future, so that will set it up to be easy to do so later. It also enables potentially showing the joiner's avatar and other info. That joining user's ID could go into the custom_fields part when creating the notification record. That will make it available in the UI later.

How does that sound? Feel free to suggest alternatives or other ideas, but that is what I had planned in my head. Lastly, I need to iron out the details on how others can run the project locally and develop, so I'll look into that this weekend and will get back soon!

@mikeminutillo
Copy link

I was thinking of adding the date/time to the notification.

USER has joined your game GAME starting DATETIME.

Session number could be useful, but as a game creator I would probably need to backport that to a date/time anyway. Especially if I am running two one-shots of the same game on the same day (which could happen).

@jongrim
Copy link
Contributor Author

jongrim commented Mar 11, 2024

Ah, I see! I do like that idea. Very nice and readable which was the goal.

So yes, you're original idea is the right one, take the session start_time (which is a unix timestamp) and add that to the custom_fields portion of the notification record so it will be available on the client. On the client side, when it builds the display for that notification type, it can use date-fns to format the time into a human readable string which will use the current browser timezone.

I did confirm that local dev should be good to go. Check out the developing doc in /docs to get started, and let me know if there's any questions!

@mikeminutillo mikeminutillo linked a pull request Apr 5, 2024 that will close this issue
@mikeminutillo
Copy link

I got everything working and raised a PR to address this #268

The layout is a bit naff. I messed with it for a bit but you can probably move things around quicker than I can.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants