Fix some issues with pinned messages #3414
Conversation
cammellos
requested review from
ilmotta,
MishkaRogachev,
Samyoul and
jrainville
Pull Request Checklist
|
Jenkins BuildsClick to see older builds (36)
|
cammellos
force-pushed
the
feature/pinned-messages
branch
from
2b29f40 to
51fd30b
Compare
|
As i remember @saledjenic and @osmaczko were working on pinned messages in desktop, so think it would be better to have a review from them |
cammellos
force-pushed
the
feature/pinned-messages
branch
from
51fd30b to
2892ba4
Compare
|
|
||
| Message *PinnedMessage `json:"pinnedMessage"` |
| type insertPinMessagesQueries struct { | ||
| selectStmt string | ||
| insertStmt *sql.Stmt | ||
| updateStmt *sql.Stmt | ||
| transaction *sql.Tx | ||
| } |
There was a problem hiding this comment.
I remember I once talked to @cammellos about the usage of a query builder library (not ORMs 
). For example http://doug-martin.github.io/goqu/ Nothing magical, just type safety, and a unified way to compose queries.
Right now, the problem I see is that concatenating strings is a suboptimal abstraction that leads to various ad-hoc solutions and certainly a lot of duplication (since it's hard to compose strings from different functions).
What are your thoughts @Samyoul? Would you like to see an experiment with such a library? Have you tried any library in this space?
There was a problem hiding this comment.
Fundamentally @ilmotta I agree with you. One of the first things I wanted to do with this repo was overhaul the database interactions because there were/are so many persistence packages, mostly written with pure SQL. There are already a number of places in this repo where we've implemented various ad-hoc query builder solutions.
Settings is one stand out example, and may even be a good candidate for an experiment with a query building library.
I'd be interest in how well joining, grouping, having and more complex queries could be handled, looking over the docs of goqu it seems that it has robust support for quite complex queries.
So another candidate could be user identity, as there is a single row for user data and multiple rows for the user images.
There was a problem hiding this comment.
Hey @Samyoul, sorry I couldn't reply because I didn't work last Friday 🎉
It's great to hear you're interested in seeing where this experiment could go. There's enough quorum! I'll try to cook something after I finish the feature I'm working on.
I'd be interest in how well joining, grouping, having and more complex queries could be handled, looking over the docs of goqu it seems that it has robust support for quite complex queries.
I'm curious too to understand where goqu "breaks" so to speak, because that would help us understand where it can be useful and where we should avoid it (if there is any such scenario).
So another candidate could be user identity, as there is a single row for user data and multiple rows for the user images.
Sounds reasonable! And we can do the improvements piecemeal since we can always convert the query back to a string.
| } | ||
|
|
||
| return | ||
| } | ||
|
|
||
| func (db sqlitePersistence) SavePinMessage(message *common.PinMessage) (inserted bool, err error) { |
There was a problem hiding this comment.
I really appreciate the use of a named return parameter in this case.
cammellos
force-pushed
the
feature/pinned-messages
branch
2 times, most recently
from
5af3563 to
a85a144
Compare
cammellos
force-pushed
the
feature/pinned-messages
branch
6 times, most recently
from
a0d23b6 to
76cdd97
Compare
There were a couple of issues on how we handle pinned messages: 1) Clock of the message was only checked when saving, meaning that the client would receive potentially updates that were not to be processed. 2) We relied on the client to generate a notification for a pinned message by sending a normal message through the wire. This PR changes the behavior so that the notification is generated locally, either on response to a network event or client event. 3) When deleting a message, we pull all the replies/pinned notifications and send them over to the client so they know that those messages needs updating.
cammellos
force-pushed
the
feature/pinned-messages
branch
from
76cdd97 to
03ce01a
Compare
There were a couple of issues on how we handle pinned messages:
client would receive potentially updates that were not to be
processed.
message by sending a normal message through the wire. This PR changes
the behavior so that the notification is generated locally, either on
response to a network event or client event.
and send them over to the client so they know that those messages
needs updating.
There's a couple of tests I would like to add, but in the meantime I think it's ready for review.