foodcoopsat / foodsoft_hackathon

Other
1 stars 2 forks source link

OrderArticle.price alias should be removed #71

Open twothreenine opened 4 months ago

twothreenine commented 4 months ago

btw, I suggest to rename order_article.price to version to avoid confusion in places like above.

Yes that would be nice - however it is quite some effort in an untyped languange such as ruby. We had discussed this when we started with our fork and decided to stick with the price method's original name until we have a higher test coverage, so we can be sure we didn't accidentally break stuff by renaming it.

I'd suggest the following approach for order_article:

  # This method returns either the ArticleVersion or the Article
  # The first will be set, when the the order is finished
  def version
    article_version || article
  end

  # Deprecated - call version instead wherever you are sure that order_article.price is called
  def price
    version
  end

Then we could replace it wherever we are sure and improve readability, without having to catch all places where it's called.

@lentschi If you could give me some feedback on this suggestion (do you think version would be the most fitting name for the method?) I could implement that.

lentschi commented 4 months ago

@twothreenine I think it best to not use the alias at all. Just use oa.article_version. (The || article can actually be removed from the current alias - OrderArticle's article_version_id is not nullable.)

lentschi commented 3 months ago

I renamed the issue to match my suggestion.