Skip to content
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

Fix 1:1 message to/from in search results #5161

Merged
merged 5 commits into from
May 13, 2021

Conversation

hiqua
Copy link
Contributor

@hiqua hiqua commented Apr 8, 2021

Fixes #5158.

Contributor checklist:

Description

See issue.

I don't think that's the proper way to address this issue, but it seems to work in practice. Feel free to close this PR in favor of a better fix.

If I note:

M(e)
G(roup)
O(ther)

I have tested these cases and the header seems correct (message sent from - to):
O - G
O - M
M - G
M - O
M - M (notes to self)

@scottnonnenberg-signal scottnonnenberg-signal changed the title Fix enveloppe header in search results Fix 1:1 message to/from in search results Apr 8, 2021
@scottnonnenberg-signal
Copy link
Contributor

@hiqua Can you show some screenshots of the before and after? The change looks pretty simple, I just want to be sure that we get the proper 'Jon to You' kind of text.

@hiqua
Copy link
Contributor Author

hiqua commented Apr 9, 2021

These would be the screenshots covering various cases, including the faulty one. Let me know if there's something missing.

Ideally there'd be tests for this, I don't know if there are some in place already. I'll have a look and see how feasible that would be.

Before:
2021-04-09-095923_898x913_scrot
2021-04-09-095946_888x918_scrot
2021-04-09-095956_890x925_scrot
2021-04-09-100051_877x926_scrot
2021-04-09-100254_866x923_scrot

After:
2021-04-09-095333_901x918_scrot
2021-04-09-095357_897x925_scrot
2021-04-09-095417_897x919_scrot
2021-04-09-095437_894x924_scrot
2021-04-09-100338_869x914_scrot

Copy link
Contributor

@EvanHahn-Signal EvanHahn-Signal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Thanks for doing this.

@hiqua
Copy link
Contributor Author

hiqua commented Apr 9, 2021

I think the test "returns incoming message" could be modified to test this change, but it's not clear to me how exactly. If you have a hint I can easily follow I could try to implement it. Otherwise I'll leave it like this.

@EvanHahn-Signal
Copy link
Contributor

I think the test "returns incoming message" could be modified to test this change, but it's not clear to me how exactly. If you have a hint I can easily follow I could try to implement it. Otherwise I'll leave it like this.

That's a good point. I think it's worth adding a new test that's very similar to the "returns incoming message" test, with two changes:

  1. fromId and toId should be the same
  2. The expected result should be updated

Let me know if I can answer more questions.

@hiqua
Copy link
Contributor Author

hiqua commented Apr 9, 2021

Thanks for your help! I think I managed to come up with something reasonable, let me know if something is still missing.

The test does fail without the changes.

I guess you'll squash the commits, so I was not very descriptive with the second one.

Copy link
Contributor

@EvanHahn-Signal EvanHahn-Signal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, other than some small comments in tests.

ts/test-both/state/selectors/search_test.ts Outdated Show resolved Hide resolved
ts/test-both/state/selectors/search_test.ts Outdated Show resolved Hide resolved
hiqua and others added 3 commits April 15, 2021 21:26
Co-authored-by: Evan Hahn <69474926+EvanHahn-Signal@users.noreply.github.com>
Co-authored-by: Evan Hahn <69474926+EvanHahn-Signal@users.noreply.github.com>
@hiqua
Copy link
Contributor Author

hiqua commented Apr 29, 2021

@EvanHahn-Signal let me know if I should try to rebase.

@scottnonnenberg-signal
Copy link
Contributor

@hiqua No, please don't make any further changes. We've merged this internally, and when we release a beta including these changes, this PR should be automatically set as 'merged.'

@josh-signal josh-signal merged commit 37ff4a1 into signalapp:development May 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

Messages appear from sender to sender on search results
4 participants