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

Remove obsolete XEP-0091 timestamp #383

Merged
merged 10 commits into from
Mar 16, 2015
Merged

Remove obsolete XEP-0091 timestamp #383

merged 10 commits into from
Mar 16, 2015

Conversation

gmodarelli
Copy link

Hi guys, at my company we are developing a mobile chat based on MongooseIM. For our android client we are using the ASmack library and we get an date parsing error when the client receives an offline message.

The offending date is the one passed inside the x tag of the XEP-0091 extension. Since I noticed it has obsoleted, I thought I could have removed it from MongooseIM.

But I'm a Ruby developer and know just the very basics of Erlang. I've managed to remove the tag from the message stanza, recompiled the repo and the server works. No date parsing error on the client anymore.

But I don't know if I need to add some tests around my changes or if the changes are correct (ie. do I need to remove/add something else?). I'd love to contribute to the project and I'd like to hear your feedback.

Bye

@michalwski
Copy link
Contributor

I suggest removing the function (jlib:timestamp_to_xml/1) generating obsolete timestamp also.

@gmodarelli
Copy link
Author

Thanks @michalwski

@@ -54,7 +54,6 @@
parse_xdata_submit/1,
timestamp_to_iso/1, % TODO: Remove once XEP-0091 is Obsolete
Copy link
Contributor

Choose a reason for hiding this comment

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

Just noticed that there is another obsolete function. I'd remove it also and see if our tests run by travis passes.

Copy link
Author

Choose a reason for hiding this comment

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

@michalwski I removed this one too.

@michalwski
Copy link
Contributor

Regarding tests there is offline_SUITE so you can modify the only test case there to ensure that the legacy timestamp is not present.
If you fork ejabberd_tests and create branch of the same name as the branch from which this PR was sent then travis-ci will automatically use your branch for tests.

@michalwski
Copy link
Contributor

And what's most important thanks for your contribution @gmodarelli 👍

@gmodarelli
Copy link
Author

Ok @michalwski I'll try to write the tests and I'll remove the other obsolete method. I'm glad to contribute 👍

@@ -2955,10 +2953,8 @@ add_timestamp({_,_,Micro} = TimeStamp, Server, Packet) ->
Time = {D,{H,M,S, Micro}},
case xml:get_subtag(Packet, <<"delay">>) of
false ->
%% TODO: Delete the next element once XEP-0091 is Obsolete
TimeStampLegacyXML = timestamp_legacy_xml(Server, Time),
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that somebody confused the binding/function name and named the modern format as legacy. It's a good opportunity to fix it :)

Copy link
Author

Choose a reason for hiding this comment

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

@studzien I hope the new name is ok. Thanks for the feedback

@gmodarelli
Copy link
Author

@michalwski I'm still not able to write tests for the project, sorry. I need to study Erlang more :) But I will!

@michalwski
Copy link
Contributor

Looks like some of the obsolete functions where used in other place(s).

Take a look at tests output:
https://travis-ci.org/esl/MongooseIM/jobs/52133477#L1543
and here's more details about the error:
https://travis-ci.org/esl/MongooseIM/jobs/52133477#L1750

This happened for all jobs except one which doesn't run this test.

@michalwski
Copy link
Contributor

I didn't meant to restore the obsolete function :) The best approach would be to fix the place where the function is used.

@gmodarelli
Copy link
Author

@michalwski ok :) I'll try to replace it.

@gmodarelli
Copy link
Author

@michalwski sorry if this is taking me so long. I had doubts on how to proceed. The obsolete function (timestamp_to_iso/1) is only used by the build_random_password/1 function in the mod_admin_extra_accounts module.

So I replaced the call to the function and generated the date in the build_random_password itself.

Please let me know if this is the right way to proceed. Thank you!

@michalwski
Copy link
Contributor

I think you can put there date in other format not necessary the obsolete one. This function is generating random password for banned user so in fact we can put any password there, even without date.

@gmodarelli
Copy link
Author

Ok cool, so we could remove the date. I'm just wondering if we have restriction on the format of the random password. Can we just remove the date part or we need to replace it with something else?

Thank you.

@michalwski
Copy link
Contributor

I don't know about any restriction. This format was inherited from ejabberd. The only reason to setup such password is to prevent banned user from authentication. The format just brings the server administrator info that user with such password was banned, when and from what reason.

@gmodarelli
Copy link
Author

Ok, I'll add the date back (with this format 1997-07-16T19:20:30.45+01:00) so we won't break code depending on it, even if it is just for logging or debugging purpose.

@gmodarelli
Copy link
Author

I was not able to add the timezone offset, so I opted for the universal time.

@michalwski
Copy link
Contributor

Ok, UTC is enough for that.

michalwski added a commit that referenced this pull request Mar 16, 2015
@michalwski michalwski merged commit bda8185 into esl:master Mar 16, 2015
@michalwski
Copy link
Contributor

Thanks for the fix!

@gmodarelli
Copy link
Author

Thank you for helping me with it!

@gmodarelli gmodarelli deleted the stop-sending-obsolete-XEP-0091 branch March 16, 2015 13:26
@michalwski michalwski mentioned this pull request Oct 15, 2015
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