open-contracting / credere-backend

A tool that facilitates the participation of Micro, Small, and Medium businesses (MSMEs) in the Colombian public procurement market.
https://credere.readthedocs.io
BSD 3-Clause "New" or "Revised" License
5 stars 0 forks source link

fix: Fix N+1 query in /applications/admin-list #324

Closed jpmckinney closed 1 month ago

jpmckinney commented 1 month ago

fixes #323

coveralls commented 1 month ago

Pull Request Test Coverage Report for Build 10333799972

Details


Files with Coverage Reduction New Missed Lines %
app/sources/colombia.py 22 70.45%
<!-- Total: 22 -->
Totals Coverage Status
Change from base Build 10333456317: 0.02%
Covered Lines: 2039
Relevant Lines: 2359

💛 - Coveralls
yolile commented 1 month ago

@jpmckinney, I had to fix the sorting to make this work, but let me know if you have a better way of doing it. The sort fields are the ones available in the front end.

jpmckinney commented 1 month ago

I didn't see your commit/comment. I repushed a fix, that needs to be merged with https://github.com/open-contracting/credere-frontend/pull/116

jpmckinney commented 1 month ago

That said, using an enum for the possible sort orders (and then a dict like in your solution b09d4d015776734867bcfd724eea969314a59c19) is probably better.

yolile commented 1 month ago

Ah, that one was my commit indeed, not sure how it ended up outside a branch

jpmckinney commented 1 month ago

I now added Lender like in your commit and fixed support for the status order.

Why is created_at -> borrower_submitted_at in your commit?

"application.created_at": models.Application.borrower_submitted_at

Your commit is off-branch because I force-pushed before I saw your updates.

Edit: I think this getattr solution might be fine – one advantage is less code repetition with the frontend.

Edit2: Done for the day. I just came online because I saw the PR was failing :)

yolile commented 1 month ago

Edit: I think this getattr solution might be fine – one advantage is less code repetition with the frontend.

Ok, so it is fine to keep your current approach or should I restore mine? I'm fine either way

Why is created_at -> borrower_submitted_at in your commit?

Because in the front end, it says "submitted date" but I will change it in the front end instead

yolile commented 1 month ago

I've deployed the changes, but had to remove the BorrowerDocument join https://github.com/open-contracting/credere-backend/commit/621f4dce7b6c575c30562283a54304b5cf89c1bc as if an application has more than 1 document it returns duplicates (as the join will give you one result per document). I didn't have more than one document per application in my local database which is why I didn't notice the issue until deploying it in the servers.

jpmckinney commented 1 month ago

Oh right. Probably can restore the joinedload for def get_applications similar to https://github.com/open-contracting/credere-backend/commit/415ea27f0346e34d2371e81bdb9622daa4747667

jpmckinney commented 1 month ago

I've restored the joinedload to avoid N+1 queries (but not the join, which causes duplicates)