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] #1121 [S] New contact will be added through invite command if necessary #1140

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

Conversation

FelixSelter
Copy link

@FelixSelter FelixSelter commented Apr 4, 2021

Your checklist for this pull request

Please, check that:

  • all commits comply with our commit message guidelines
  • your code complies with the google java format (see here for details)
  • all unit tests are still successful

Description

In this Pull request issue, #1121 is fixed.
If someone is invited by the invite command of the Server it will check if this person is an actual contact and if not add him as one. Note the invited person still has to accept the contactInvite. After he accepted the server admin has to reinvite him so he will get the session invite request

Also its my first time contributing so I hope everything is fine. Else tell me. I'm a human and got the ability learn

Thank you for the awesome work and to the reviewers!

@srossbach
Copy link
Contributor

I still have issues figuring out what is going wrong ? Neither the commit description or the the description of the issue mention what is going wrong. And before try to respond I do not accept the answer: You have to add the contact. I want to know what part of Saros is not working as expected and WHY.

@srossbach
Copy link
Contributor

Ok after digging through the code, Stefan changed how Saros support is determined. This now requires presence information which you will not obtain as long as there is no subscription status. So the minimum requirement is subscription='to': .

This does not fix anything unless the user somehow (depending on Saros/E or Saros/I) accepts the invitation. To be more specific, the user has to accept the invitation ASAP which it simply cannot.

@FelixSelter
Copy link
Author

FelixSelter commented Apr 5, 2021

As far as I understood the issue is that, users are confused if they try to invite someone but it's not working because they are not on the contact list of the server. My approach to fixing this was to send a contact request in this case. It would make more sense if I would print the warning and then return it so they won't be covered by the error traces. The user then has time to accept and the admin will reinvite him. I also suggested adding this contact invitation as an external command but this won't fix the issue.

To my understanding, the issue is fixed because the admin gets notified and knows that the user has to accept the contact invitation. May @Drakulix can take a look because he knows the most about the issue due to the title change etc

I will try to take a look into this subscription system but as I started I'm not really familiar with saros. As far as I understood there's only a subscription if he's already a contact.

Any other suggestions? What would fix the issue in your opinion? Is it possible to override the rights of the other user so that there's no need to accept the contact invite (I don't think that should be done but seems to be the only other solution)?

@FelixSelter
Copy link
Author

Example session till I fix the issue with the subscription:

# Welcome to Saros Server (type 'help' for available commands)
> invite [email protected]
Adding [email protected] as new contact. You will have to reinvite him after he accepted being added to the contacts
10:47:17.751 [main] ERROR saros.server.console.InviteCommand - Unknown ErrorAn unknown error has occured:

subscription-required(407) Not subscribed

It will be continued anyway
The contact was added successfully. Please invite him again after he accepted
> invite [email protected]
> [-] Starting session negotiation...
[\] Checking Saros support...
[|] Checking version compatibility...
[/] Sending negotiation request...
[-] Waiting for negotiation acknowledgement...
[\] Waiting for [email protected] to accept invitation...
[|] Waiting for remote session configuration data...
[/] Sending final session configuration data...
[-] Waiting for [email protected] to connect...
[\] Establishing connection and performing final initialization...
[|] Synchronizing user list...

for (JID jid : jids.get(true)) {
sessionManager.startSharingReferencePoints(jid);

// --------------add contact if not already--------------
Copy link
Contributor

Choose a reason for hiding this comment

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

The comments are pretty pointless unless you are not able to read code at all.

out.println(
"Adding "
+ jid
+ " as new contact. You will have to reinvite him after he accepted being added to the contacts"); // the previous warning wont be displayed in the console
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you really want to show these messages in the log file ? It is an interactive command at all.

+ jid
+ " as new contact. You will have to reinvite him after he accepted being added to the contacts"); // the previous warning wont be displayed in the console

final BiPredicate<String, String> questionDialogHandler = // error handler
Copy link
Contributor

Choose a reason for hiding this comment

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

It is an automated process, either you are query the STDIN or simply return true, As a side node, this will executed mostly all the time because of privacy reasons, i.e the server will not tell you if such a JID exists.

"The contact was added successfully. Please invite him again after he accepted");
return; // exit because the user cant accept the invite in time
} catch (OperationCanceledException e) {
log.error(e);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this your intention to continue with the code on line 102 ? I do not think so.

}
// --------------end add contact if not already--------------

sessionManager.invite(
Copy link
Contributor

Choose a reason for hiding this comment

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

If you do not see the contact online (I do not know if XMPP supports invisibility, but that is another issue) then it makes no sense to continue. The invitation will simply fail.

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.

2 participants