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

unfurling: add settings module and dev mode conversation storage system CORE-9242 #14556

Merged
merged 11 commits into from Nov 3, 2018

Conversation

Projects
None yet
2 participants
@mmaxim
Member

mmaxim commented Nov 2, 2018

Patch does the following:

1.) Introduces a new interface ConversationBackedStorage with two simple accessor methods with the purpose if being implemented by a chat conversation.
2.) Add DevConversationBackedStorage which writes TEXT messages into DEV conversations, and always uses the contents of the latest message in the return type. Data is JSON encoded into the chat text messages.
3.) Add unfurl.Settings which manages the unfurling settings via a ConversationBackedStorage.
4.) Add simple methods and new types for tracking settings.

@mmaxim mmaxim requested a review from joshblum Nov 2, 2018

@mmaxim mmaxim changed the title from unfurling: add settings module and dev mdoe conversation storage system CORE-9242 to unfurling: add settings module and dev mode conversation storage system CORE-9242 Nov 2, 2018

@joshblum

small style comments, overall looks good i like the storage abstraction

Show resolved Hide resolved go/chat/devstorage.go Outdated
Show resolved Hide resolved go/chat/devstorage.go
Show resolved Hide resolved go/chat/unfurl/settings.go
Show resolved Hide resolved go/chat/unfurl/settings.go Outdated
@mmaxim

This comment has been minimized.

Member

mmaxim commented Nov 2, 2018

@joshblum ok made assertLoggedInUID change, let me know if you accept my other replies.

mmaxim added some commits Nov 3, 2018

@mmaxim

This comment has been minimized.

Member

mmaxim commented Nov 3, 2018

Going to merge since I think I hit all the feedback, let me know if there is anythign else.

@mmaxim mmaxim merged commit 5531f96 into master Nov 3, 2018

2 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/jenkins/pr-head This commit looks good
Details

@mmaxim mmaxim deleted the mike/CORE-9242 branch Nov 3, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment