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

[INTERNAL][CORE] Removes deprecated methods from XMPPAccountStore #615

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

Conversation

srossbach
Copy link
Contributor

[INTERNAL][CORE] Removes deprecated methods from XMPPAccountStore

This patch also fixes an issue in the getAccountsMethods so they lookup
the account correctly by ignoring the case sensitivity of the username.

Adds a FIXME to the XMPPAccount class regarding RFC 6122

Introduces get/set defaultAccount methods which should be a replacement
for set/get activeAccount in the future. 

They allow null values making the call to isEmpty obsolete. And now it
is also possible to completely remove all account data.

Add getAccount methods as there was no way to retrieve specific account
unless you called getAllAccounts and perform the search by yourself.
This patch introduces the usage of the new XMPPStore API.

Renaming UI buttons is not done yet as changing UI values is likely to
crash the whole STF.
This patch also fixes an issue in the getAccountsMethods so they lookup
the account correctly by ignoring the case sensitivity of the username.

Adds a FIXME to the XMPPAccount class regarding RFC 6122
@srossbach srossbach requested review from m273d15 and tobous August 2, 2019 22:13
@srossbach srossbach force-pushed the pr/use_new_accountstore_api branch 2 times, most recently from 362e2b6 to e4ba631 Compare August 5, 2019 18:55
@srossbach
Copy link
Contributor Author

I guess @m273d15 have to review this first as this patch may cause issues in the HTML UI.

@@ -51,6 +51,9 @@
if (!domain.toLowerCase().equals(domain))
throw new IllegalArgumentException("domain url must be in lower case letters");

// FIXME see https://tools.ietf.org/html/rfc6122#section-2 - the localpart/username is also
// case-insensitive
Copy link
Member

Choose a reason for hiding this comment

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

My assumption is that this means we should add a check (equivalent to the checks for server and domain above). Either way, I think it would be better to spell out exactly what you want to happen to ensure that the fixme is interpreted correctly.

Copy link
Contributor Author

@srossbach srossbach Aug 5, 2019

Choose a reason for hiding this comment

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

The problem I have is I do not know how XStream works. What happens if we load the set after changing the equal method of this class ? Can I ensure that duplicates got removed or do I have to manually "fix" the set ?

Copy link
Member

@tobous tobous left a comment

Choose a reason for hiding this comment

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

Wupsi, missed the travis results. As you already mentioned in another PR, the Saros server stuff needs updating as well.

@srossbach srossbach changed the base branch from pr/use_new_accountstore_api to master August 5, 2019 20:24
@stefaus
Copy link
Contributor

stefaus commented Jul 11, 2022

@srossbach would you like to continue working on this pr or should we close it?

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.

3 participants