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

[markers] Prepare a documentation page for the smart markers #3535

Merged
merged 1 commit into from
Mar 14, 2022

Conversation

NelsonVides
Copy link
Collaborator

These documents explain the functionality that mod_smart_markers offer alone, together with an iq-protocol not currently implemented, but that I'd like to start reviewing in order to make the implementation cleaner.

Note that for the iq-protocol, I didn't want to use Forms like we did for inbox, maybe because I imagine Forms as being actual forms presented to the user that the user fills on its own, while this request is more programmatically filled, but also, because I find forms much harder to implement, parse, validate, and test, and considering this module will already need all the removal hooks (remove_domain, remove_user, forget_room, affiliations_changed...), I didn't want to introduce more complexity to it.

@NelsonVides NelsonVides changed the base branch from master to feature/smart_markers February 7, 2022 16:01
@mongoose-im

This comment was marked as outdated.

@codecov
Copy link

codecov bot commented Feb 7, 2022

Codecov Report

Merging #3535 (8518373) into feature/smart_markers (ef8b27c) will not change coverage.
The diff coverage is n/a.

❗ Current head 8518373 differs from pull request most recent head c3b310f. Consider uploading reports for the commit c3b310f to get more accurate results

Impacted file tree graph

@@                  Coverage Diff                   @@
##           feature/smart_markers    #3535   +/-   ##
======================================================
  Coverage                  80.83%   80.83%           
======================================================
  Files                        425      425           
  Lines                      32273    32273           
======================================================
  Hits                       26087    26087           
  Misses                      6186     6186           
Impacted Files Coverage Δ
src/cassandra/mongoose_cassandra.erl 77.77% <0.00%> (-3.71%) ⬇️
src/mam/mod_mam_muc_rdbms_arch_async.erl 82.85% <0.00%> (-2.86%) ⬇️
src/cassandra/mongoose_cassandra_worker.erl 65.72% <0.00%> (-2.82%) ⬇️
src/rdbms/mongoose_rdbms.erl 62.54% <0.00%> (-1.10%) ⬇️
src/mod_bosh_socket.erl 78.26% <0.00%> (-0.32%) ⬇️
src/mod_muc_room.erl 77.26% <0.00%> (+0.17%) ⬆️
src/offline/mod_offline.erl 77.17% <0.00%> (+1.08%) ⬆️
src/logger/mongoose_log_filter.erl 79.45% <0.00%> (+1.36%) ⬆️
src/mam/mod_mam_elasticsearch_arch.erl 86.84% <0.00%> (+1.75%) ⬆️
...bal_distrib/mod_global_distrib_hosts_refresher.erl 77.35% <0.00%> (+5.66%) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ef8b27c...c3b310f. Read the comment docs.


* `<peer-bare-jid>` MUST be the bare jid of the peer whose last marker wants to be checked.
* `<thread>` is an optional attribute that indicates if the check refers to specific a thread in the conversation. If not provided, defaults to the main conversation thread.
* `<after>` is an optional attribute indicating whether markers sent only after a certain timestamp are desired.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add some note that this field makes sense for group chats only.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Wait, why only for groupchats? I might want to know which one was the last marker I sent to a regular user, just as much as to a room as well, and filtering by thread or timestamp applies the same 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

just add one actual example for each possible destination. I.e. for groupchat and for one2one. Including a date as a date ;)

Something like:

# Query for markers in a room

Request
Result

# Query for markers in one2one conversation

Request
Result

Copy link
Collaborator

Choose a reason for hiding this comment

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

It can be a generic implementation, but it brings the most value for group chats. Since there will be not more than just 2 chat markers for 1-2-1 chats.

```
where:

* `<peer-bare-jid>` MUST be the bare jid of the peer whose last marker wants to be checked.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please state explicitly that it can be either user or muc/muclight jid

@@ -0,0 +1,39 @@
When a client enters a conversation after being offline for a while, such client might want to know what was the last message-id that was marked according to the rules defined in [XEP-0333 - Chat Markers][chat-markers], in order to know where he left of, and build an enhanced UI.
Copy link
Collaborator

Choose a reason for hiding this comment

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

user might want to know where other participants left off as well, that was the main intention of this module.

also please fix typo - left off

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

where other participants left off is not what beekeeper needs, so I'm not implementing that part, yet. Would probably require a different IQ, and very likely make it configurable, because some deployments might consider where other users are as private data to those other users only and they shouldn't be fetched otherwise.

Might add a note to state that such functionality will be implemented as well if that makes the doc better 🤔

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure that we want it in open source, if it's too BKPR specific.

The main idea was to let the user fetch all the latest chat markers (his own and other participants as well) for a conversation (1-2-1 or a group chat), and I believe that is the protocol that we need in open source.

if you wish to have a common implementation, you may want to add an optional field [from=<jid>] to cover BKPR's use case.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Putting here a short summary of an offline discussion with @NelsonVides:

  • IQ protocol can stay as is.
  • a new configuration option for smart markers module can be introduced to cover beekeeper's use case:
    • the behaviour must be consistent for the offline and the online use cases. If a user receives online chat markers, it must be possible to fetch offline chat markers as well.
    • when enabled, this option must ensure filtering of the online chat markers (i.e. no chat markers delivered to the users). As for the carbon copies - the user should receive carbon copies only for his own chat marker messages.
    • when enabled, this option must limit fetching from the DB only to the user's own chat markers.
    • by default, this option must be disabled to ensure the consistent behaviour with XEP-333.
    • The implementation of this configuration option must be done in a generic way (in mod_smart_markers:get_chat_markers() interface), there should be no difference in code for supporting this option in mod_offline_chatmarkers.erl nor in IQ protocol implementation.

Justification: such option allows implementing behaviour similar to a various existing popular messaging application (e.g. slack).

doc/modules/mod_smart_markers.md Outdated Show resolved Hide resolved
doc/modules/mod_smart_markers.md Show resolved Hide resolved

* `<peer-bare-jid>` MUST be the bare jid of the peer whose last marker wants to be checked.
* `<thread>` is an optional attribute that indicates if the check refers to specific a thread in the conversation. If not provided, defaults to the main conversation thread.
* `<after>` is an optional attribute indicating whether markers sent only after a certain timestamp are desired.
Copy link
Contributor

Choose a reason for hiding this comment

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

just add one actual example for each possible destination. I.e. for groupchat and for one2one. Including a date as a date ;)

Something like:

# Query for markers in a room

Request
Result

# Query for markers in one2one conversation

Request
Result

doc/open-extensions/smart-markers.md Outdated Show resolved Hide resolved
@mongoose-im

This comment was marked as outdated.

@NelsonVides NelsonVides changed the title Prepare a documentation page for the smart markers [markers] Prepare a documentation page for the smart markers Feb 15, 2022
@mongoose-im

This comment was marked as outdated.

@mongoose-im

This comment was marked as outdated.

@mongoose-im

This comment was marked as outdated.

@mongoose-im

This comment was marked as outdated.

@mongoose-im
Copy link
Collaborator

mongoose-im commented Mar 11, 2022

small_tests_24 / small_tests / c3b310f
Reports root / small


small_tests_23 / small_tests / c3b310f
Reports root / small


dynamic_domains_mysql_redis_24 / mysql_redis / c3b310f
Reports root/ big
OK: 2756 / Failed: 0 / User-skipped: 150 / Auto-skipped: 0


dynamic_domains_pgsql_mnesia_24 / pgsql_mnesia / c3b310f
Reports root/ big
OK: 2773 / Failed: 0 / User-skipped: 133 / Auto-skipped: 0


dynamic_domains_pgsql_mnesia_23 / pgsql_mnesia / c3b310f
Reports root/ big
OK: 2773 / Failed: 0 / User-skipped: 133 / Auto-skipped: 0


ldap_mnesia_23 / ldap_mnesia / c3b310f
Reports root/ big
OK: 1504 / Failed: 0 / User-skipped: 399 / Auto-skipped: 0


ldap_mnesia_24 / ldap_mnesia / c3b310f
Reports root/ big
OK: 1504 / Failed: 0 / User-skipped: 399 / Auto-skipped: 0


internal_mnesia_24 / internal_mnesia / c3b310f
Reports root/ big
OK: 1545 / Failed: 0 / User-skipped: 358 / Auto-skipped: 0


dynamic_domains_mssql_mnesia_24 / odbc_mssql_mnesia / c3b310f
Reports root/ big
OK: 2773 / Failed: 0 / User-skipped: 133 / Auto-skipped: 0


pgsql_mnesia_24 / pgsql_mnesia / c3b310f
Reports root/ big
OK: 3147 / Failed: 0 / User-skipped: 142 / Auto-skipped: 0


mysql_redis_24 / mysql_redis / c3b310f
Reports root/ big
OK: 3142 / Failed: 0 / User-skipped: 147 / Auto-skipped: 0


elasticsearch_and_cassandra_24 / elasticsearch_and_cassandra_mnesia / c3b310f
Reports root/ big
OK: 1846 / Failed: 0 / User-skipped: 366 / Auto-skipped: 0


pgsql_mnesia_23 / pgsql_mnesia / c3b310f
Reports root/ big
OK: 3147 / Failed: 0 / User-skipped: 142 / Auto-skipped: 0


mssql_mnesia_24 / odbc_mssql_mnesia / c3b310f
Reports root/ big
OK: 3147 / Failed: 0 / User-skipped: 142 / Auto-skipped: 0


riak_mnesia_24 / riak_mnesia / c3b310f
Reports root/ big
OK: 1692 / Failed: 0 / User-skipped: 365 / Auto-skipped: 0

Copy link
Contributor

@arcusfelis arcusfelis left a comment

Choose a reason for hiding this comment

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

ok

@arcusfelis arcusfelis merged commit 9cf140b into feature/smart_markers Mar 14, 2022
@arcusfelis arcusfelis deleted the markers/docs branch March 14, 2022 21:27
@Premwoik Premwoik added this to the 5.1.0 milestone May 31, 2022
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.

None yet

5 participants