Skip to content

Remove Devise :trackable module#6148

Merged
mitchellhenke merged 2 commits intomainfrom
mitchellhenke/remove-devise-trackable-module
Apr 4, 2022
Merged

Remove Devise :trackable module#6148
mitchellhenke merged 2 commits intomainfrom
mitchellhenke/remove-devise-trackable-module

Conversation

@mitchellhenke
Copy link
Contributor

@mitchellhenke mitchellhenke commented Apr 4, 2022

We have 5 columns created for Devise's :trackable module:

  1. sign_in_count
  2. current_sign_in_at
  3. last_sign_in_at
  4. current_sign_in_ip
  5. last_sign_in_ip

We currently update these columns, but do not reference them in code or use them for any reports, so we can safely stop writing them and then later drop them.

This goes wayyyyyyyyy back to @jgrevich's #2 (which will be celebrating it's 6th birthday next week!), but I'm not sure when we may have stopped using these columns.

Copy link
Contributor

@zachmargolis zachmargolis left a comment

Choose a reason for hiding this comment

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

LGTM! should we also add these to ignored_columns so that we get errors if any code uses them?

changelog: Internal, Optimization, Stop updating unused columns in database
@mitchellhenke mitchellhenke force-pushed the mitchellhenke/remove-devise-trackable-module branch from 0cf2651 to d4bfcb5 Compare April 4, 2022 19:32
@mitchellhenke
Copy link
Contributor Author

mitchellhenke commented Apr 4, 2022

LGTM! should we also add these to ignored_columns so that we get errors if any code uses them?

Yep, great call, added that

Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com>
Copy link
Contributor

@orenyk orenyk left a comment

Choose a reason for hiding this comment

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

Question - are we going to also remove the columns from the DB? Otherwise looks good to me (assuming all tests pass and whatnot)

@mitchellhenke
Copy link
Contributor Author

Question - are we going to also remove the columns from the DB? Otherwise looks good to me (assuming all tests pass and whatnot)

Yep, that's the plan. We need to do it in two steps though so that the deploy doesn't drop the columns while the old servers are still trying to write to them.

@mitchellhenke mitchellhenke merged commit ee17446 into main Apr 4, 2022
@mitchellhenke mitchellhenke deleted the mitchellhenke/remove-devise-trackable-module branch April 4, 2022 20:06
@orenyk
Copy link
Contributor

orenyk commented Apr 4, 2022

@mitchellhenke 🤦 I should have thought of that, thanks!

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