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

Make all non-required attributes nullable #3970

Merged
merged 2 commits into from Jul 6, 2017

Conversation

@enigmaeth
Copy link
Member

commented Jul 5, 2017

Fixes #3958

Checklist

  • I have read the Contribution & Best practices Guide and my PR follows them.
  • My branch is up-to-date with the Upstream nextgen branch.
  • The unit tests pass locally with my changes

Short description of what this resolves:

Allows all non-required attributes accept null in API requests

Changes proposed in this pull request:

  • Adds allow_none as per marshmallow fields specification.
nullable=True)
op.alter_column('event_copyrights', 'licence',
existing_type=sa.VARCHAR(),
nullable=True)

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Jul 5, 2017

continuation line under-indented for visual indent

existing_type=sa.VARCHAR(),
nullable=True)
op.alter_column('event_copyrights', 'licence',
existing_type=sa.VARCHAR(),

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Jul 5, 2017

continuation line under-indented for visual indent

nullable=True)
op.alter_column('sessions', 'title',
existing_type=sa.VARCHAR(),
nullable=True)

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Jul 5, 2017

continuation line under-indented for visual indent

existing_type=sa.VARCHAR(),
nullable=True)
op.alter_column('sessions', 'title',
existing_type=sa.VARCHAR(),

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Jul 5, 2017

continuation line under-indented for visual indent

nullable=True)
op.alter_column('social_links', 'link',
existing_type=sa.VARCHAR(),
nullable=True)

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Jul 5, 2017

continuation line under-indented for visual indent

existing_type=postgresql.TIMESTAMP(timezone=True),
nullable=True)
op.alter_column('tickets', 'name',
existing_type=sa.VARCHAR(),

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Jul 5, 2017

continuation line under-indented for visual indent

nullable=True)
op.alter_column('tickets', 'sales_ends_at',
existing_type=postgresql.TIMESTAMP(timezone=True),
nullable=True)

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Jul 5, 2017

continuation line under-indented for visual indent

existing_type=postgresql.TIMESTAMP(timezone=True),
nullable=True)
op.alter_column('tickets', 'sales_ends_at',
existing_type=postgresql.TIMESTAMP(timezone=True),

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Jul 5, 2017

continuation line under-indented for visual indent

nullable=True)
op.alter_column('tickets', 'sales_starts_at',
existing_type=postgresql.TIMESTAMP(timezone=True),
nullable=True)

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Jul 5, 2017

continuation line under-indented for visual indent

existing_type=sa.VARCHAR(),
nullable=True)
op.alter_column('tickets', 'sales_starts_at',
existing_type=postgresql.TIMESTAMP(timezone=True),

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Jul 5, 2017

continuation line under-indented for visual indent

@enigmaeth enigmaeth force-pushed the enigmaeth:nullable branch from 93a7635 to e3d93ab Jul 5, 2017

@niranjan94
Copy link
Member

left a comment

The fields you made to accept null in API should also be nullable in the database right ? @enigmaeth

@enigmaeth enigmaeth force-pushed the enigmaeth:nullable branch from e3d93ab to fde551e Jul 5, 2017


from alembic import op
import sqlalchemy as sa
import sqlalchemy_utils

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Jul 5, 2017

'sqlalchemy_utils' imported but unused

@codecov

This comment has been minimized.

Copy link

commented Jul 5, 2017

Codecov Report

Merging #3970 into nextgen will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff            @@
##           nextgen    #3970   +/-   ##
========================================
  Coverage    54.49%   54.49%           
========================================
  Files          136      136           
  Lines         8153     8153           
========================================
  Hits          4443     4443           
  Misses        3710     3710
Impacted Files Coverage Δ
app/models/speakers_call.py 62.96% <100%> (ø) ⬆️
app/api/speakers_calls.py 55.55% <100%> (ø) ⬆️
app/api/ticket_tags.py 46.15% <100%> (ø) ⬆️
app/api/discount_codes.py 55.55% <100%> (ø) ⬆️
app/api/roles.py 100% <100%> (ø) ⬆️
app/api/tax.py 64.38% <100%> (ø) ⬆️
app/api/events.py 48.73% <100%> (ø) ⬆️
app/models/event_copyright.py 62.06% <100%> (ø) ⬆️
app/models/ticket.py 52.38% <100%> (ø) ⬆️
app/models/session.py 96% <100%> (ø) ⬆️
... and 17 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 910d659...75e79dd. Read the comment docs.

@enigmaeth

This comment has been minimized.

Copy link
Member Author

commented Jul 5, 2017

@niranjan94 They are accepting null values by default( since nullable=False is not set).
The API requests gave error because flask-rest-jsonapi raises validation error on null values. To avoid which I have added allow_none=True. [marshmallow fields]

Please correct me if I misunderstood. 😄

@enigmaeth enigmaeth requested a review from SaptakS Jul 5, 2017

@enigmaeth

This comment has been minimized.

Copy link
Member Author

commented Jul 5, 2017

@niranjan94 @SaptakS Please review. Thanks!

@enigmaeth

This comment has been minimized.

Copy link
Member Author

commented Jul 5, 2017

@niranjan94 Please take a look at this also 😅 Thanks!!

@open-event-bot open-event-bot bot removed the needs-review label Jul 5, 2017

zipcode = fields.Str()
created_at = fields.DateTime()
identifier = fields.Str(allow_none=True)
amount = fields.Float(validate=lambda n: n >= 0, allow_none=True)

This comment has been minimized.

Copy link
@arp95

arp95 Jul 6, 2017

whats the point of having an identifier when we have an id for the event?

created_at = fields.DateTime(dump_only=True)
deleted_at = fields.DateTime(dump_only=True)
submitted_at = fields.DateTime()
submitted_at = fields.DateTime(allow_none=True)
is_mail_sent = fields.Boolean()
microlocation = Relationship(attribute='microlocation',
self_view='v1.session_microlocation',

This comment has been minimized.

Copy link
@arp95

arp95 Jul 6, 2017

cant there be some cases when the location for a session be null?
like an online session or something.

@shubham-padia shubham-padia merged commit be39143 into fossasia:nextgen Jul 6, 2017

4 checks passed

codecov/patch 100% of diff hit (target 54.49%)
Details
codecov/project 54.49% (+0%) compared to 910d659
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
hound 1 violation found.
@shubham-padia

This comment has been minimized.

Copy link
Member

commented Jul 6, 2017

@arp95 Please do not ask general doubts like why do event identifiers exist on the PRs 😄. Feel free to ask any query on the server gitter channel itself.

niranjan94 added a commit that referenced this pull request Jul 7, 2017

@enigmaeth enigmaeth deleted the enigmaeth:nullable branch Jul 10, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.