Skip to content
This repository has been archived by the owner on Mar 3, 2020. It is now read-only.

add "price lists" support #2

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

alepore
Copy link
Member

@alepore alepore commented Apr 24, 2015

hope it's clear :)


# Drawbacks

I don't see many drawbacks: we don't need to *add* much code, we just have to
Copy link
Member

Choose a reason for hiding this comment

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

Potential drawbacks are degraded performance when determining which list should be used to display pricing to the user.

Copy link
Contributor

Choose a reason for hiding this comment

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

I concur.

@JDutil
Copy link
Member

JDutil commented Apr 24, 2015

I'm in favor of improving Spree for something like this, and I had helped @simmsy & @forvalho put together the spree_price_books extension for Surfdome. It definitely required overriding some important pieces of Spree like Variant#price_in it is on the https://trello.com/b/PQsUfCL0/spree-roadmap to add price books/lists to Spree but whichever route we take your more simplified version or their more complex version I believe either one needs some more polish before it's ready. On the plus side though a good deal of work has already been done on this front. I think we just need to determine how complex we want it to be, and polish it up so performance isn't an issue.

@BenMorganIO
Copy link
Contributor

@alepore with formal english, the word "i" is always capitalized. Hope that helps with the English.

@BenMorganIO
Copy link
Contributor

@alepore an unresolved question is not for you to be better at english. Can we make the RFC a more formal proposition of change in Spree?

You're opening for Motivation is:

I'm doing this because i needed this feature on some different projects and
the current spree prices/currency logic was a limit.

This sentence takes into assumption that I already know what your issue is. I'm actually left guessing. What is a Price List?

Can you update your RFC for a more formal audience and update the Drawbacks based on @JDutil's comment. If you do not concur, please also update in a respectable section as to why it won't be.

@alepore
Copy link
Member Author

alepore commented Apr 29, 2015

thanks for feedback, i made some updates

@alepore alepore changed the title add proposal to add price lists on the core add "price lists" support Apr 29, 2015
@BenMorganIO
Copy link
Contributor

I'm 👍 on this. I'd love to hear other peoples thoughts.

@mamhoff
Copy link

mamhoff commented May 20, 2015

This will require some work on the frontend: Caching will have to work differently for all partials with prices. All the cache keys for those partials will have to include whatever variable is used to determine which price from the list should be shown or selected.

We did this for differing prices for different VAT zones, and it required a current_tax_zone helper. Is there any way to generalize this?

How would the choosing of different prices from the book work? Based on the order's address, or on the user? or some current_price_parameters hash?

I think it's a worthwhile addition nevertheless. Keep up the good work!

@alepore
Copy link
Member Author

alepore commented May 23, 2015

@mamhoff :

  • yes we must switch current_currency cache keys to current_price_list. that's probably the easy part :)
  • adding an helper for common shared cache keys sounds good
  • choosing the price list will work by overriding the current_price_list method. we should just provide a simple default to not break existing store, than each store wit different needs will add its own logic.

thanks for reply!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants