Implementation of feature 1 and 2 by ahteshamtariq · Pull Request #1 · ahteshamtariq/payments-assignment

ahteshamtariq · 2023-04-20T17:51:27Z

No description provided.

- associate municipality with price - associate municipality with package - add test cases for Municipality model - add DB seed for municipality

- adding some test cases for feature 1

- add test cases for feature 2

ziadsawalha · 2023-04-24T09:08:33Z

import/municipalities.yaml

@@ -0,0 +1,5 @@
+---


I don't know why we need --- here since we have only one document in this file. But I did notice that's how the other imports were set up. So 👍 for following the pre-existing convention.

Yes, there was no need to add this.
Yes only reason to add this is follow the previous convention.

ziadsawalha · 2023-04-24T09:12:28Z

spec/lib/price_history_spec.rb

-      "Göteborg" => [100_00],
-      "Stockholm" => [30_00, 40_00],
-    )
+                         "Göteborg" => [100_00],


Is this change an intentional formatting update?

@ziadsawalha Yes, As for the new test cases, I've used my preset formatting so I've changed this one to match the file in the same format. Otherwise there is no need to do this.

ziadsawalha · 2023-04-24T09:13:34Z

I very much appreciate the commit messages and how you kept each change in a separate commit!

romangt · 2023-04-24T09:55:06Z

Gemfile.lock

      net-protocol
      timeout
    nio4r (2.5.8)
+    nokogiri (1.13.10-arm64-darwin)


What was the intention to include this gem?

@romangt This is my platform dependency. It's the same gem that is at line 109. I'm using Mac M1 so this gem is used to operate on the platform.

Yeah. I have the same pop up for me since I have an M1 as well. Apple Silicon uses an arm64 architecture and some gems need to be compiled to it.

romangt · 2023-04-24T10:00:31Z

app/models/package.rb

@@ -2,6 +2,7 @@

 class Package < ApplicationRecord
  has_many :prices, dependent: :destroy


Could you please explain your intention of leaving prices directly related to package? Do you think this a good approach? This will force us to always do if/else depending on municipalities. Maybe some kind of "global" municipality can act as direct package prices?

I've left this price directly related to the package keeping in mind the backward compatibility. If we remove this the old data in the application may not be able to fully functional. As in Feature 1 it is stated In other words, a package should be able to have different prices depending on a municipality. and We still want to have our pricing log, but now with the added municipalities. and there is no statement of ignoring the backward compatibility so I assumed it should be backward compatible. Also, as stated in Feature 2 requirements the municipality is an optional filter.
Putting if/else is not good in general, if we ignore backward compatibility then I think what you have suggested is the more reasonable and best fitted.

Is there any statement in the docs about handling pre-existing prices in the code? Should they be migrated (in the db), supported, or assumed to not exist?

@ziadsawalha please see my comments above.

romangt · 2023-04-24T10:06:48Z

app/models/municipality.rb

+  has_many :prices
+
+  validates :name, presence: true, uniqueness: true
+  validates :price_cents, presence: true


We have has_many: prices and we have price_cents. Why do we need both?

We can eliminate the price_cents, but I've assumed the same structure as for the Package that Municipality will store the current price in price_cents and has_many: prices will store the history of price.

romangt · 2023-04-24T10:15:11Z

app/services/update_package_price.rb

-
-      # Update the current price
-      package.update!(price_cents: new_price_cents)
+      price.save!


Is it correct that running this method we will get a price attached to package and municipality?

If we see the backward compatibility then it can generate a price with the only package and with new requirements with the municipality and package also.

romangt · 2023-04-24T10:16:48Z

spec/services/update_package_price_spec.rb

  # the existing one.

-  xit "supports adding a price for a specific municipality" do
+  it "supports adding a price for a specific municipality if municipality not exists" do


You should use context to avoid conditions in spec names

I agree it's good to use context. I just tried it to match the already used convention.

romangt · 2023-04-24T10:20:14Z

spec/services/update_package_price_spec.rb

+    expect(package.municipalities.first.name).to eq("Göteborg")
+  end
+
+  it "supports adding a price for a specific municipality if municipality exists" do


generally it is a good practice to have one expect per spec.
Here you can have 3 separate specs:

it creates municipality

it sets correct price

it sets correct name
It is much easier to read specs in that case

Also, Municipality.create!(name: 'Göteborg', price_cents: 100_00, package: package) looks like a preparation step which can be moved to before hook. It makes specs clearer

romangt · 2023-04-24T10:24:22Z

app/lib/price_history.rb

+    # Price history records including their municipality associations
+    price_history = Price.includes(:municipality)
+    # Apply additional filters to the query based on the given options.
+    price_history = price_history.where(package: options[:package]) if options[:package].present?


According to conditions of test task, package and year will be always provided

Yes in the task description, both will be present.
I've updated it by considering that we can extend the functionality to this also.

romangt · 2023-04-24T10:25:33Z

app/lib/price_history.rb

-
-    raise NotImplementedError, "Implement this to pass the assignment"
+    # Group the results by the name of the municipality and extract the price values as an array
+    puts price_history.group_by { |price| price.municipality&.name }.map { |key, value| [key, value.map(&:price_cents)] }.to_h


Redundant puts

Thank you, for highlighting this.

ahteshamtariq added 4 commits April 20, 2023 18:30

- gem install

5d63d0b

- add municipality structure

25f834c

- associate municipality with price - associate municipality with package - add test cases for Municipality model - add DB seed for municipality

- implementing feature 1 request

634a797

- adding some test cases for feature 1

- implement feature 2

8e415d6

- add test cases for feature 2

ahteshamtariq changed the title #1 Implementation of feature 1 and 2 Apr 20, 2023

ziadsawalha reviewed Apr 24, 2023

View reviewed changes

romangt reviewed Apr 24, 2023

View reviewed changes

		@@ -2,6 +2,7 @@

		class Package < ApplicationRecord
		has_many :prices, dependent: :destroy

Conversation

ahteshamtariq commented Apr 20, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ziadsawalha commented Apr 24, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants