fix: Revert PR for adding required fields of ngo events #6009

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.

Sign up for GitHub

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

Jump to bottom

Merged

iamareebjamal merged 2 commits into fossasia:development from prateekj117:ngo-events

Jun 16, 2019

Member

prateekj117 commented Jun 7, 2019

Fixes #5991

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.

mrsaicharan1 previously approved these changes

View reviewed changes

codecov bot commented Jun 7, 2019 •

edited

Loading

Codecov Report

Merging #6009 into development will decrease coverage by <.01%.
The diff coverage is 100%.

@@               Coverage Diff               @@
##           development    #6009      +/-   ##
===============================================
- Coverage        66.16%   66.15%   -0.01%     
===============================================
  Files              285      285              
  Lines            14047    14044       -3     
===============================================
- Hits              9294     9291       -3     
  Misses            4753     4753

Impacted Files	Coverage Δ
app/api/schema/events.py	`89.93% <100%> (-0.07%)`	⬇️
app/models/event.py	`81.92% <100%> (-0.14%)`	⬇️

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 fd99ecf...f9bf833. Read the comment docs.

shreyanshdwivedi previously approved these changes

View reviewed changes

kushthedude previously approved these changes

View reviewed changes

iamareebjamal previously approved these changes

View reviewed changes

iamareebjamal changed the title ~~Revert PR for adding required fields of ngo events~~ fix: Revert PR for adding required fields of ngo events

auto-label bot added the fix label

prateekj117 dismissed stale reviews from iamareebjamal, kushthedude, shreyanshdwivedi, and mrsaicharan1 via

3de48d3

June 11, 2019 04:52

prateekj117 force-pushed the ngo-events branch from df59e50 to 3de48d3 Compare

June 11, 2019 04:52

mrsaicharan1 reviewed

View reviewed changes

migrations/versions/31a6cf1c9374_.py Outdated

+              # revision identifiers, used by Alembic.
+              revision = '31a6cf1c9374'
+              down_revision = '89a5b836b406'

Member

mrsaicharan1 Jun 12, 2019

Please update the migration file with the latest revision

Member Author

prateekj117 commented Jun 14, 2019

@iamareebjamal Please review.

Member

iamareebjamal commented Jun 14, 2019

#6009 (comment)

Member Author

prateekj117 commented Jun 14, 2019 •

edited

Loading

@iamareebjamal Instead of that, I pulled the latest changes of development into this branch and merged the migrations heads and checked locally, everything works fine.

Member

iamareebjamal commented Jun 14, 2019

Please keep a single migration


          fix: Revert PR for adding required fields of ngo events

0f68fc2

prateekj117 force-pushed the ngo-events branch from 17124cb to 0f68fc2 Compare

June 15, 2019 18:22

Member Author

prateekj117 commented Jun 15, 2019

@mrsaicharan1 @uds5501 @shreyanshdwivedi @iamareebjamal Please review.

iamareebjamal reviewed

View reviewed changes

migrations/versions/14e31b3a860b_.py Outdated

+                             existing_type=sa.BOOLEAN(),
+                             nullable=True,
+                             autoincrement=False,
+                             existing_server_default=sa.text('false'))

Member

iamareebjamal Jun 15, 2019

This should not be here

Member Author

prateekj117 Jun 15, 2019

@iamareebjamal This is a auto-generated migration.

Member

iamareebjamal Jun 15, 2019

Still

mrsaicharan1 suggested changes

View reviewed changes

migrations/versions/14e31b3a860b_.py Outdated

+              def upgrade():
+                  # ### commands auto generated by Alembic - please adjust! ###
+                  op.drop_column('events', 'is_ngo')
+                  op.alter_column('events_version', 'show_remaining_tickets',

Member

mrsaicharan1 Jun 16, 2019

This seems to be occurring due to some issue with migration commands. Please remove this as we are not making any changes to the remaining tickets column in events_version

Contributor

uds5501 Jun 16, 2019

@mrsaicharan1 is correct. This automatic script just saw that your events have a null show remaining tickets column.
That's why this line is being generated. so removing it should be fine.

migrations/versions/14e31b3a860b_.py Outdated

+              def downgrade():
+                  # ### commands auto generated by Alembic - please adjust! ###
+                  op.add_column('events_version', sa.Column('is_ngo', sa.BOOLEAN(), autoincrement=False, nullable=True))
+                  op.alter_column('events_version', 'show_remaining_tickets',

Member

mrsaicharan1 Jun 16, 2019

Same here. Remove this and this is good to go.
Also, just a pointer, whenever migration files are generated, there is a line which tells you that
# ### commands auto generated by Alembic - please adjust! ###

Contributor

uds5501 Jun 16, 2019

@prateekj117 Same goes with this block

uds5501 suggested changes

View reviewed changes

migrations/versions/14e31b3a860b_.py Outdated

+              def upgrade():
+                  # ### commands auto generated by Alembic - please adjust! ###
+                  op.drop_column('events', 'is_ngo')
+                  op.alter_column('events_version', 'show_remaining_tickets',

Contributor

uds5501 Jun 16, 2019

@mrsaicharan1 is correct. This automatic script just saw that your events have a null show remaining tickets column.
That's why this line is being generated. so removing it should be fine.

migrations/versions/14e31b3a860b_.py Outdated

+              def downgrade():
+                  # ### commands auto generated by Alembic - please adjust! ###
+                  op.add_column('events_version', sa.Column('is_ngo', sa.BOOLEAN(), autoincrement=False, nullable=True))
+                  op.alter_column('events_version', 'show_remaining_tickets',

Contributor

uds5501 Jun 16, 2019

@prateekj117 Same goes with this block


          Update migration file

f9bf833

Member Author

prateekj117 commented Jun 16, 2019

@mrsaicharan1 @uds5501 @iamareebjamal Made the changes. Please review.

uds5501 approved these changes

View reviewed changes

iamareebjamal approved these changes

View reviewed changes

iamareebjamal merged commit 0caf6a8 into fossasia:development

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

iamareebjamal iamareebjamal approved these changes

+4 more reviewers

mrsaicharan1 mrsaicharan1 requested changes

uds5501 uds5501 approved these changes

shreyanshdwivedi shreyanshdwivedi left review comments

kushthedude kushthedude left review comments

Labels

fix