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

Conversation

@mxstbr
Copy link
Contributor

@mxstbr mxstbr commented Feb 20, 2019

Status

  • WIP
  • Ready for review
  • Needs testing

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

  • api

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

Together with #4683 this will get us very close to the "activity feed" concept we want to get to.

Questions

  • Rather than storing the message directly in the db should we go through the addMessage mutation?

@brianlovin
Copy link
Contributor

What happens if the thread gets deleted later? Do we leave the dead link?

@brianlovin
Copy link
Contributor

  • Rather than storing the message directly in the db should we go through the addMessage mutation?

This is probably the better option, but hard to do purely through the backend without passing through the entire graphql resolver args. But that way we at least have consistent logic for adding messages.

@mxstbr
Copy link
Contributor Author

mxstbr commented Feb 21, 2019

  • Yeah we just leave a dead link right now. I will see if I can clean that up but honestly, I do not care that much
  • I will see what I can do about going through addMessage!

@brianlovin
Copy link
Contributor

Not too concerned about leaving the dead link either I suppose - the thread preview won't load anyways

@mxstbr
Copy link
Contributor Author

mxstbr commented Feb 22, 2019

Exactly 👍

Max Stoiber added 2 commits February 25, 2019 09:43
We can use this refactored `addMessage()` method to add messages
programmatically while skipping all the permission checks and stuff like
that!

/cc @brianlovin can you give this a good review to make sure this makes
sense?
@mxstbr
Copy link
Contributor Author

mxstbr commented Feb 25, 2019

Latest commits refactor the addMessage mutation into a reusable util that we then use from the publishThread mutation to post new threads to the watercooler.

@brianlovin want to give this a good review and test to make sure I did not mess anything up? Otherwise this would basically be ready to ship, pending #4683.

@mxstbr
Copy link
Contributor Author

mxstbr commented Feb 25, 2019

Also changed the posted message to:

screenshot 2019-02-25 at 09 46 31

How's that look? Would you prefer we add a isBot field to messages that renders a bot badge next to the team badge?

@brianlovin
Copy link
Contributor

How's that look? Would you prefer we add a isBot field to messages that renders a bot badge next to the team badge?

Hm, honestly not a fan of the latest implementation there - it just feels like this weird middleground between being a system-message and looking like a user actually typed it. I think I'd be surprised if I saw that show up in a chat feed when I published a thread.

I think the simplest thing here that helps a tiny bit is to just have it only post the link, or maybe just "New thread: {url}" - simple, and doesn't add any expressiveness that might be misconstrued as the user's.

A bot label would help here, but maybe the simplest thing actually is to just create SAM and have SAM post the message. That way it has its own name and we could add a [bot] badge to it...not sure how this would work with private communities or private channels, or if permissions matter for a bot like this.

Either way, I think we're close but not quite there on making this feel right.

@mxstbr
Copy link
Contributor Author

mxstbr commented Feb 26, 2019

Agreed on those thoughts, I think this was the wrong direction.

Latest commit adds a migration for the @spectrumbot user called "Sam" and uses that user to post new threads to the watercooler. I also made it so first-time-posters get a different message:

screenshot 2019-02-26 at 10 16 42
screenshot 2019-02-26 at 10 16 09

What do you think about that?

@brianlovin
Copy link
Contributor

Will this @ mention trigger a notification to the thread publisher? Ideally it wouldn't, since that would be bit annoying.

@brianlovin
Copy link
Contributor

Otherwise, I'm much happier with this implementation and then we can just find a good profile photo for Sam

@brianlovin
Copy link
Contributor

Although maybe instead of Sam we should actually just call it 'Spectrum Bot' for now, to align with the username and not cause any confusion about the user

@mxstbr
Copy link
Contributor Author

mxstbr commented Feb 27, 2019

Awesome! I will confirm re: notification but I do not think it will with the way I have split up addMessage. Will also rename to Spectrum Bot, good point.

/cc @superbryntendo do we have a nice profile photo for Sam somewhere?

@mxstbr
Copy link
Contributor Author

mxstbr commented Feb 27, 2019

It does not trigger a notification, and the latest commit renames the bot to Spectrum Bot!

@brianlovin
Copy link
Contributor

@mxstbr here's the SAM profile photo Bryn made a while back:

https://www.dropbox.com/s/znxozpr359anmk2/SAM%20Button.png?dl=0

@brianlovin
Copy link
Contributor

@mxstbr I put that on S3, you should be able to set the profile photo as 'default_images/sam.png' and imgix will handle the rest!

@mxstbr
Copy link
Contributor Author

mxstbr commented Feb 27, 2019

Nice, done! 😍 This is what Sammy looks like now:

screenshot 2019-02-27 at 12 35 39

Might need to get a round version of the profile photo, but that can be fixed with a quick S3 upload—which means this is ready once #4683 is ready!

@brianlovin
Copy link
Contributor

Whoa why are those edges getting cropped like that? A square profile photo shouldn't have this problem in the first place...

@brianlovin
Copy link
Contributor

To expedite this, I went ahead and uploaded a round profile photo...really confused why a square one would get cropped like that though. Once we merge #4683 (pending a small merge conflict if you can grab it!) let's ship this today.

@brianlovin brianlovin merged commit 12b0474 into alpha Feb 28, 2019
@brianlovin brianlovin deleted the post-new-threads-to-watercooler branch February 28, 2019 00:13
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.

Post new threads to the watercooler automatically

3 participants