Details

Reviewers

Commits

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

No Lint Coverage

Unit

No Test Coverage

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		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!