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 email and self_entity changing ids server-side. And initial conv list. #104

Merged
merged 3 commits into from
Oct 26, 2018

Conversation

HomerSp
Copy link
Contributor

@HomerSp HomerSp commented Oct 24, 2018

As the title says, this fixes the ids of email and self_entity, as well as fixing conversation states no longer being sent in the init message by running sync after initializing.
Wasn't sure what to do about conv_states being initialized to ds:20 still, so I left it as it doesn't seem to do any harm.

This fixes #103

Fix conversation states no longer being sent in the init message by running sync after initializing.
test/body.html Outdated
]
]
}});</script><script>AF_initDataCallback({key: 'ds:32', isError: false , hash: '29', data:function(){return [["cib:sc",3600,3600,3,600,10]
}});</script>><script>AF_initDataCallback({key: 'ds:32', isError: false , hash: '29', data:function(){return [["cib:sc",3600,3600,3,600,10]

Choose a reason for hiding this comment

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

Looks like an extra <

@@ -58,8 +58,7 @@
]
}});</script><script>AF_initDataCallback({key: 'ds:30', isError: false , hash: '27', data:function(){return [["cin:acc","https://myphonenumbers-pa.clients6.google.com"]
]
}});</script><script>AF_initDataCallback({key: 'ds:31', isError: false , hash: '28', data:function(){return [["cic:sc",["AF","DZ","AO","BH","BD","BJ","BF","KH","CM","CG","CD","CI","GH","GU","GN","ID","IQ","IL","JO","KZ","KE","KW","KG","LA","LR","MG","MW","MY","MV","MA","MZ","NE","NG","OM","PK","PS","PH","SA","SN","SC","SL","SO","ZA","LK","TZ","TH","TG","TN","TR","UG","UA","UZ","VN","ZM"]
]
}});</script><script>AF_initDataCallback({key: 'ds:31', isError: false , hash: '37', data:function(){return [["cic:vd","104889609452098951178","[email protected]",0,""]
Copy link

Choose a reason for hiding this comment

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

What is the significance of 104889609452098951178 and [email protected] here?

Choose a reason for hiding this comment

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

I edited the patch and left the original text on the line and am able to connect.

Copy link

@plessbd plessbd Oct 25, 2018

Choose a reason for hiding this comment

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

@dmidthun that is because this wont effect the running of the program, just the tests.

@smammy I assumed it was so that test would pass unlike #102 which fails https://travis-ci.org/yakyak/hangupsjs/builds/445768280

Copy link

Choose a reason for hiding this comment

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

OK, got it. Thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since the id of email was changed it had to be changed in body.html as well for it to pass, so I just copied over the data from the old id to the new one.

Copy link
Member

Choose a reason for hiding this comment

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

The email is from a fake account that I used to generate the tests manually.

The tests need to be greatly improved as they do close to nothing.

@cgatesman
Copy link

Please do not comment on pull requests if you don't know what you are doing or how to apply and test it properly.

@smammy
Copy link

smammy commented Oct 25, 2018

@psouza4 @edraven this is not the place to ask for tech support. Please refer to yakyak/yakyak#981 (comment)

@nicholashudson2
Copy link

I was able to apply the changes in the three commits associated with this pull request, compile, and run without any issues. Connection was established quickly, message history populated as expected, and no error messages were encountered. Have been testing off and on throughout the day today, and so far, it's been smooth sailing. @HomerSp Good work! Problem appears to be resolved, tests are passing consistently, and I've yet to encounter any errors/exceptions.

@Mange
Copy link

Mange commented Oct 26, 2018

I manually modified the compiled code with what I think the resulting JS would look like, and I can also confirm that this works for me. Thank you!

(It's been some time since I wrote CoffeeScript, but I could also use the code around it to remind me of how the JS should look like.)

@blondehouse
Copy link

Hi all,
I'm brand new to coding and understanding patches. Can someone walk me through on how I can fix this same issue i've been having? I'm running 1.5.2 on Mojave.
Thanks!

@averissimo averissimo merged commit 2bc6c76 into yakyak:master Oct 26, 2018
@smammy
Copy link

smammy commented Oct 26, 2018

@blondehouse this fix is in a subcomponent and not a straightforward patch so you're probably better off waiting for the YakYak release (which looks like it's in progress).

@averissimo
Copy link
Member

@smammy exactly!

@blondehouse we're doing the necessary steps to release a new yakyak version in the next hour or so

@blondehouse
Copy link

@smammy @averissimo Thank you guys!!

@averissimo
Copy link
Member

@HomerSp @smammy @akholodenko thanks for the patch and peer review of code. We appreciate so much.

We'd welcome more patches and new blood in hangupsjs ;)

@HomerSp
Copy link
Contributor Author

HomerSp commented Oct 27, 2018

You're very welcome - glad I could help! If I do any more work on it I will certainly send in more pull requests.

@HomerSp HomerSp deleted the work branch November 1, 2018 09:30
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.

broken again?
10 participants