feat: add conflict check for TicketFee #5657

uds5501 · 2019-03-02T10:18:15Z

Fixes fossasia/open-event-frontend#2169

Checklist

I have read the Contribution & Best practices Guide and my PR follows them.
My branch is up-to-date with the Upstream development branch.
The unit tests pass locally with my changes
I have added tests that prove my fix is effective or that my feature works
I have added necessary documentation (if appropriate)

All the functions created/modified in this PR contain relevant docstrings.

Short description of what this resolves:

this makes sure no more than 1 object of a particular Currency-Country combination is being formed

Changes proposed in this pull request:

implement conflict checks for currency-country combinations

app/api/ticket_fees.py

houndci-bot · 2019-03-02T10:19:57Z

app/api/ticket_fees.py

+            pass
+        else:
+            raise ConflictException({'pointer': ''},
+                                    "Currency Country combination ({}-{}) already exists"\


the backslash is redundant between brackets

uds5501 · 2019-03-02T10:22:27Z

Only unique objects are present now:

uds5501 · 2019-03-02T10:24:05Z

@iamareebjamal @CosmicCoder96 Please review :) 🍻

app/factories/ticket_fee.py

- implement conflict checks for currency-country combinations - change TicketFee Factory model - add Unprocessable Entity exception for API

uds5501 · 2019-03-03T07:15:27Z

@CosmicCoder96 @iamareebjamal According to my API conditions, this particular test case should've failed. So, how do I fix that Travis build?

iamareebjamal · 2019-03-05T10:07:55Z

app/api/ticket_fees.py

+            else:
+                raise UnprocessableEntity({'source': ''}, "Country or Currency cannot be NULL")
+        else:
+            raise UnprocessableEntity({'source': ''}, "Country or Currency Attribute is missing")


Use marshmallow for validation

@iamareebjamal could you please elaborate?

https://marshmallow.readthedocs.io/en/3.0/

Understood, shall commit soon

@uds5501 I think here @iamareebjamal simply means that use allow_none=False in the schema itself. It will raise error if country or currency is missing and you can use nullable=False in model to disallow use of NULL values.

@shreyanshdwivedi Ooooh! I understand now. I will update it soon

uds5501 · 2019-03-07T06:08:49Z

@iamareebjamal I was using @validate_schema decorator to apply the condition checks on schema level, but it can't differenciate between a POST and a PATCH request.
My objective is to allow PATCH requests but deny any POST request which aims to create a new object of same currency and country which is already existing.
What shall I do in this case??

iamareebjamal · 2019-03-07T06:11:04Z

The same thing is applied to all types of schema like event, speaker, etc.

Check those.

You can use before hooks but don't validate the schema there.

uds5501 · 2019-03-07T06:39:51Z

@iamareebjamal Thanks! Fixed it now, committing

iamareebjamal

You are still validating the schema. There should be no check for country or currency. It should be handled by Marshmallow

shreyanshdwivedi · 2019-05-17T17:38:04Z

@uds5501 can you please update this ? It seems quite an important bug to me as it deals with the fees charged.

uds5501 · 2019-05-17T18:10:07Z

@shreyanshdwivedi I will be needing some help regarding the marshmallow schema validation. As to how to determine if the new combination is present or not

shreyanshdwivedi · 2019-05-20T00:12:37Z

@uds5501 also don't validate schema where you are doing it currently. Instead use the before hooks which means functions like - before_update_object or before_post_object to raise errors which you are currenlty doing via validating schema.
Reference examples -

open-event-server/app/api/events.py

Line 143 in ad322e3

def before_post(self, args, kwargs, data=None):

See how validate_event function is called before posting. If there is any discrepancies error is raised. Similarly you can find checks before updating the object. Implement something like this.
Hope you understood 😅

app/api/ticket_fees.py

app/api/schema/ticket_fees.py

app/api/ticket_fees.py

uds5501 · 2019-05-21T14:18:36Z

@mrsaicharan1 @shreyanshdwivedi @iamareebjamal @CosmicCoder96
I want the model to not take any NULL values, but for that, I would need the migration file to ensure that old entries of ticket_fees country and currency fields are filled up with some server_default values.
can I set them to be 'United States' and 'USD' respectively?

iamareebjamal · 2019-05-21T15:10:31Z

You can just define those in marshmallow schema for now

uds5501 · 2019-05-22T04:22:46Z

@iamareebjamal understood, shall do that

uds5501 · 2019-05-22T06:48:31Z

@iamareebjamal @shreyanshdwivedi Please review

iamareebjamal · 2019-05-22T08:29:54Z

app/api/schema/ticket_fees.py

    id = fields.Integer(dump_only=True)
-    currency = fields.Str(validate=validate.OneOf(choices=PAYMENT_CURRENCY_CHOICES), allow_none=True)
-    country = fields.String(allow_none=True)
+    currency = fields.Str(validate=validate.OneOf(choices=PAYMENT_CURRENCY_CHOICES), allow_none=False, default='USD')


Don't default to USD. We need the user to provide the values. There is no UI showing the user that it will default to USD if no value is provided

@iamareebjamal So I should use no default value?

uds5501 · 2019-05-30T16:11:57Z

@iamareebjamal I have removed the default values from schema, does anything else needs to be done now?

iamareebjamal · 2019-05-30T16:51:27Z

app/api/ticket_fees.py

+        if (data['country'] and data['currency']):
+            try:
+                already_existing_combination = TicketFees.query.filter_by(country=data['country'],
+                                                                          currency=data['currency']).one()


Just add a unique key on country and currency. No need for this check

@iamareebjamal A unique key constraint would ensure that I don't get to save similar values of the same column.

I want to be able to save :USA - USD and USA-INR combination simultaneously. I don't think adding unique constraints would allow that

You are simulating unique key constraint on TicketFee, if unique key won't work, this shouldn't work too

You are ensuring that country and currency pair is unique and not repeated. That's the work on unique key

shreyanshdwivedi · 2019-06-03T16:20:52Z

@uds5501 similar to my PR, this is failing due to multiple heads. Please fix it

…to feat-f-2169

app/api/ticket_fees.py

uds5501 · 2019-06-03T19:37:55Z

@shreyanshdwivedi do i need to change the revises in migration file here?

shreyanshdwivedi · 2019-06-03T19:41:40Z

migrations/versions/cb4ccda76a2b_.py

+
+# revision identifiers, used by Alembic.
+revision = 'cb4ccda76a2b'
+down_revision = '0e80c49a6e28'


@uds5501 change down_revision and revises to Revision ID of migration in my latest merged PR

uds5501 · 2019-06-04T05:20:12Z

@shreyanshdwivedi this PR won't work until the dredd hook request is fixed. I am opening an issue to fix this request :

{
  "data": {
    "type": "ticket-fee",
    "attributes": {
      "currency": "USD",
      "service-fee": "3.14",
      "maximum-fee": "5.14"
    }
  }
}

iamareebjamal

You have to add composite unique key on country and currency if I understand correctly. Shouldn't I be able to have ('UK', 'GBP') and ('UK', 'EUR')?

uds5501 · 2019-06-04T10:58:37Z

@iamareebjamal you are right, I will look for corresponding resources and update this soon

kushthedude · 2019-06-09T11:43:03Z

Closed in favour of #6017

houndci-bot reviewed Mar 2, 2019

View reviewed changes

app/api/ticket_fees.py Outdated Show resolved Hide resolved

app/api/ticket_fees.py Outdated Show resolved Hide resolved

houndci-bot reviewed Mar 2, 2019

View reviewed changes

app/factories/ticket_fee.py Outdated Show resolved Hide resolved

uds5501 force-pushed the feat-f-2169 branch from fd7427e to d0c60ee Compare March 2, 2019 18:25

fossasia deleted a comment Mar 2, 2019

fossasia deleted a comment Mar 3, 2019

feat : add conflict check for TicketFee

71169ba

- implement conflict checks for currency-country combinations - change TicketFee Factory model - add Unprocessable Entity exception for API

uds5501 force-pushed the feat-f-2169 branch from 24d2e48 to 71169ba Compare March 3, 2019 06:08

update error messages

0095c1c

fossasia deleted a comment Mar 3, 2019

iamareebjamal reviewed Mar 5, 2019

View reviewed changes

uds5501 added 2 commits March 7, 2019 12:11

remove before_post hook

d522e78

add schema validation for ticket_fee

422809e

fossasia deleted a comment Mar 7, 2019

iamareebjamal requested changes Mar 7, 2019

View reviewed changes

Merge branch 'development' into feat-f-2169

eff6b1e

houndci-bot reviewed May 21, 2019

View reviewed changes

app/api/ticket_fees.py Outdated Show resolved Hide resolved

uds5501 force-pushed the feat-f-2169 branch from dcd77cb to 80742a3 Compare May 21, 2019 14:12

uds5501 changed the title ~~feat : add conflict check for TicketFee~~ feat: add conflict check for TicketFee May 21, 2019

auto-label bot added the feature label May 21, 2019

fossasia deleted a comment May 22, 2019

iamareebjamal reviewed May 22, 2019

View reviewed changes

uds5501 force-pushed the feat-f-2169 branch from 63f5c32 to 80742a3 Compare May 30, 2019 16:09

iamareebjamal reviewed May 30, 2019

View reviewed changes

uds5501 added 2 commits June 3, 2019 22:11

Merge remote-tracking branch 'upstream/development' into feat-f-2169

8b670c1

Merge branch 'development' into feat-f-2169

88c3765

fossasia deleted a comment Jun 3, 2019

Merge branch 'feat-f-2169' of github.com:uds5501/open-event-server in…

50c844e

…to feat-f-2169

houndci-bot reviewed Jun 3, 2019

View reviewed changes

app/api/ticket_fees.py Outdated Show resolved Hide resolved

remove api checks and make fields unique

1257bb0

uds5501 force-pushed the feat-f-2169 branch from 0686d8b to 1257bb0 Compare June 3, 2019 17:35

fossasia deleted a comment Jun 3, 2019

shreyanshdwivedi reviewed Jun 3, 2019

View reviewed changes

uds5501 mentioned this pull request Jun 4, 2019

The ticket_fees documentation needs to be updated #5994

Closed

iamareebjamal requested changes Jun 4, 2019

View reviewed changes

uds5501 mentioned this pull request Jun 9, 2019

fix: add unique constraint making country-currency combination #6017

Merged

6 tasks

uds5501 closed this Jun 9, 2019

feat: add conflict check for TicketFee #5657

feat: add conflict check for TicketFee #5657

Uh oh!

Conversation

uds5501 commented Mar 2, 2019

Checklist

Short description of what this resolves:

Changes proposed in this pull request:

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

uds5501 commented Mar 2, 2019

Uh oh!

uds5501 commented Mar 2, 2019

Uh oh!

Uh oh!

uds5501 commented Mar 3, 2019

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!

uds5501 commented Mar 7, 2019

Uh oh!

iamareebjamal commented Mar 7, 2019

Uh oh!

uds5501 commented Mar 7, 2019

Uh oh!

iamareebjamal left a comment

Choose a reason for hiding this comment

Uh oh!

shreyanshdwivedi commented May 17, 2019

Uh oh!

uds5501 commented May 17, 2019

Uh oh!

shreyanshdwivedi commented May 20, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

uds5501 commented May 21, 2019

Uh oh!

iamareebjamal commented May 21, 2019

Uh oh!

uds5501 commented May 22, 2019

Uh oh!

uds5501 commented May 22, 2019

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!

uds5501 commented May 30, 2019

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