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(userstatus): Track message timestamp too #40564

Merged
merged 1 commit into from
Sep 29, 2023

Conversation

ChristophWurst
Copy link
Member

@ChristophWurst ChristophWurst commented Sep 21, 2023

Summary

Add new database column to track the message change date.

The fix requires a database schema change and data migration. I would vote against backporting.

How to test

  1. Set up user A
  2. Set up user B
  3. Log in as A
  4. Log in as B in a private window
  5. Set your status to DND and the status message to "test" as user A
  6. Wait 2 minutes
  7. Set your status to online as user A (do not use the Save status message button. Online status change doesn't need a confirmation)
  8. Open the dashboard and recent statuses as user B

master: User A shows "test, seconds ago"
this branch: User A shows "test, 2 minutes ago"

Screenshots

Before After
Bildschirmfoto vom 2023-09-26 14-31-52 Bildschirmfoto vom 2023-09-26 14-31-22

TODO

Checklist

$schema = $schemaClosure();

$statusTable = $schema->getTable('user_status');
$statusTable->getColumn('status_message_timestamp')->setNotnull(true);
Copy link
Member

Choose a reason for hiding this comment

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

could skip this migration by going with default => 0 on the column

@ChristophWurst ChristophWurst added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Sep 26, 2023
@ChristophWurst ChristophWurst marked this pull request as ready for review September 26, 2023 17:48
Copy link
Contributor

@Antreesy Antreesy left a comment

Choose a reason for hiding this comment

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

🦭

Tested with my 0-PHP-knowledge, works fine for custom statuses.
Should it also work for predefined ones, like "Commuting"?

Also noted:

  • upgrade to 1.8.1 works smoothly, status is saved
  • downgrade to 1.8.0 breaks the instance, but that should be expected, i guess

@nickvergessen
Copy link
Member

downgrade to 1.8.0 breaks the instance, but that should be expected, i guess

Unfortunately yes. YOu can manually delete the column from the database table (but that will break on updating, unless you also remove the row in oc_migrations table, so I personally would prefer to reinstall)

@nickvergessen
Copy link
Member

Should it also work for predefined ones, like "Commuting"?

When selecting them I guess yes. When they fade (because clearAt was reached) and you turn into "Away/Online" I guess also yes?

@ChristophWurst
Copy link
Member Author

ChristophWurst commented Sep 27, 2023

When they fade

Right! Forgot to test those.

When the predefined statuses are selected they should definitely update the status message timestamp.

@ChristophWurst
Copy link
Member Author

When they fade

Right! Forgot to test those.

Going from a preset status (or custom status) to no status erases the database entry. There is no record of the change on the recent statuses widget. I would say this is the expected behavior, because there is no recent status to list.

@nickvergessen
Copy link
Member

Going from a preset status (or custom status) to no status erases the database entry.

Right, then that's how it is

@nickvergessen
Copy link
Member

Restoring from backup status (e.g. custom status > join a call for IN_CALL hardcoded one > leave call again) should also update the timestamp? Or not?

@ChristophWurst
Copy link
Member Author

ChristophWurst commented Sep 28, 2023

Phew. It's getting complicated.

If we update the status message timestamp when an automated status is entered, it should also update when that status ends and the previous status is restored from backup. Con: status shows as updated at restore when the user didn't update it themselves.

  1. "Working remotely, 2h ago"
  2. "In a call, 28 min ago"
  3. "Working remotely, 4m ago"

Alternatively, we can not update the timestamp for entering/leaving an automated status. Pro/con: automated statuses don't show on the recent statuses widget.

  1. "Working remotely, 4h ago"

@jancborchardt are you okay with the first second approach? It's easier to implement.

@ChristophWurst
Copy link
Member Author

Integration test suite added so we can be sure the strange edge cases all work as expected ☺️

@ChristophWurst
Copy link
Member Author

@Antreesy @nickvergessen if you are okay with the current solution we can integrate this (and unblock #40559). If we decide that automated/backup statuses should update the timestamp as well we can still change that behavior later on 👍

@nickvergessen
Copy link
Member

if you are okay with the current solution we can integrate this (and unblock #40559). If we decide that automated/backup statuses should update the timestamp as well we can still change that behavior later on 👍

Agreed

@ChristophWurst ChristophWurst force-pushed the fix/userstatus/message-timestamp branch from d7a5f00 to 2790602 Compare September 29, 2023 07:49
@ChristophWurst ChristophWurst force-pushed the fix/userstatus/message-timestamp branch from 2790602 to fbdf733 Compare September 29, 2023 07:56
@ChristophWurst ChristophWurst merged commit 94430ed into master Sep 29, 2023
38 checks passed
@ChristophWurst ChristophWurst deleted the fix/userstatus/message-timestamp branch September 29, 2023 17:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: User status widget shows incorrect relative timestamp
4 participants