Pass district id as parameter for `update` and `trigger` #667

Yuuki77 · 2021-04-02T03:20:49Z

Part of #617

pass district id as parameter for update and trigger endpoints.
in following pr, I will add the logic to create heritage with district name.
By adding this endpoint, we can make the heritage unique for each district in the future.

degicat · 2021-04-02T03:46:24Z

@davidsiaw please review this

davidsiaw

Can we use the same update method and just use district if its provided for now?

davidsiaw · 2021-04-02T08:17:44Z

app/controllers/heritages_controller.rb

+  def update_with_district
+    @heritage = BuildHeritage.new(permitted_params, district: @district).execute
+    @heritage.save_and_deploy!(without_before_deploy: false,
+                               description: "Update to #{@heritage.image_path}")
+    render json: @heritage


can't this just be in update but with district as an optional?

@davidsiaw ok I updated the logic to pass the value in body as optional.
Let me know what you think 🙏

davidsiaw

Looks great! We just need more testing to cover some cases

davidsiaw · 2021-04-05T05:33:42Z

app/controllers/heritages_controller.rb

@@ -22,7 +23,7 @@ def create
  end

  def update
-    @heritage = BuildHeritage.new(permitted_params).execute
+    @heritage = BuildHeritage.new(permitted_params, district: @district).execute


Thanks this looks like a very clean way to handle it. Its also a very implicit if statement, so I think we should have a test to get the heritage for the following cases:

endpoint has no district, name passed in

endpoint has no district, name and district passed in

endpoint has district, name passed in

endpoint has district, name and district passed in

endpoint has district, with another endpoint in different district with the same name, name passed in

endpoint has district, with another endpoint in different district with the same name, name and district passed in

@davidsiaw just to confirm, what do you mean by endpointhere?
like endpoint has no district, name passed in
this is the case when pass heritage name but there is no district?

Not pass, but the database has an endpoint but district is set to nil

district has endpionts right?

barcelona/app/models/district.rb

Line 12 in 07009e6

has_many :endpoints, inverse_of: :district, dependent: :destroy

also district name should be unique
also I could not understand what you mean by 🙏

endpoint has district, with another endpoint in different district with the same name, name passed in

@davidsiaw updated the logic
05f4bae
also existing tests cover for when no district name is given

@Yuuki77 I think it is hard to write this test from what I see, because you are testing the API. You should test BuildHeritage instead, and it will be clearer. What we need to test essentially is all the permutations of

The various states of the database

The inputs passed to update

@davidsiaw added the test.
I think the other scenario is that the wrong district is passed (which has always been possible).
So I added the checks.
other that existing tests should cover.
The change I made here was to pass district in update, which was nil.
dd9fef7

davidsiaw · 2021-04-13T03:25:25Z

app/controllers/heritages_controller.rb

@@ -22,7 +23,7 @@ def create
  end

  def update
-    @heritage = BuildHeritage.new(permitted_params).execute
+    @heritage = BuildHeritage.new(permitted_params, district: @district).execute


@Yuuki77 I think it is hard to write this test from what I see, because you are testing the API. You should test BuildHeritage instead, and it will be clearer. What we need to test essentially is all the permutations of

The various states of the database

The inputs passed to update

davidsiaw

Just a few more things

app/controllers/heritages_controller.rb

app/services/build_heritage.rb

spec/requests/update_heritage_spec.rb

Co-authored-by: David Siaw <[email protected]>

Yuuki77 · 2021-04-21T08:42:45Z

Thank you for the reviews 🙇
I have updated it

davidsiaw

LGTM

Yuuki77 force-pushed the update-heritage-with-district branch from d70e332 to bc5f8fe Compare April 2, 2021 03:27

degicat requested a review from davidsiaw April 2, 2021 03:46

davidsiaw requested changes Apr 2, 2021

View reviewed changes

Yuuki77 force-pushed the update-heritage-with-district branch 2 times, most recently from 73a4865 to bcba35c Compare April 5, 2021 05:12

Yuuki77 changed the title ~~Add update_with_district endpoint~~ Pass district id as parameter for update and trigger Apr 5, 2021

Yuuki77 requested a review from davidsiaw April 5, 2021 05:16

davidsiaw requested changes Apr 5, 2021

View reviewed changes

Yuuki77 requested a review from davidsiaw April 5, 2021 06:55

davidsiaw requested changes Apr 13, 2021

View reviewed changes

Yuuki77 added 3 commits April 14, 2021 10:23

Add endpoint

f2714c7

pass district as param

0285686

Add more tests

42a9a9e

Yuuki77 force-pushed the update-heritage-with-district branch from 5c38f2f to 42a9a9e Compare April 14, 2021 02:09

Add test to check if district is valid

dd9fef7

Yuuki77 force-pushed the update-heritage-with-district branch from e12dc5e to dd9fef7 Compare April 14, 2021 02:11

Yuuki77 requested a review from davidsiaw April 14, 2021 02:22

davidsiaw requested changes Apr 20, 2021

View reviewed changes

app/controllers/heritages_controller.rb Outdated Show resolved Hide resolved

app/services/build_heritage.rb Outdated Show resolved Hide resolved

spec/requests/update_heritage_spec.rb Outdated Show resolved Hide resolved

Yuuki77 and others added 5 commits April 21, 2021 15:44

Update app/controllers/heritages_controller.rb

a75aba4

Co-authored-by: David Siaw <[email protected]>

Update spec/requests/update_heritage_spec.rb

935398e

Co-authored-by: David Siaw <[email protected]>

Update app/services/build_heritage.rb

516bd83

Co-authored-by: David Siaw <[email protected]>

Fix according to the reviews

4a5d458

Fix up the tests

506cbc5

Yuuki77 force-pushed the update-heritage-with-district branch from 3cac758 to 506cbc5 Compare April 21, 2021 07:44

Yuuki77 requested a review from davidsiaw April 21, 2021 08:42

davidsiaw approved these changes Apr 27, 2021

View reviewed changes

davidsiaw merged commit 3df6846 into master Apr 27, 2021

davidsiaw deleted the update-heritage-with-district branch April 27, 2021 07:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pass district id as parameter for `update` and `trigger` #667

Pass district id as parameter for `update` and `trigger` #667

Yuuki77 commented Apr 2, 2021 •

edited

Loading

degicat commented Apr 2, 2021

davidsiaw left a comment

davidsiaw Apr 2, 2021

Yuuki77 Apr 5, 2021

davidsiaw left a comment

davidsiaw Apr 5, 2021

Yuuki77 Apr 5, 2021

davidsiaw Apr 5, 2021

Yuuki77 Apr 5, 2021

Yuuki77 Apr 5, 2021 •

edited

Loading

Yuuki77 Apr 5, 2021

davidsiaw Apr 13, 2021

Yuuki77 Apr 14, 2021

davidsiaw Apr 13, 2021

davidsiaw left a comment

Yuuki77 commented Apr 21, 2021

davidsiaw left a comment

Pass district id as parameter for update and trigger #667

Pass district id as parameter for update and trigger #667

Conversation

Yuuki77 commented Apr 2, 2021 • edited Loading

degicat commented Apr 2, 2021

davidsiaw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidsiaw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yuuki77 Apr 5, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidsiaw left a comment

Choose a reason for hiding this comment

Yuuki77 commented Apr 21, 2021

davidsiaw left a comment

Choose a reason for hiding this comment

Pass district id as parameter for `update` and `trigger` #667

Pass district id as parameter for `update` and `trigger` #667

Yuuki77 commented Apr 2, 2021 •

edited

Loading

Yuuki77 Apr 5, 2021 •

edited

Loading