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(groups): Fix search crashes #1736

Merged
merged 7 commits into from
Jan 23, 2024
Merged

fix(groups): Fix search crashes #1736

merged 7 commits into from
Jan 23, 2024

Conversation

Flemmli97
Copy link
Collaborator

What this PR does 📖

  • Fixes crash when searching for groups if a group has chars that are bigger than a byte (usually non latin chars)

Which issue(s) this PR fixes 🔨

@github-actions github-actions bot added Don't merge yet DO NOT MERGE Missing dev review Still needs to be reviewed by a dev Failed Automated Test This PR is failing Luis's Appium test and needs revised labels Jan 19, 2024
Copy link
Contributor

github-actions bot commented Jan 19, 2024

UI Automated Test Results Summary for MacOS/Windows

490 tests   438 ✅  2h 39m 34s ⏱️
 42 suites   52 💤
  3 files      0 ❌

Results for commit 9ec1792.

♻️ This comment has been updated with latest results.

Copy link
Contributor

github-actions bot commented Jan 19, 2024

UI Automated Tests execution is complete! You can find the test results report here

@luisecm
Copy link
Contributor

luisecm commented Jan 19, 2024

The following test is failing:
If you have a group named "Test" that has the user "ChatUserB" (who is a friend also) and then you look for the string "Ch", its only shows the user itself and not the group that has the user as results

image

@Flemmli97
Copy link
Collaborator Author

The following test is failing: If you have a group named "Test" that has the user "ChatUserB" (who is a friend also) and then you look for the string "Ch", its only shows the user itself and not the group that has the user as results

image

hmm strange. its working for me. can you show the test group?
Screenshot 2024-01-19 at 20 04 56

Copy link
Member

@stavares843 stavares843 left a comment

Choose a reason for hiding this comment

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

it crashes using korean chars

안녕하세요

STR:

  • go to chat
  • create group with name 안녕하세요
  • search for it
  • crash

crash log from macOS
https://gist.github.com/stavares843/df879ebc26b2c1995008f38a42c23356

crash log from uplink
19-01-2024_19-27-35.log

video

Gravacao.do.ecra.2024-01-19.as.19.37.27.mov

@stavares843
Copy link
Member

i also noticed that searching using greek or cyrillic does not perform the search

STR:

  • go to chat
  • add a group with either of these names
    Ё Ж З И Й К
    Η Θ Ι Κ Λ Μ
  • search for the group
  • no results

unsure if works on dev because trying the same on dev crashes

Cyrillic Script:
Ё Ж З И Й К

Greek Script:
Η Θ Ι Κ Λ Μ

macOS, m1

@stavares843 stavares843 added the Changes requested When other dev or QA request a change label Jan 19, 2024
@Flemmli97
Copy link
Collaborator Author

it crashes using korean chars

안녕하세요

STR:

  • go to chat
  • create group with name 안녕하세요
  • search for it
  • crash

crash log from macOS https://gist.github.com/stavares843/df879ebc26b2c1995008f38a42c23356

crash log from uplink 19-01-2024_19-27-35.log

video

Gravacao.do.ecra.2024-01-19.as.19.37.27.mov

i am unable to reproduce it. are you sure its on this branch?
as that byte error should be the dev branch which this pr should fix

@Flemmli97
Copy link
Collaborator Author

i also noticed that searching using greek or cyrillic does not perform the search

STR:

  • go to chat
  • add a group with either of these names
    Ё Ж З И Й К
    Η Θ Ι Κ Λ Μ
  • search for the group
  • no results

unsure if works on dev because trying the same on dev crashes

Cyrillic Script: Ё Ж З И Й К

Greek Script: Η Θ Ι Κ Λ Μ

macOS, m1

was a case sensitiv issue. fixed now

@Flemmli97 Flemmli97 removed the Changes requested When other dev or QA request a change label Jan 22, 2024
@github-actions github-actions bot removed the Failed Automated Test This PR is failing Luis's Appium test and needs revised label Jan 22, 2024
@luisecm
Copy link
Contributor

luisecm commented Jan 22, 2024

The following test is failing: If you have a group named "Test" that has the user "ChatUserB" (who is a friend also) and then you look for the string "Ch", its only shows the user itself and not the group that has the user as results
image

hmm strange. its working for me. can you show the test group? Screenshot 2024-01-19 at 20 04 56

Strange, I see that the test is passing now on last execution, I tested it manually on friday and it worked correctly. I'll keep an eye on the test results for next runs before it's merged and let you know any other findings.

@phillsatellite phillsatellite added the QA Tested QA has tested and approved label Jan 22, 2024
@stavares843 stavares843 removed QA Tested QA has tested and approved Missing dev review Still needs to be reviewed by a dev labels Jan 23, 2024
@stavares843 stavares843 added Waiting for CI Waiting for at least one CI job to complete before merging and removed Don't merge yet DO NOT MERGE labels Jan 23, 2024
@stavares843 stavares843 merged commit c521da4 into dev Jan 23, 2024
7 checks passed
@stavares843 stavares843 deleted the search_fix branch January 23, 2024 20:08
@github-actions github-actions bot removed the Waiting for CI Waiting for at least one CI job to complete before merging label Jan 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug(groups): group chat search crash
5 participants