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(sync)_: Improve EnableInstallationAndSync to include installation in MessengerResponse #5869

Closed

Conversation

qfrank
Copy link
Contributor

@qfrank qfrank commented Sep 24, 2024

Major changes:

  • Refactor EnableInstallationAndSync for better error handling and response merging
  • Add new EnableInstallationV2 method returning the installation
  • Update tests to check for installation in response
  • Deprecate old EnableInstallation method

fix relate comment

status: ready

@qfrank qfrank self-assigned this Sep 24, 2024
@qfrank qfrank requested a review from ilmotta September 24, 2024 08:18
@status-im-auto
Copy link
Member

status-im-auto commented Sep 24, 2024

Jenkins Builds

Click to see older builds (5)
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ 5e7284f #1 2024-09-24 08:20:26 ~2 min tests-rpc 📄log
✔️ 5e7284f #1 2024-09-24 08:22:41 ~4 min linux 📦zip
✔️ 5e7284f #1 2024-09-24 08:22:47 ~4 min ios 📦zip
✔️ 5e7284f #1 2024-09-24 08:23:22 ~5 min android 📦aar
✔️ 5e7284f #1 2024-09-24 08:51:13 ~33 min tests 📄log
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ abfa2eb #2 2024-09-26 23:04:49 ~2 min android 📦aar
✔️ abfa2eb #2 2024-09-26 23:05:03 ~2 min linux 📦zip
✔️ abfa2eb #2 2024-09-26 23:05:24 ~2 min tests-rpc 📄log
✔️ abfa2eb #2 2024-09-26 23:07:33 ~5 min ios 📦zip
✔️ abfa2eb #2 2024-09-26 23:35:03 ~32 min tests 📄log
✔️ 6640204 #3 2024-09-30 01:24:29 ~2 min android 📦aar
✔️ 6640204 #3 2024-09-30 01:24:47 ~2 min linux 📦zip
✔️ 6640204 #3 2024-09-30 01:25:00 ~2 min tests-rpc 📄log
✔️ 6640204 #3 2024-09-30 01:25:33 ~3 min ios 📦zip

Copy link
Member

@jrainville jrainville 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. As far as I can see, this doesn't modify the API so no work is needed on the client side right?

@qfrank qfrank requested a review from Samyoul September 24, 2024 14:12
@qfrank
Copy link
Contributor Author

qfrank commented Sep 24, 2024

Looks good. As far as I can see, this doesn't modify the API so no work is needed on the client side right?

not sure if desktop exists the same issue as mobile, if desktop does not need updated installation info within MessengerResponse returned from EnableInstallationAndSync, that's okay. @jrainville

@qfrank qfrank force-pushed the fix/sync_fallback_response_installation_update branch from 5e7284f to abfa2eb Compare September 26, 2024 23:02
@status-im-auto
Copy link
Member

✔️ status-go/prs/android/PR-5869#2 🔹 ~2 min 43 sec 🔹 abfa2eb 🔹 📦 android package

@codecov-commenter
Copy link

codecov-commenter commented Sep 26, 2024

Codecov Report

Attention: Patch coverage is 47.36842% with 10 lines in your changes missing coverage. Please review.

Please upload report for BASE (develop@38308d4). Learn more about missing BASE report.

✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
protocol/messenger_pairing_and_syncing.go 53.84% 3 Missing and 3 partials ⚠️
services/ext/api.go 0.00% 2 Missing ⚠️
services/ext/service.go 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             develop    #5869   +/-   ##
==========================================
  Coverage           ?   46.06%           
==========================================
  Files              ?      884           
  Lines              ?   157316           
  Branches           ?        0           
==========================================
  Hits               ?    72462           
  Misses             ?    76535           
  Partials           ?     8319           
Flag Coverage Δ
functional 11.60% <0.00%> (?)
unit 45.64% <47.36%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
protocol/messenger_sync_raw_messages.go 30.50% <100.00%> (ø)
protocol/messenger_testing_utils.go 60.37% <100.00%> (ø)
services/ext/api.go 6.27% <0.00%> (ø)
services/ext/service.go 20.39% <0.00%> (ø)
protocol/messenger_pairing_and_syncing.go 58.75% <53.84%> (ø)

…ionV2

- Refactor EnableInstallationAndSync for better error handling and response merging
- Add new EnableInstallationV2 method returning the installation
- Update tests to check for installation in response
- Deprecate old EnableInstallation method
@qfrank qfrank force-pushed the fix/sync_fallback_response_installation_update branch from abfa2eb to 6640204 Compare September 30, 2024 01:22
@qfrank
Copy link
Contributor Author

qfrank commented Sep 30, 2024

close this and use PR 5888 instead cc @Parveshdhull

@qfrank qfrank closed this Sep 30, 2024
@status-im-auto
Copy link
Member

✔️ status-go/prs/tests/PR-5869#3 🔹 ~31 min 🔹 6640204 🔹 📦 tests package

@igor-sirotin igor-sirotin deleted the fix/sync_fallback_response_installation_update branch October 1, 2024 13:28
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.

5 participants