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

Initial MyTurn import and field rearrangement #1822

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Conversation

jim
Copy link
Member

@jim jim commented Jan 23, 2025

What it does

  • Moves some fields from ItemPool to ReservableItem as discussed at a recent meetup.
  • Implements the first pass at a script to import item inventory from a MyTurn export.
  • Adds a page that shows the list of ReservableItems in an ItemPool so we can see that the import works 😄

This change was a bit bigger than I was hoping for, so the diff is a bit chaotic.

@jim jim requested a review from crismali January 23, 2025 02:09
module ItemAttributes
module ItemCategorization
Copy link
Member Author

@jim jim Jan 23, 2025

Choose a reason for hiding this comment

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

The story here is that the category code is reusable in both Item and ItemPool, but power_source is only on Item and ReservableItem, so I made this file only include the category parts.

Comment on lines +1 to +22
class AdjustItemPoolFields < ActiveRecord::Migration[7.2]
def change
remove_column :item_pools, :brand, :string
remove_column :item_pools, :model, :string
remove_column :item_pools, :size, :string
remove_column :item_pools, :strength, :string
remove_column :item_pools, :power_source, :enum, enum_type: "power_source"
remove_column :item_pools, :location_area, :string
remove_column :item_pools, :location_shelf, :string
remove_column :item_pools, :purchase_link, :string
remove_column :item_pools, :url, :string
add_column :reservable_items, :size, :string
add_column :reservable_items, :strength, :string
add_column :reservable_items, :power_source, :enum, enum_type: "power_source"
add_column :reservable_items, :location_area, :string
add_column :reservable_items, :location_shelf, :string
add_column :reservable_items, :purchase_link, :string
add_column :reservable_items, :url, :string
remove_column :reservable_items, :name, :string
add_column :item_pools, :checkout_notice, :string
end
end
Copy link
Member Author

Choose a reason for hiding this comment

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

This migration kind of tells the whole story of what's changing, in terms of which fields are moving to where.

Copy link
Contributor

@crismali crismali left a comment

Choose a reason for hiding this comment

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

Looks good. 👍 I had a couple of little notes that are really more like "maybe this would help a little?"

@@ -31,12 +31,6 @@
<strong>
<%= link_to item_pool.name, account_item_pool_path(item_pool) %>
</strong>
<% if item_pool.size.present? %>
Copy link
Contributor

Choose a reason for hiding this comment

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

You can also write this as item_pool.size? if that's your preference


namespace :myturn do
task reset: :environment do
# Just for development
Copy link
Contributor

Choose a reason for hiding this comment

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

Might want to through a raise unless Rails.env.development? at the top of this task.


item_pool = ItemPool.create_with(creator: user).find_or_create_by!(name: row["Name"])

item_pool.update(
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be helpful to make this item.update!. Right now this update can fail silently. (although this one isn't caught by the rescue below)

status: ReservableItem.statuses[:active]
)

item.update(
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like a better candidate for item.update! since it's in the rescue.

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.

2 participants