Page MenuHomePhabricator

[lib] Added new MessageSpec for editing messages
ClosedPublic

Authored by kuba on Mar 7 2023, 1:39 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Jun 22, 3:15 PM
Unknown Object (File)
Thu, Jun 20, 12:33 PM
Unknown Object (File)
Wed, Jun 19, 11:26 PM
Unknown Object (File)
Thu, Jun 13, 12:51 PM
Unknown Object (File)
Wed, Jun 12, 3:30 PM
Unknown Object (File)
Wed, Jun 12, 1:10 AM
Unknown Object (File)
Wed, Jun 5, 11:54 AM
Unknown Object (File)
Fri, May 31, 7:34 PM
Subscribers

Details

Summary

Added new MessageSpec for editing messages. Registered it in the MessageSpecs.

Test Plan

Checked if code compiles. Run app on the web and native. Then added new message with new type to database, and checked if the app still works, and loads properly the new message type by logging info to the console. Couldn't check yet all scenarios (like creating new message type locally on client side)

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

ginsu requested changes to this revision.Mar 9 2023, 10:21 AM

I think we should consider if we also need shimUnsupportedMessageInfo and unshimMessageInfo. These functions will help us support older native clients that haven't been updated yet.

lib/shared/messages/edit-message-spec.js
39 ↗(On Diff #23476)
55 ↗(On Diff #23476)

Should this be row.content.text?

Also in reaction-message-spec I parsed row.content into JSON before returning it? should we do the same here?

110–112 ↗(On Diff #23476)

This can be simplified to generatesNotifs: async () => undefined,

This revision now requires changes to proceed.Mar 9 2023, 10:21 AM
kuba marked 3 inline comments as done.

Responded to comments

In D6963#208218, @ginsu wrote:

I think we should consider if we also need shimUnsupportedMessageInfo and unshimMessageInfo. These functions will help us support older native clients that haven't been updated yet.

Yes. You are right. I will add it in another diff.

lib/shared/messages/edit-message-spec.js
55 ↗(On Diff #23476)

I store the new version of the message right in the content column in the database. For this reason, parsing it into JSON isn't necessary.

Here is a screenshot from the DB:

Screenshot 2023-03-13 at 11.24.47.png (92×2 px, 111 KB)

Is it possible we'll want to add some metadata to the content field later? I think it would be safer to use JSON

Added shimming message info, changed content to JSON format.

I don't know how to test message shimming yet, so for now it isn't tested.

lib/shared/messages/edit-message-spec.js
121–123 ↗(On Diff #23707)

I wasn't sure what version should I add here.

In D6963#209487, @kuba wrote:

I don't know how to test message shimming yet, so for now it isn't tested.

The proper way of testing older clients involves running an older client in prod mode (so that it won't get hot reloaded when we update the code version), then checking out the newest code, and running the server with this code. In this case we can consider a simplified approach: the thing which interests us the most is if the server sends an original message or the shimmed version. To test that we can run a client and set code version in the condition on the keyserver. If the client's version is higher than server's, we should receive an original message. Otherwise, it should be the shimmed one.

lib/shared/messages/edit-message-spec.js
121–123 ↗(On Diff #23707)

We usually put some high code version, because we're not sure in which version the support for new message type will be included. E.g. 1000 might work ok.

Changed minCodeVersion for messageShimming

In D6963#209536, @tomek wrote:

The proper way of testing older clients involves running an older client in prod mode (so that it won't get hot reloaded when we update the code version), then checking out the newest code, and running the server with this code. In this case we can consider a simplified approach: the thing which interests us the most is if the server sends an original message or the shimmed version. To test that we can run a client and set code version in the condition on the keyserver. If the client's version is higher than server's, we should receive an original message. Otherwise, it should be the shimmed one.

Thanks for the help! Here is how it looks after changing the version to a higher one:

Screenshot 2023-03-14 at 11.01.43.png (846×740 px, 160 KB)

However, I'm not sure if this is what the user would like to see, because in reality that robotexts don't contain any information, but they can take up a lot of space.

lib/shared/messages/edit-message-spec.js
77 ↗(On Diff #23712)

In D7086, I add the message type to this error message for the other MessageSpecs. Can you add it here too?

Nice thanks for addressing my comments and adding shimUnsupportedMessageInfo and unshimMessageInfo . Just make sure before you land that you address @ashoat's comment above and please make note somewhere or create a linear task to set the correct code version when we launch this feature

This revision is now accepted and ready to land.Mar 16 2023, 5:14 PM
kuba marked an inline comment as done.

Added message type for error message

lib/shared/messages/edit-message-spec.js
121–123 ↗(On Diff #23980)

Change minCodeVersion before landing

kuba marked an inline comment as not done.Mar 22 2023, 5:09 AM