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

sync filter params moved to matrix config #7843

Merged
merged 5 commits into from
Dec 28, 2022

Conversation

fedrunov
Copy link
Contributor

Type of change

  • Feature
  • Bugfix
  • Technical
  • Other :

Content

currently sync filter params are stored in session realm which can be cleared. This leads to incorrect filter being used (e.g. without lazyLoadEvents param).

We need to move params to somewhere else, so they won't be cleared

Motivation and context

Screenshots / GIFs

Tests

  • Step 1
  • Step 2
  • Step ...

Tested devices

  • Physical
  • Emulator
  • OS version(s):

Checklist

@fedrunov fedrunov requested a review from bmarty December 23, 2022 12:12
@ElementBot
Copy link

Fails
🚫

node failed.

Log

Error:  Error: Could not find the Dangerfile at ./tools/danger/dangerfile-lint.js - does a file exist at danger/dangerfile-lint.js in ./tools?.
    at /usr/src/danger/distribution/platforms/GitHub.js:166:27
    at step (/usr/src/danger/distribution/platforms/GitHub.js:44:23)
    at Object.next (/usr/src/danger/distribution/platforms/GitHub.js:25:53)
    at fulfilled (/usr/src/danger/distribution/platforms/GitHub.js:16:58)
    at processTicksAndRejections (internal/process/task_queues.js:95:5)
danger-results://tmp/danger-results-1a64d801.json

Generated by 🚫 dangerJS against bfa2844

@@ -72,7 +72,6 @@ import org.matrix.android.sdk.internal.database.model.threads.ThreadSummaryEntit
SpaceParentSummaryEntity::class,
UserPresenceEntity::class,
ThreadSummaryEntity::class,
SyncFilterParamsEntity::class,
Copy link
Member

Choose a reason for hiding this comment

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

Do we need a database migration because of this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've tested upgrading develop build with this branch and there were no problems

Copy link
Member

Choose a reason for hiding this comment

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

I added the migration to cleanup the database from the entity added in 45

Copy link
Member

@bmarty bmarty left a comment

Choose a reason for hiding this comment

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

LGTM, thanks, just a question about the database migration.
Also did you test to clear cache and see what was the sync filter applied to the sync request?

@fedrunov
Copy link
Contributor Author

Also did you test to clear cache and see what was the sync filter applied to the sync request?

Yes, I've tested enabling and disabling threads which clears session realm and also tested login with different account. All works fine

@BillCarsonFr
Copy link
Member

BillCarsonFr commented Dec 27, 2022

After a clear cache homeServerCapabilities will be null. That means that the initial sync will have a filter without useThreadNotifications. Is this expected?

Edit
Looks like it's detected and it stores the new filter. Not to familiar with that part of the code but seems to work

@ElementBot
Copy link

Warnings
⚠️ Please add a reviewer to your PR.

Generated by 🚫 dangerJS against 5583172

@ElementBot
Copy link

Warnings
⚠️ Please add a reviewer to your PR.

Generated by 🚫 dangerJS against 25f4f21

@sonarcloud
Copy link

sonarcloud bot commented Dec 28, 2022

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

44.0% 44.0% Coverage
0.0% 0.0% Duplication

@BillCarsonFr BillCarsonFr merged commit d75d7a3 into develop Dec 28, 2022
@BillCarsonFr BillCarsonFr deleted the bugfix/nfe/sync_filter_fix branch December 28, 2022 10:04
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.

4 participants