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

OrderArticle.price alias should be removed #71

Open
twothreenine opened this issue May 26, 2024 · 2 comments
Open

OrderArticle.price alias should be removed #71

twothreenine opened this issue May 26, 2024 · 2 comments

Comments

@twothreenine
Copy link
Contributor

twothreenine commented May 26, 2024

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
Copy link
Contributor

lentschi commented Jun 7, 2024

@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 added a commit that referenced this issue Jun 7, 2024
@lentschi lentschi added this to the Post-merge milestone Jun 7, 2024
@lentschi lentschi changed the title order_article.price should be renamed to version OrderArticle.price alias should be removed Jul 5, 2024
@lentschi
Copy link
Contributor

lentschi commented Jul 5, 2024

I renamed the issue to match my suggestion.

lentschi added a commit that referenced this issue Jul 26, 2024
lentschi added a commit that referenced this issue Oct 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants