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 migration manager #657

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

pasnox
Copy link
Contributor

@pasnox pasnox commented Oct 6, 2024

PR check list:

  • Document your code
  • Add \since QXmpp 1.X, QXMPP_EXPORT
  • Update doc/doap.xml
  • Add unit tests
  • Format the code: Run clang-format -i src/<edited-file(s)> tests/<edited-file(s)>

Fixed a bunch of issues in the QXmppAccountMigrationManager:

  • Stabilized the unit tests moving from unordered to sorted keys nodes
  • Handle correctly joining mix channels

src/client/QXmppRosterManager.cpp Outdated Show resolved Hide resolved
src/client/QXmppRosterManager.cpp Outdated Show resolved Hide resolved
@pasnox pasnox force-pushed the fix/migration-manager branch 2 times, most recently from 9041fa3 to 9c7f0fd Compare October 12, 2024 16:46
tests/TestClient.h Outdated Show resolved Hide resolved
src/base/Algorithms.h Outdated Show resolved Hide resolved
@lnjX
Copy link
Member

lnjX commented Oct 21, 2024

QXmppMovedManager: Fix doxygen missing documentation

I fixed this on master now without using QXmppClient::EmptyResult.

The alias QXmppClient::EmptyResult should not be used in other classes as QXmppClient isn't really the right place for a global alias and I'd also like to avoid including QXmppClient.h in every other file.

@pasnox pasnox force-pushed the fix/migration-manager branch 4 times, most recently from 1e1ee0e to e69f349 Compare October 26, 2024 17:11
@lnjX
Copy link
Member

lnjX commented Oct 28, 2024

Util: Add more helpers

Can you change the commit description so that it properly describes the changes (not just "more utils"), e.g. tests: util: Add xmlToFormattedByteArray() func.

@lnjX
Copy link
Member

lnjX commented Oct 28, 2024

QXmppRosterManager: Handle better mix roster entries

Please include that this is only about data import/export in the commit description. And in the long description you could note what is actually fixed.

Copy link
Member

@lnjX lnjX left a comment

Choose a reason for hiding this comment

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

apart from last minor comments looks good :)

The original xmlToDom now use xmlToDomDoc under the wood.
Some tests are more complex and involve inner tasks in between calls
and the test client does not handle that great, leading to iq id clash
and random iq id generation which make tests unreliable.
This is especially visible when a test involve many expect/inject calls
like in the migration manager tests.
The QXmppExportData class store data in an unordered_map and would then
serialize each extensions in a random order, which break string based
unit tests.
We then serialize in the std::type_index sorting
order.
It can filter and convert items into a given output container.
Originally we only stored the roster as is in the QXmppRosterManager
without taking care of MIX channels. This change split the handling
of data, QXmppMixManager now export/import the MIX roster items taking
care to join again those channels later while QXmppRosterManager now
export/import non MIX roster items.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants