Merge pull request #40 from beekpr/inbox/timestamp

As reported on the hive by Raffaele

```
I found a bug or undesired behavior with the Inbox module. So when we fetch the Inbox, we can order it by date right? Every Inbox response has this "delay" element with a timestamp.

However, it looks like when we perform any of the follwing actions, it resets that timestamp to the current time:
- Archiving/unarchiving
- Muting/unmuting
- Marking as read/unread

So that means that if, for example, I mute a conversation, then it gets bumped to the top of my inbox which is a bit strange.
```

The fix simply removes setting the timestamp, which I shouldn't have done to begin with, but for the query to be valid I need to do some simple comma-separator fixing.

big_tests/bkpr/inbox_extensions_SUITE.erl

@@ -68,7 +68,8 @@
          properties_can_be_get/1,
          properties_many_can_be_set/1,
          max_queries_can_be_limited/1,
-         max_queries_can_fetch_ahead/1
+         max_queries_can_fetch_ahead/1,
+         timestamp_is_not_reset_with_setting_properties/1
         ]).
 %% Groupchats
 -export([
@@ -131,7 +132,8 @@ bkpr_tests() ->
         properties_can_be_get,
         properties_many_can_be_set,
         max_queries_can_be_limited,
-        max_queries_can_fetch_ahead
+        max_queries_can_fetch_ahead,
+        timestamp_is_not_reset_with_setting_properties
       ]},
      {muclight, [sequence], [
         groupchat_setunread_stanza_sets_inbox
@@ -472,6 +474,21 @@ max_queries_can_fetch_ahead(Config) ->
                   #{limit => 2, 'end' => TimeAfterKate})
     end).
 
+timestamp_is_not_reset_with_setting_properties(Config) ->
+    escalus:fresh_story(Config, [{alice, 1}, {bob, 1}], fun(Alice, Bob) ->
+        % Alice sends a message to Bob
+        inbox_helper:send_msg(Alice, Bob),
+        %% We capture the timestamp
+        [Item1] = inbox_helper:get_inbox(Bob, #{count => 1}),
+        TStamp1 = inbox_helper:timestamp_from_item(Item1),
+        % Bob sets a bunch of properties
+        set_inbox_property(Bob, Alice, [{read, true}, {mute, 24*?HOUR}]),
+        % Bob gets the inbox again, and timestamp should be the same
+        [Item2] = inbox_helper:get_inbox(Bob, #{count => 1}),
+        TStamp2 = inbox_helper:timestamp_from_item(Item2),
+        ?assertEqual(TStamp1, TStamp2)
+  end).
+
 % muclight
 groupchat_setunread_stanza_sets_inbox(Config) ->
     escalus:story(Config, [{alice, 1}, {bob, 1}, {kate, 1}], fun(Alice, Bob, Kate) ->

src/bkpr/mod_inbox_bkpr.erl

@@ -96,19 +96,19 @@ extract_requests(Acc, IQ, From, EntryJID, Requests0) ->
 process_requests(Acc, IQ, From, EntryJID, CurrentTS, Requests) ->
     {LUser, LServer} = jid:to_lus(From),
     BinEntryJID = jid:to_binary(jid:to_lus(EntryJID)),
-    Query = build_sql_query(LUser, LServer, BinEntryJID, CurrentTS, Requests),
+    Query = build_sql_query(LUser, LServer, BinEntryJID, Requests),
     case execute_requests(LServer, Query) of
         {error, Msg} ->
             return_error(Acc, IQ, Msg);
         Result ->
             forward_request(Acc, IQ, From, BinEntryJID, Result, CurrentTS)
     end.
 
--spec build_sql_query(jid:luser(), jid:lserver(), binary(), integer(), iolist()) -> iolist().
-build_sql_query(LUser, LServer, BinEntryJID, CurrentTS, Requests) ->
+-spec build_sql_query(jid:luser(), jid:lserver(), binary(), iolist()) -> iolist().
+build_sql_query(LUser, LServer, BinEntryJID, Requests) ->
     ["UPDATE inbox ",
-     "SET ", Requests, "timestamp=", esc_int(CurrentTS),
-     "WHERE "
+     "SET ", Requests,
+     " WHERE "
          "luser=", esc_string(LUser), " AND "
          "lserver=", esc_string(LServer), " AND "
          "remote_bare_jid=", esc_string(BinEntryJID),
@@ -216,25 +216,25 @@ return_error(Acc, IQ, Msg) when is_binary(Msg) ->
 form_to_query(_, [], []) ->
     {error, <<"no-property">>};
 form_to_query(_, [], Acc) ->
-    Acc;
+    lists:droplast(Acc);
 form_to_query(TS, [#xmlel{name = <<"archive">>,
                           children = [#xmlcdata{content = <<"true">>}]} | Rest], Acc) ->
-    form_to_query(TS, Rest, ["archive=true," | Acc]);
+    form_to_query(TS, Rest, ["archive=true", "," | Acc]);
 form_to_query(TS, [#xmlel{name = <<"archive">>,
                           children = [#xmlcdata{content = <<"false">>}]} | Rest], Acc) ->
-    form_to_query(TS, Rest, ["archive=false," | Acc]);
+    form_to_query(TS, Rest, ["archive=false", "," | Acc]);
 form_to_query(TS, [#xmlel{name = <<"read">>,
                           children = [#xmlcdata{content = <<"true">>}]} | Rest], Acc) ->
-    form_to_query(TS, Rest, ["unread_count=0," | Acc]);
+    form_to_query(TS, Rest, ["unread_count=0", "," | Acc]);
 form_to_query(TS, [#xmlel{name = <<"read">>,
                           children = [#xmlcdata{content = <<"false">>}]} | Rest], Acc) ->
-    form_to_query(TS, Rest, ["unread_count = CASE unread_count WHEN 0 THEN 1 ELSE unread_count END," | Acc]);
+    form_to_query(TS, Rest, ["unread_count = CASE unread_count WHEN 0 THEN 1 ELSE unread_count END", "," | Acc]);
 form_to_query(TS, [#xmlel{name = <<"mute">>,
                           children = [#xmlcdata{content = Value}]} | Rest], Acc) ->
     case maybe_binary_to_positive_integer(Value) of
         {error, _} -> {error, <<"bad-request">>};
         0 ->
-            form_to_query(TS, Rest, ["muted_until=0," | Acc]);
+            form_to_query(TS, Rest, ["muted_until=0", "," | Acc]);
         N when N > 0 ->
             MutedUntilSec = erlang:convert_time_unit(TS, microsecond, second) + N,
             MutedUntilMicroSec = erlang:convert_time_unit(MutedUntilSec, second, microsecond),