Move references to user_storage_ids_table to one place #44801
Conversation
maureensturgeon
force-pushed
the
maureen/user-storage-ids-query-consolidation
branch
from
a67a23f
to
1ea1b6c
Compare
maureensturgeon
force-pushed
the
maureen/user-storage-ids-query-consolidation
branch
from
77e595d
to
9e7ba06
Compare
dashboard/lib/sample_data.rb
Outdated
| # For the given user id, looks up the storage id, or creates a new one if the user doesn't have one. | ||
| def self.find_or_create_storage_id_for_user_id(user_id) | ||
| environment_check! | ||
| row = user_storage_ids_table.where(user_id: user_id) | ||
| return row[:id] if row | ||
| user_storage_ids_table.insert(user_id: user_id) | ||
| storage_id = storage_id_for_user_id(user_id) | ||
| return storage_id if storage_id | ||
| create_storage_id_for_user(user_id) | ||
| end |
There was a problem hiding this comment.
This whole helper method seems like something that should exist in storage_id.rb. We might even be able to avoid exposing create_storage_id_for_user directly?
There was a problem hiding this comment.
This method looks like it's only used in this sample_data file for creating test data
There was a problem hiding this comment.
The main entry point to creating new storage id records is https://github.com/code-dot-org/code-dot-org/blob/staging/shared/middleware/helpers/storage_id.rb#L94-L96
There was a problem hiding this comment.
I see. Maybe we can make it clearer that create_storage_id_for_user should only be used for test purposes, either with a comment on the method or by renaming it?
There was a problem hiding this comment.
Sure I'll update the comment.
maureensturgeon
deleted the
maureen/user-storage-ids-query-consolidation
branch
The intent of this PR is to replace references to
PEGASUS_DB[:user_storage_ids]with helper methods to create a layer of abstraction and have less direct references to the table since the table is going to change. Note that there are still direct queries through joins inprojects_list.rbwhich don't make sense to replace.For a sneak peek of where this could be headed see: https://github.com/code-dot-org/code-dot-org/pull/44957/files
One option that I was considering was making a new class
UserStorageIdswhich would have methods likeget_by_id,get_by_user_id,exists?andcreate, but I wasn't sure if it was worth the effort because we'll be replacing a lot of this code soon anyway.Testing story
This is a refactor with no increase in test coverage, however I did update some unit tests where it made sense.
PR Checklist: