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

Remove gsheet-based summing for state promote #31056

Merged
merged 3 commits into from Oct 2, 2019

Conversation

breville
Copy link
Member

@breville breville commented Oct 2, 2019

Long ago we apparently took advantage of an extra row in the cdo-state-promote gsheet that was labeled with "Sum_states" rather than a state itself. This has since caused an issue in an iterator when we attempted to generate a PDF for a state of "Sum_states".

With this change, we just do the summing in the SQL query, rather than relying upon the sum generated by the gsheet. This will allow us to simultaneously remove the row from the gsheet.

Long ago (in #3591) we apparently took advantage of an extra row in the cdo-state-promote gsheet that was labeled with "Sum_states" rather than a state itself.  This caused an issue in the iterator at https://github.com/code-dot-org/code-dot-org/blob/staging/pegasus/rake/generate_pdfs.rake#L21 when we attempted to generate a PDF for a state of "Sum_states".

With this change, we just do the summing in the SQL query, rather than relying upon the sum generated by the gsheet.  This will allow us to simultaneously remove the row from the gsheet.
Copy link
Contributor

@islemaster islemaster left a comment

Choose a reason for hiding this comment

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

👍 Yay using our database instead!

Copy link

@uponthesun uponthesun left a comment

Choose a reason for hiding this comment

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

I assume the traffic to the page(s) that use this is low enough that we don't need to think about query performance here? LGTM if so.

@breville
Copy link
Member Author

breville commented Oct 2, 2019

@uponthesun Good question. Front-ends caches these pages locally with varnish and they are cached again by the CDN, so we're pretty good on performance, but it turns out test_pegasus_documents also does a nice job of testing that all pegasus page-rendering database queries are cached. I've made follow-up commits to resolve that.

@breville breville merged commit 14bd642 into staging Oct 2, 2019
@breville breville deleted the remove-gsheet-based-summing-for-state-promote branch October 2, 2019 23:00
@breville
Copy link
Member Author

breville commented Oct 2, 2019

Just merged this and also removed the final row from the gsheet simultaneously.

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.

None yet

4 participants