Skip to content
This repository has been archived by the owner on Oct 11, 2022. It is now read-only.

Add watercooler to community settings #4823

Merged
merged 12 commits into from
Mar 6, 2019
Merged

Conversation

mxstbr
Copy link
Contributor

@mxstbr mxstbr commented Mar 6, 2019

Status

  • WIP
  • Ready for review
  • Needs testing

Deploy after merge (delete what needn't be deployed)

  • api
  • hyperion (frontend)

Related issues (delete if you don't know of any)
Closes #4799

Todo

  • Add enableCommunityWatercooler mutation to backend
  • Add disableCommunityWatercooler mutation to backend
  • Add card to community settings with "Enable" button
  • Make it possible to disable the watercooler from the frontend
  • Make sure toggling the watercooler on and off does not create more than one (will probably require a new index on the thread table)

@brianlovin
Copy link
Contributor

Make sure toggling the watercooler on and off does not create more than one (will probably require a new index on the thread table)

I wouldn't mind if this is a bit slower if it means we don't have to add another index to the threads table...that table already has a lot of indexes. Why not just a getAll by communityId and then filter for { watercooler: true } ? - at most you'd be filtering on a few hundred or a thousand records, right?

@mxstbr
Copy link
Contributor Author

mxstbr commented Mar 6, 2019

Why not just a getAll by communityId and then filter for { watercooler: true } ? - at most you'd be filtering on a few hundred or a thousand records, right?

That is much more database intensive than just adding another index 😅 The only tradeoff of using an index is that it uses up more storage, but we are not even close to strapped for hard drive disk space at all, so it should be fine to add another one.

@brianlovin
Copy link
Contributor

The only tradeoff of using an index is that it uses up more storage, but we are not even close to strapped for hard drive disk space at all, so it should be fine to add another one.

Fair, but it does impact writes to that table the more we add there. Although we're probably still small enough that this doesn't matter right now.

@mxstbr
Copy link
Contributor Author

mxstbr commented Mar 6, 2019

That's fair, but I think given how high our percentage of reads vs. writes is adding an index is a worthy tradeoff in most cases. I have added a communityIdAndWatercooler index to the threads table and handled re-enabling watercoolers in the latest commits, last thing here is the disable mutation and then this should be good for a first review 👍

@mxstbr
Copy link
Contributor Author

mxstbr commented Mar 6, 2019

@brianlovin this is ready for a review! 🎉

@brianlovin
Copy link
Contributor

Awesome! @mxstbr can you poke at that e2e failure, and then I'll test :D

@mxstbr
Copy link
Contributor Author

mxstbr commented Mar 6, 2019

Done, this is ready for a test & review! 👍

@brianlovin
Copy link
Contributor

@mxstbr I pushed some tweaks, the biggest of which is I don't auto-redirect when enabled. This was super jarring for me. Instead, let's just show a button so they can open it if they want, otherwise they can keep editing other settings.

@mxstbr
Copy link
Contributor Author

mxstbr commented Mar 6, 2019

Nice experience polish

@brianlovin brianlovin merged commit 6b84315 into alpha Mar 6, 2019
@brianlovin brianlovin deleted the watercooler-settings branch March 6, 2019 16:46
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants