Feature/overwrite #574
Feature/overwrite #574
Conversation
|
Missing from this PR is the UI for lawrence to populate the table, but I wanted to do a sanity check with you guys first before carrying forward.. any comments @kkolotyuk @sharipov-ru @rmascarenhas ? |
|
So I didn't get. We want to override host's fields or each property's fields? |
|
As I understood, @keang wants to be able to easily update cancellation policies, without modifying every supplier. |
|
Yes, but he wants to update it per host or per property? |
|
This PR will allow both =D |
|
Ok I see. The idea looks fine to me. |
keang
force-pushed
the
feature/overwrite
branch
from
5051f68
to
5558e4a
Compare
Access Overwrites from hosts list:
list of overwrites for a host
Create/edit form:
Complete with flashes:
|
|
@kkolotyuk @sharipov-ru please 👀 |
| change do | ||
| create_table :overwrites do | ||
| primary_key :id | ||
| foreign_key :host_id, :hosts, on_delete: :set_null, null: false |
There was a problem hiding this comment.
on_delete: :set_null, null: false this is a contradiction isn't it?
There was a problem hiding this comment.
this is would actually prevent deleting a host that still has overwrites.
The host deletion flow need to delete existing overwrites before deleting a host. Will add to our exising Flows::HostDeletion
| class OverwriteRepository | ||
| include Hanami::Repository | ||
|
|
||
| # returns the total number of suppliers in the database. |
| expose :overwrite | ||
|
|
||
| def call(params) | ||
| @overwrite = Overwrite.new |
| end | ||
|
|
||
| def valid_json? | ||
| json_decode(data_json).success? |
There was a problem hiding this comment.
May be add Hash class check here. Because json_decode returns success result for nil and any string and here what I see in this case:
keang
force-pushed
the
feature/overwrite
branch
from
83ac013
to
1717c20
Compare
keang
force-pushed
the
feature/overwrite
branch
from
1717c20
to
9370f07
Compare
| @@ -5,7 +5,7 @@ class New | |||
| expose :overwrite | |||
|
|
|||
| def call(params) | |||
| @overwrite = Overwrite.new | |||
| @overwrite = Overwrite.new | |||
|
Arggghhh
…On Wed, Nov 30, 2016 at 7:44 PM, Konstantin Kolotyuk < ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In apps/web/controllers/overwrites/new.rb
<#574 (review)>
:
> @@ -5,7 +5,7 @@ class New
expose :overwrite
def call(params)
- @overwrite = Overwrite.new
+ @overwrite = Overwrite.new
still 2 spaces 💅
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#574 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AB01GioqJL7luSD6NOsxZZtqL7xWc99Iks5rDWGQgaJpZM4K8K6K>
.
|
|
Looks good! |
Context
We have a request for a feature to allow admin to overwrite certain host's (all his cancellation policy) cancellation_policy. Effort started with https://github.com/roomorama/concierge/pull/565/files, to add a new column to host and inject the host's cancellation policy into supplier mappers.
The issue
Then next week there'll be a request to overwrite the cancellation_policy per property. I want to prepare for more of such adhoc custom field overwrites (some host want to default available? some host want to have overwrite currencies? because of the Chinese distribution maybe?)
So what might be better than creating new column on the properties, then injecting into Mapper implementations? (11 classes to modify for 11 suppliers)
Solution (this PR)
We can have a "overwrites" table which has:
host_id: int,
property_identifier: string,
data: text(json)
As Lawrence receive a spreadsheet from host detailing the custom
cancellation_policy, he just populate the above table, preempting property identifiers before the synchronisation process created a row on the properties table.PropertySynchronisation#start applies these overwrites before sending to properties to Roomorama.
If an update is removed from the database, the supplier defaults will be kicked in and sent to Roomorama instead.
With this we can halt #565