crowdint / gopher-spree-api

2 stars 1 forks source link

Variant repository hardcoded currency #91

Closed joiggama closed 9 years ago

joiggama commented 9 years ago

Right now:

# interfaces/repositories/variant.go
func (this *VariantRepository) FindByProductIds(productIds []int64) ([]*domain.Variant, error) {

    variants := []*domain.Variant{}

    if len(productIds) == 0 {
        return variants, nil
    }

    query := this.dbHandler.
        Table("spree_variants").
        Select("spree_variants.*, SUM(count_on_hand) AS real_stock_items_count, spree_stock_items.backorderable, spree_prices.amount price").
        Joins("INNER JOIN spree_stock_items ON spree_variants.id = spree_stock_items.variant_id INNER JOIN spree_prices ON spree_variants.id = spree_prices.variant_id").
        Where("spree_prices.currency='USD'"). //TODO: Remove hardcoded currency
        Where("spree_variants.product_id IN (?)", productIds).
        Group("spree_variants.id, spree_variants, backorderable, price").
        Scan(&variants)

    if query.Error != nil && query.Error != gorm.RecordNotFound {
        return variants, nil
    } else {
        return variants, query.Error
    }
}

@crowdint/gopher-spree-api We should remove the hardcoded currency, I wonder... how can we achieve this?

chischaschos commented 9 years ago

What do you think about introducing a Money abstraction as in https://github.com/RubyMoney/money ?

jpfuentes2 commented 9 years ago

@chischaschos we know we need one so if they do we should share thoughts because I've compiled a big list on the challenges. It depends on how deep they need to go :)

joiggama commented 9 years ago

@chischaschos @jpfuentes2 I've already ported some of the very basic functionality, joiggama/money

It just deals with representation, so IT DOES NOT:

jpfuentes2 commented 9 years ago

Yeah, the big problem with your money is that it uses float:

The initial concerns that we have about money are:

joiggama commented 9 years ago

I see, then we also have the problem from the spree database, right? I understand they use floats as well, we have all of our domain entities using floats from the db.

joiggama commented 9 years ago

Is that an actual problem if we don't do any of the previously mentioned features?. As of now that library just handles the string representation, and don't intend to add any floating point arithmetic.

jpfuentes2 commented 9 years ago

No, I think you're right that it's not a problem for gopher-spree-api but FoxComm couldn't re-use it :sob:. No worries, may be fork off your base and change to our needs. Ignore our static on this issue :)

joiggama commented 9 years ago

Alright, I thought I might be missing something. I was aware that implementing something like arithmetics for currency would not be an easy thing. Thanks.

joiggama commented 9 years ago

For now, we'll be using default spree currency read from spree config.