Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update ticket_holder table schema #6675

Closed
wants to merge 10 commits into from

Conversation

ashwani99
Copy link
Contributor

Fixes #6669

Short description of what this resolves:

Updated the fields ticket_id and event_id to be not null

Changes proposed in this pull request:

  • Updated schema of ticket_holder table. The fields ticket_id and event_id have been added with not null contraint

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.

@@ -11,6 +12,7 @@ class Meta:
sqlalchemy_session = db.session

event = factory.RelatedFactory(EventFactoryBasic)
ticket = factory.RelatedFactory(TicketFactory)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without doing this, while running tests, some test were failing because of ticket_id being None

Copy link
Contributor

@codedsun codedsun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please check for CI Failing.

@vishakhanihore
Copy link

Migration is needed for this.

Copy link
Contributor

@codedsun codedsun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your dredd test are failing, also do create the migration

@codecov
Copy link

codecov bot commented Dec 19, 2019

Codecov Report

❗ No coverage uploaded for pull request base (development@537a263). Click here to learn what that means.
The diff coverage is 95.94%.

Impacted file tree graph

@@              Coverage Diff               @@
##             development    #6675   +/-   ##
==============================================
  Coverage               ?   65.61%           
==============================================
  Files                  ?      300           
  Lines                  ?    15256           
  Branches               ?        0           
==============================================
  Hits                   ?    10010           
  Misses                 ?     5246           
  Partials               ?        0
Impacted Files Coverage Δ
app/views/blueprints.py 58.2% <ø> (ø)
app/models/event.py 78.7% <ø> (ø)
app/api/schema/events.py 91.33% <ø> (ø)
app/api/routes.py 100% <ø> (ø)
app/api/schema/attendees.py 98.21% <ø> (ø)
app/api/attendees.py 40% <ø> (ø)
tests/all/integration/test_migrations.py 100% <100%> (ø)
tests/all/integration/api/helpers/test_errors.py 92.85% <100%> (ø)
tests/all/integration/api/helpers/test_auth.py 97.22% <100%> (ø)
...all/integration/api/helpers/test_scheduled_jobs.py 100% <100%> (ø)
... and 46 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 537a263...642cbab. Read the comment docs.

Copy link
Member

@kushthedude kushthedude left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please follow Semantic PR Naming for your PR title.

@@ -21,4 +21,5 @@ class Meta:
is_checked_in = True
pdf_url = common.url_
event_id = 1
# ticket_id = 1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove comment

order_id: int = db.Column(db.Integer, db.ForeignKey('orders.id', ondelete='CASCADE'))
is_checked_in: bool = db.Column(db.Boolean, default=False)
is_checked_out: bool = db.Column(db.Boolean, default=False)
device_name_checkin: str = db.Column(db.String)
checkin_times: str = db.Column(db.String)
checkout_times: str = db.Column(db.String)
attendee_notes: str = db.Column(db.String)
event_id: int = db.Column(db.Integer, db.ForeignKey('events.id', ondelete='CASCADE'))
event_id: int = db.Column(db.Integer, db.ForeignKey('events.id', ondelete='CASCADE'), nullable=False)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add migration files for the following.

@ashwani99 ashwani99 force-pushed the fix-6665 branch 2 times, most recently from 5aeac31 to 9028721 Compare December 22, 2019 16:18
@@ -73,7 +73,8 @@ Create a new attendee.
"country": "IN",
"is-checked-in": "false",
"attendee-notes": "example",
"pdf-url": "http://example.com"
"pdf-url": "http://example.com",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Revert

@@ -73,7 +73,8 @@ Create a new attendee.
"country": "IN",
"is-checked-in": "false",
"attendee-notes": "example",
"pdf-url": "http://example.com"
"pdf-url": "http://example.com",
"ticket-id": "1"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

revert

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kushthedude If you explain the problem it would be very helpful

@ashwani99 ashwani99 force-pushed the fix-6665 branch 5 times, most recently from e5012a1 to ef88094 Compare December 29, 2019 20:37
@@ -1858,7 +1858,7 @@ def attendee_post(transaction):
ticket = TicketFactory()
db.session.add(ticket)

attendee = AttendeeFactory(ticket_id=1)
attendee = AttendeeFactory(ticket_id=1, event_id=1)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's the default value. Not needed

"pdf-url": "http://example.com"
"pdf-url": "http://example.com",
"ticket-id": "1",
"event-id": "1"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unneeded

@@ -50,7 +50,6 @@ def validate_json(self, data, original_data):
gender = fields.Str(allow_none=True)
birth_date = fields.DateTime(allow_none=True)

ticket_id = fields.Str(allow_none=True)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Undo this and run the tests.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@iamareebjamal I think the tests are failing due to this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have tried it. The issue still persists.

@@ -37,15 +37,15 @@ class TicketHolder(SoftDeletionModel):
gender: str = db.Column(db.String)
birth_date: datetime = db.Column(db.DateTime(timezone=True))
pdf_url: str = db.Column(db.String)
ticket_id: int = db.Column(db.Integer, db.ForeignKey('tickets.id', ondelete='CASCADE'))
ticket_id: int = db.Column(db.Integer, db.ForeignKey('tickets.id', ondelete='CASCADE'), nullable=False)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Black would make changes.


# populate event id and ticket id
data['ticket_id'] = ticket.id
data['event_id'] = ticket.event_id
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not needed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @iamareebjamal I am trying to debug what is the exact reason of the ticket_id and event_id becoming null after the object creation. That's why I tried passing them in the data payload. Any you help me find out what can be the exact reason? Am I going in the right direction as per tracing the problem?

@@ -93,7 +93,7 @@ class Meta:
self_view_kwargs={'id': '<id>'},
related_view='v1.ticket_detail',
related_view_kwargs={'attendee_id': '<id>'},
schema='TicketSchemaPublic',
schema='TicketSchema',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please stop hit and trial. This is not needed and wrong

@iamareebjamal
Copy link
Member

Stale

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make ticket_id and event_id not null in TicketHolder
7 participants