fix: Add proper unique constraints and remove soft deletion from models #7098

Haider8 · 2020-06-30T15:09:07Z

Fixes #7093

Short description of what this resolves:

Not all models requires soft deletion so we have to remove it from models like CustomForms and EventSubTopic.

In some models (like DiscountCode) we need to add a unique constraint on deleted_at column.

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.

houndci-bot · 2020-06-30T17:03:08Z

app/models/discount_code.py

@@ -13,7 +13,9 @@
 class DiscountCode(SoftDeletionModel):
    __tablename__ = "discount_codes"
    __table_args__ = (
-        UniqueConstraint('event_id', 'code', name='uq_event_discount_code'),
+        UniqueConstraint(
+            'event_id', 'code', 'deleted_at', name='uq_event_discount_code_deleted_at'


Black would make changes.

iamareebjamal · 2020-06-30T17:40:21Z

app/models/discount_code.py

@@ -13,7 +13,9 @@
 class DiscountCode(SoftDeletionModel):
    __tablename__ = "discount_codes"
    __table_args__ = (
-        UniqueConstraint('event_id', 'code', name='uq_event_discount_code'),
+        UniqueConstraint(
+            'event_id', 'code', 'deleted_at', name='uq_event_discount_code_deleted_at'


Don't add deleted_at in constraint name, it's an implementation detail

Remove from all?

houndci-bot · 2020-06-30T18:28:58Z

app/models/discount_code.py

@@ -13,7 +13,7 @@
 class DiscountCode(SoftDeletionModel):
    __tablename__ = "discount_codes"
    __table_args__ = (
-        UniqueConstraint('event_id', 'code', name='uq_event_discount_code'),
+        UniqueConstraint('event_id', 'code', 'deleted_at', name='uq_event_discount_code'),


Black would make changes.

…server into issue-7093

iamareebjamal · 2020-07-01T15:59:51Z

Please fix the tests

iamareebjamal · 2020-07-01T16:00:30Z

Also, for all the models where deleted_at was removed, you need to delete the items with non null deleted_at before removing the column

…server into issue-7093

Haider8 · 2020-07-02T15:13:14Z

@iamareebjamal There are 4 apib dredd fails like in GET /v1/events/1/custom-form. So, should I change that now?

iamareebjamal · 2020-07-02T15:27:16Z

Yes

codecov · 2020-07-02T16:06:33Z

Codecov Report

Merging #7098 into development will decrease coverage by 0.52%.
The diff coverage is 85.71%.

@@               Coverage Diff               @@
##           development    #7098      +/-   ##
===============================================
- Coverage        62.51%   61.99%   -0.53%     
===============================================
  Files              262      262              
  Lines            13006    12996      -10     
===============================================
- Hits              8131     8057      -74     
- Misses            4875     4939      +64

Impacted Files	Coverage Δ
app/api/custom/orders.py	`36.99% <ø> (ø)`
app/api/event_copy.py	`28.57% <0.00%> (ø)`
app/api/helpers/custom_forms.py	`100.00% <ø> (ø)`
app/models/discount_code.py	`94.00% <ø> (ø)`
app/models/role_invite.py	`85.18% <ø> (ø)`
app/api/schema/custom_forms.py	`100.00% <100.00%> (ø)`
app/api/schema/event_sub_topics.py	`100.00% <100.00%> (ø)`
app/models/custom_form.py	`97.91% <100.00%> (-0.05%)`	⬇️
app/models/event_sub_topic.py	`94.11% <100.00%> (-0.33%)`	⬇️
app/api/helpers/speaker.py	`50.00% <0.00%> (-40.00%)`	⬇️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e2e038c...2787ed9. Read the comment docs.

iamareebjamal · 2020-07-02T16:45:31Z

Also, for all the models where deleted_at was removed, you need to delete the items with non null deleted_at before removing the column

.

Haider8 · 2020-07-02T18:14:21Z

@iamareebjamal Done

iamareebjamal · 2020-07-02T19:28:59Z

migrations/versions/rev-2020-06-30-20:07:55-f177c14710f1_.py

@@ -18,6 +18,9 @@

 def upgrade():
    # ### commands auto generated by Alembic - please adjust! ###
+    op.execute('DELETE FROM custom_forms WHERE deleted_at IS NOT NULL')
+    op.execute('DELETE FROM event_sub_topics WHERE deleted_at IS NOT NULL')


~~Add equivalent statements in downgrade as well~~

Sorry, you can't

What will be the equivalent?

iamareebjamal · 2020-07-02T19:29:48Z

Will merge after manual testing

iamareebjamal · 2020-07-02T19:31:53Z

app/models/ticket_fee.py

 from app.models import db

 DEFAULT_FEE = 0.0


-class TicketFees(db.Model):
+class TicketFees(SoftDeletionModel):


Don't add more soft deletion

Removed 👍

iamareebjamal · 2020-07-02T20:30:43Z

app/models/permission.py


-class Permission(db.Model):
+
+class Permission(SoftDeletionModel):


Here as well. Don't add more soft deletion. It has already complicated matters a lot

Haider8 added 3 commits June 30, 2020 20:29

Remove soft deletion for custom_forms and event_sub_topics

faa1da0

fix deepsource issue unused import

bb4c74c

Add soft deletion and deleted_at unique constraint

93c649c

houndci-bot reviewed Jun 30, 2020

View reviewed changes

iamareebjamal reviewed Jun 30, 2020

View reviewed changes

Remove deleted_at from constraint name

c120d2a

houndci-bot reviewed Jun 30, 2020

View reviewed changes

Merge branch 'development' of https://github.com/fossasia/open-event-…

8d22f7e

…server into issue-7093

Haider8 added 4 commits July 2, 2020 14:06

Merge branch 'development' of https://github.com/fossasia/open-event-…

01d308a

…server into issue-7093

Remove deleted_at from custom_form query filter

08f153e

Remove soft deleted schema and deleted_at from query

88bb9c4

Remove soft deletion schema for event sub-topic

aee231c

fix dredd api tests

4393a56

Add row delete query in alembic upgrade method

85cc1ad

iamareebjamal changed the title ~~enh: Add unique constraints and remove soft deletion from models~~ feat: Add unique constraints and remove soft deletion from models Jul 2, 2020

auto-label bot added the feature label Jul 2, 2020

iamareebjamal changed the title ~~feat: Add unique constraints and remove soft deletion from models~~ feat: Add proper unique constraints and remove soft deletion from models Jul 2, 2020

iamareebjamal changed the title ~~feat: Add proper unique constraints and remove soft deletion from models~~ fix: Add proper unique constraints and remove soft deletion from models Jul 2, 2020

auto-label bot added fix and removed feature labels Jul 2, 2020

iamareebjamal reviewed Jul 2, 2020

View reviewed changes

iamareebjamal approved these changes Jul 2, 2020

View reviewed changes

iamareebjamal requested changes Jul 2, 2020

View reviewed changes

Remove soft deletion from TicketFees model

6eb0aff

iamareebjamal reviewed Jul 2, 2020

View reviewed changes

Haider8 and others added 2 commits July 3, 2020 02:10

Remove soft deletion from Permission model

1574bbc

Merge branch 'development' into issue-7093

2787ed9

iamareebjamal approved these changes Jul 4, 2020

View reviewed changes

iamareebjamal merged commit ee0bd29 into fossasia:development Jul 4, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Add proper unique constraints and remove soft deletion from models #7098

fix: Add proper unique constraints and remove soft deletion from models #7098

Haider8 commented Jun 30, 2020

houndci-bot Jun 30, 2020

iamareebjamal Jun 30, 2020

Haider8 Jun 30, 2020

iamareebjamal Jun 30, 2020

houndci-bot Jun 30, 2020

iamareebjamal commented Jul 1, 2020

iamareebjamal commented Jul 1, 2020

Haider8 commented Jul 2, 2020

iamareebjamal commented Jul 2, 2020

codecov bot commented Jul 2, 2020 •

edited

Loading

iamareebjamal commented Jul 2, 2020

Haider8 commented Jul 2, 2020

iamareebjamal Jul 2, 2020 •

edited

Loading

Haider8 Jul 2, 2020

iamareebjamal commented Jul 2, 2020

iamareebjamal Jul 2, 2020

Haider8 Jul 2, 2020

iamareebjamal Jul 2, 2020

Haider8 Jul 2, 2020


		class Permission(db.Model):

		class Permission(SoftDeletionModel):

fix: Add proper unique constraints and remove soft deletion from models #7098

fix: Add proper unique constraints and remove soft deletion from models #7098

Conversation

Haider8 commented Jun 30, 2020

Short description of what this resolves:

Checklist

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

iamareebjamal commented Jul 1, 2020

iamareebjamal commented Jul 1, 2020

Haider8 commented Jul 2, 2020

iamareebjamal commented Jul 2, 2020

codecov bot commented Jul 2, 2020 • edited Loading

Codecov Report

iamareebjamal commented Jul 2, 2020

Haider8 commented Jul 2, 2020

iamareebjamal Jul 2, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iamareebjamal commented Jul 2, 2020

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

codecov bot commented Jul 2, 2020 •

edited

Loading

iamareebjamal Jul 2, 2020 •

edited

Loading