Keep track of user activity

[cyhsutw]

Hello guys,

This PR is related to #356, it tracks the timestamp of a user's last access to the app.

Staffs can view the timestamp using stafftools, as shown below:

The difference between #571 and this pull request is that whether the updated_at of User will be updated or not.

[tarebyte]

Why don't we set the last_active_at to created_at when the user is first created? That way we never have the case where the last_active_at is unknown.

[cyhsutw]

This should not be null in our main Postgres database.

I was wondering if there is a case that the indexes on Elasticsearch is corrupted/outdated, which might cause this value to be null.

[tarebyte]

@cyhsutw thanks for doing this.

What do you think about writing a data transition script so that we can set these timestamps for everyone?

The process I see here is:

• Make a PR with just the migration, with last_active_at allowed to be null
• Run a data transition script that uses the created_at or updated_at timestamp whatever you prefer.
• Make a PR with another migration that enforces last_active_at to be present
• Merge this PR with the appropriate changes.

Does that sound alright?

[cyhsutw]

@tarebyte thanks for the feedback.

I'm totally 🆒 with the process you suggested.

Just a little question about:

Run a data transition script that uses the created_at or updated_at timestamp whatever you prefer.

If we do this in a migration like:

class SetUserLastActiveAtValue < ActiveRecord::Migration
  def change
    User.update_all('last_active_at = updated_at')
  end
end

The elasticsearch indexes will not be updated, thus we might have null values in Stafftools.
Is there any way to rebuild the indexes, or we should create a rake to scan all of the users and update index for each user instead?

[cyhsutw]

@tarebyte I added some migration files, if there's no problem with those, I'll go ahead and open separate PRs for each of them.

Also, I am working on adding few more test for this change, so I'm marking this as WIP for now.

[cyhsutw]

I added a test to the User model.

Should we add another test to check if the StafftoolsIndex::User is updated?

cc/ @tarebyte

[tarebyte]

Should we add another test to check if the StafftoolsIndex::User is updated?

I don't think that's necessary

[tarebyte]

@cyhsutw What do you think about something like this?

return become_active if logged_in? && adequate_scopes?
auth_redirect

[cyhsutw]

🆒 That's much more elegant!

[tarebyte]

I think these comments are useful, but would be better placed at the top.

# This updates the `last_active_at` column without
# updating the model, but keeps the index updated.
def become_active
  update_columns(last_active_at: Time.zone.now)
  self.class.update_index('stafftools#user') { self }
end

[tarebyte]

We should also check here that the updated_at column remains the same before and after.

it 'does not change the updated_at column' do
  updated_at = user.updated_at
  user.become_active
  expect(updated_at).to eql(user.updated_at)
end

[cyhsutw]

Done in #b3ed41b.

[tarebyte]

I left a few minor comments, @cyhsutw. After that this is good to go!

[cyhsutw]

@tarebyte Thanks for reviewing the changes, just updated the code based on your comments.

[tarebyte]

Confirmed in production!