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

Issue 1628 #1690

Merged
merged 7 commits into from
Oct 5, 2017
Merged

Issue 1628 #1690

merged 7 commits into from
Oct 5, 2017

Conversation

teleyinex
Copy link
Member

No description provided.

The removed code was too heavy. In Crowdcrafting for example it has to
compute around 2500 SQL queries to analyze all the projects, which is
too much. As we've now specific endpoints with pagination via JSON the
pybossa theme could query them to show the data that anyone wants.

This should make PYBOSSA much faster, specially for the front page.
@teleyinex
Copy link
Member Author

@alexandermendes, @mialondon and @therealmarv I think this now fixes the issue #1628 but I've added a few extra improvements. Basically, I've removed from the home endpoint loading all the categories as well as the projects and their data. While for your project might be okay, in Crowdcrafting is generating around 2.5K SQL queries. We handle all of them via the background jobs, so the users do not see any impact, but when you don't the jobs running this is pretty bad (especially for first timers using PYBOSSA).

My aim with these changes is to use the new API endpoints that will allow us to do whatever we want as we can query anything: leaderboards, categories, etc.

Before merging I would love your feedback, as the homepage will be affected by this changes (not yours, as you're now running a full SPA).

@alexandermendes
Copy link
Contributor

Ok cool, so basically categories_projects will now be empty, apart from any featured projects?

We were taking advantage of this in a couple of places but that's fine - as you say we can use the other endpoints.

@teleyinex
Copy link
Member Author

That's why I wanted to ask first :-) I think we put a lot of login into the backend for this endpoint, and now that we have the other solution, we can built that logic in any way thanks to SPAs. If it's fine for you, we will work with it.

@teleyinex
Copy link
Member Author

The last commit just refactored more this endpoint. Please @alexandermendes take a look before we break anything for you.

@alexandermendes
Copy link
Contributor

Yeah so basically it's fine, I think we only have 3 references to this endpoint at the moment so not much to check/replace! Thanks for asking/warning us 😄

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 98.116% when pulling 56699c0 on issue-1628 into 4924dd0 on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 98.116% when pulling 56699c0 on issue-1628 into 4924dd0 on master.

@therealmarv
Copy link
Contributor

In my opinion this is a step forward to a more future proof PYBOSSA. Remove that old code!

Btw. do we mention breaking changes somewhere? We should for a few legacy users?!

@teleyinex
Copy link
Member Author

@therealmarv yes, we do via the version numbers. We can increase to version 2.8.0 and explain in the docs that this is the major change regarding how the frontend works. In any case, as we will be merging the changes in the default theme, everyone using it as their theme should update it as well.

@teleyinex
Copy link
Member Author

@alexandermendes I'll confirm with another group if this is working for them, and then merge it. We'll bump the version to 2.8.0

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 98.116% when pulling cea4e90 on issue-1628 into 4924dd0 on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 98.116% when pulling cea4e90 on issue-1628 into 4924dd0 on master.

@teleyinex teleyinex merged commit 77e7f75 into master Oct 5, 2017
@teleyinex teleyinex deleted the issue-1628 branch October 5, 2017 15:47
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.

5 participants