Page MenuHomePhabricator

[keyserver] Add migration to populate `target_message` for sidebar_source
ClosedPublic

Authored by inka on Apr 19 2023, 3:40 AM.
Tags
None
Referenced Files
F1865527: D7517.id.diff
Sun, May 26, 10:07 AM
F1863534: D7517.id.diff
Sun, May 26, 6:09 AM
F1863533: D7517.diff
Sun, May 26, 6:08 AM
Unknown Object (File)
Fri, May 24, 11:03 AM
Unknown Object (File)
Sun, May 19, 5:42 PM
Unknown Object (File)
Sun, May 19, 5:42 PM
Unknown Object (File)
Sun, May 19, 5:42 PM
Unknown Object (File)
Sun, May 19, 5:42 PM
Subscribers

Details

Summary

issue: https://linear.app/comm/issue/ENG-3708/add-a-migration-to-populate-target-message-for-sidebar-source
Adding a migration that sets the target_message column for sidebar_source messages to the id of the original message, that we store in the content JSON as well.

Test Plan

Run keyserver, checked that taget_message got set for messages with type 17.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

inka requested review of this revision.Apr 19 2023, 3:57 AM
ashoat requested changes to this revision.Apr 19 2023, 7:11 AM
ashoat added inline comments.
keyserver/src/database/migration-config.js
332–334 ↗(On Diff #25331)

Can you please fix up this query, following the feedback I've given you in the past? If you need further pointers, let me know and I can share more explicit feedback

This revision now requires changes to proceed.Apr 19 2023, 7:11 AM
keyserver/src/database/migration-config.js
332–334 ↗(On Diff #25331)

I'm not sure about indentation for the SET word, since we seem to use two approaches in the codebase (example1, example2). I followed the same convention as was already in this file, but I can see that the second one is more common. So probably the indentation of the SET in this file was a mistake.
Should I change them both to have the same indentation as the UPDATE? (if so can I change both of them in this diff?)

Update query to have spaces around "="

keyserver/src/database/migration-config.js
332–334 ↗(On Diff #25331)

Other than that, and the spaces around "=" I think the formatting is fine. If this is incorrect please let me know.

ashoat requested changes to this revision.Apr 20 2023, 11:40 AM
ashoat added inline comments.
keyserver/src/database/migration-config.js
334 ↗(On Diff #25454)

You should never have 17 appear like this. This is feedback that has been shared before – please integrate it into your approach going forward, and make sure you always consider it before submitting a diff

332–334 ↗(On Diff #25331)

I'm not sure about indentation for the SET word, since we seem to use two approaches in the codebase (example1, example2). I followed the same convention as was already in this file, but I can see that the second one is more common. So probably the indentation of the SET in this file was a mistake.
Should I change them both to have the same indentation as the UPDATE? (if so can I change both of them in this diff?)

Yes, that would be great!

This revision now requires changes to proceed.Apr 20 2023, 11:40 AM
This revision is now accepted and ready to land.Apr 21 2023, 6:38 AM