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

chore: update sqlchemy dependency #5902

Merged

Conversation

uds5501
Copy link
Contributor

@uds5501 uds5501 commented May 16, 2019

Fixes #5828

Updates the SQLAlchemy dependency to 1.3.0

@auto-label auto-label bot added the chore label May 16, 2019
@uds5501
Copy link
Contributor Author

uds5501 commented May 16, 2019

@iamareebjamal How do I resolve the lock conflicts?

@iamareebjamal
Copy link
Member

git pull

@@ -51,7 +51,7 @@ raven[flask]
healthcheck
elasticsearch-dsl
flask-redis
SQLAlchemy==1.1.15 # 1.2.0 doesn't work, check if any future versions work
SQLAlchemy>=1.3.0 # 1.2.0 doesn't work, check if any future versions work
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
SQLAlchemy>=1.3.0 # 1.2.0 doesn't work, check if any future versions work
SQLAlchemy>=1.3.0<2.0.0 # 1.2.0 doesn't work, check if any future versions work

@iamareebjamal
Copy link
Member

Let the tests run first. They'll most likely fail

@uds5501
Copy link
Contributor Author

uds5501 commented May 16, 2019

@iamareebjamal Okay few queries, I tried git pull upstream development, no result.

And I have to use pipenv install "SQLAlchemy>=1.3.0<2.0.0", correct?

@iamareebjamal
Copy link
Member

Yes, the command is correct.

About git pull upstream development. That seems odd. Pipfile was updated in latest PR, it should be pulled

@uds5501
Copy link
Contributor Author

uds5501 commented May 16, 2019

@iamareebjamal This is the shell display on git pull

There is no tracking information for the current branch.
Please specify which branch you want to merge with.
See git-pull(1) for details.

    git pull <remote> <branch>

If you wish to set tracking information for this branch you can do so with:

    git branch --set-upstream-to=<remote>/<branch> dependency-update-sqlaclehmy

@iamareebjamal
Copy link
Member

I meant git pull upstream development

@iamareebjamal
Copy link
Member

Now your PR has conflicts in Pipfile.lock

You should have let the tests run first and if they passed, fix the conflicts

@uds5501
Copy link
Contributor Author

uds5501 commented May 16, 2019

@iamareebjamal I will move back one commit and push again, that will be fine?

@uds5501 uds5501 force-pushed the dependency-update-sqlaclehmy branch from 69d5664 to 8eeca38 Compare May 16, 2019 21:55
@iamareebjamal
Copy link
Member

Build is failing. Please check why

@uds5501
Copy link
Contributor Author

uds5501 commented May 17, 2019

@iamareebjamal This is happening due to : https://docs.sqlalchemy.org/en/13/changelog/migration_13.html#key-behavioral-changes-orm

As for this particular query, in api/admin_sales/locations.py,

def sales_per_location_by_status(status):
    return db.session.query(
        Event.location_name.label('location'),
        func.sum(Order.amount).label(status + '_sales'),
        func.sum(OrderTicket.quantity).label(status + '_tickets')) \
                     .outerjoin(Order) \
                     .outerjoin(OrderTicket) \
                     .filter(Event.id == Order.event_id) \
                     .filter(Order.status == status) \
                     .group_by(Event.location_name, Order.status) \
                     .cte()

is the issue.

I can change .outerjoin(Order) to .outerjoin(Order, Order.event_id==Event.id) but what do I do about .outerjoin(OrderTicket)?
Shall I use .outerjoin(OrderTicket, OrderTicket.order_id==Order.id)?

@iamareebjamal
Copy link
Member

@uds5501 See the previous result and match it after upgrading

app/api/admin_sales/locations.py Outdated Show resolved Hide resolved
app/api/admin_sales/locations.py Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented May 17, 2019

Codecov Report

Merging #5902 into development will decrease coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@               Coverage Diff               @@
##           development    #5902      +/-   ##
===============================================
- Coverage        66.54%   66.53%   -0.01%     
===============================================
  Files              286      286              
  Lines            13919    13921       +2     
===============================================
  Hits              9262     9262              
- Misses            4657     4659       +2
Impacted Files Coverage Δ
app/api/admin_sales/locations.py 64.1% <ø> (ø) ⬆️
app/api/events.py 26.09% <0%> (-0.16%) ⬇️

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 ba637e3...be1fa36. Read the comment docs.

@uds5501
Copy link
Contributor Author

uds5501 commented May 17, 2019

@iamareebjamal have a look?

@iamareebjamal
Copy link
Member

Great. Please resolve conflicts and I will merge it. Also, remove the comment about SQLAlchemy in requirements.txt

@iamareebjamal iamareebjamal merged commit 7bc397e into fossasia:development May 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Depenendency Upgrades
3 participants