Serve static pages from app.yaml #6974

jameesjohn · 2019-06-19T09:54:06Z

Explanation

Following the work on the static pages, this PR creates a new render_static_template method to serve the jinja-less templates

Checklist

The PR title starts with "Fix #bugnum: ", followed by a short, clear summary of the changes. (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)
The PR explanation includes the words "Fixes #bugnum: ..." (or "Fixes part of #bugnum" if the PR only partially fixes an issue).
The linter/Karma presubmit checks have passed.
- These should run automatically, but if not, you can manually trigger them locally using python scripts/pre_commit_linter.py and bash scripts/run_frontend_tests.sh.
The PR is made from a branch that's not called "develop".
The PR has an appropriate "CHANGELOG: ..." label (If you are unsure of which label to add, ask the reviewers for guidance).
The PR follows the style guide.
The PR addresses the points mentioned in the codeowner checks for the files/folders changed. (See the codeowner's wiki page.)
The PR is assigned to an appropriate reviewer.
- If you're a new contributor, please ask on Gitter for someone to assign a reviewer and don't tick this checkbox.
- If you're not sure who the appropriate reviewer is, please assign to the issue's "owner" -- see the "talk-to" label on the issue. Do not only request the review but also add the reviewer as an assignee.

vojtechjelinek

Thanks! Two important comments!

core/controllers/base.py

codecov · 2019-06-19T17:54:57Z

Codecov Report

Merging #6974 into develop will decrease coverage by 0.02%.
The diff coverage is 6.66%.

@@             Coverage Diff             @@
##           develop    #6974      +/-   ##
===========================================
- Coverage    97.54%   97.52%   -0.03%     
===========================================
  Files          379      379              
  Lines        63366    63354      -12     
===========================================
- Hits         61809    61783      -26     
- Misses        1557     1571      +14

Impacted Files	Coverage Δ
main.py	`87.61% <ø> (ø)`	⬆️
core/controllers/base_test.py	`98.33% <ø> (ø)`	⬆️
core/controllers/pages.py	`100% <ø> (ø)`	⬆️
core/controllers/pages_test.py	`100% <ø> (ø)`	⬆️
scripts/build.py	`76.54% <6.66%> (-2.23%)`	⬇️

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 b8280c3...33c6f79. Read the comment docs.

oppiabot · 2019-06-21T10:50:26Z

Hi @Jamesjay4199. Due to recent changes in the "develop" branch, this PR now has a merge conflict. Please follow this link if you need help resolving the conflict, so that the PR can be merged. Thanks!

…serving4

vojtechjelinek

LGTM! Just one question.

core/controllers/base.py

seanlip

Just a couple of things, happy to approve after that.

core/controllers/base.py

seanlip · 2019-06-22T01:08:06Z

core/controllers/base.py

+
+        path = os.path.join(
+            feconf.FRONTEND_TEMPLATES_DIR, filepath)
+        self.response.write(template.render(path, None))


Just wondering ... what does template.render do? Does it have any unnecessary overhead? Is there a reference link you can point to that says "this is the preferred way to serve static files"?

I don't think this is "the preferred way" to serve static files, but this is what was in the documentation and I expect that since it comes bundled with webapp, then it should have been optimized for the purpose.
https://webapp2.readthedocs.io/en/latest/tutorials/gettingstarted/templates.html

template.render is similar to rendering with jinja, but it uses Django templating instead, so I'm not sure it's what you want to do here if your aim is to serve the files statically.

If you do want to serve a file statically, then typically that is done in app.yaml -- see https://cloud.google.com/appengine/docs/standard/python/config/appref

(You might also want to try the three ways -- jinja render, django template.render, and app.yaml static rendering -- and compare their times.)

app.yaml - 42ms
Jinja render - 147ms
template.render - 119ms

So we should go with app.yaml, but how about the headers?

seanlip · 2019-06-23T16:38:22Z

You can specify http_headers in app.yaml, look at the documentation.

…

On Sunday, June 23, 2019, James James ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In core/controllers/base.py <#6974 (comment)>: > + def render_static_template(self, filepath, iframe_restriction='DENY'): + """Prepares a Jinja-less HTML response to be sent to the client. + Args: + filepath: str. The template filepath. + iframe_restriction: str or None. Possible values are + 'DENY' and 'SAMEORIGIN': + + DENY: Strictly prevents the template to load in an iframe. + SAMEORIGIN: The template can only be displayed in a frame + on the same origin as the page itself. + """ + self.prepare_headers(iframe_restriction) + + path = os.path.join( + feconf.FRONTEND_TEMPLATES_DIR, filepath) + self.response.write(template.render(path, None)) app.yaml - 42ms Jinja render - 147ms template.render - 119ms So we should go with app.yaml, but how about the headers? — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#6974?email_source=notifications&email_token=ACQV5SR23YTWSKHGZLUBVB3P35UH3A5CNFSM4HZH3WT2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOB4LHMVY#discussion_r296479115>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACQV5SQGMKKDUHASAV3TBVDP35UH3ANCNFSM4HZH3WTQ> .

jameesjohn · 2019-06-23T21:34:19Z

@seanlip, while implementing this, I noticed that it wouldn't work well for the other variants of splash page.
and also the X-Frame-Options header needs some script before getting set

seanlip · 2019-06-23T21:36:28Z

Can you be more descriptive about what "wouldn't work well" means? Otherwise it is hard to offer advice.

For X-Frame-Options, I think it's usually DENY except for the exploration pages. Is that correct? If so you can just go with that...

jameesjohn · 2019-06-23T21:39:29Z

Other splash pages like splash_at0 and splash_at1
they are rendered based on the value of c from the url

jameesjohn · 2019-06-23T21:44:38Z

I'll just render them individually

seanlip · 2019-06-23T21:55:49Z

Yep, sg.

seanlip

Hi @Jamesjay4199, are there e2e tests for each of these pages? If so, could you please point me to them? I think we should add a note in build.py or app.yaml to say that each of these pages needs to be tested in an e2e test, too (in case future devs add stuff).

scripts/build.py

…serving4

…pages

jameesjohn · 2019-07-03T13:17:00Z

core/tests/protractor_desktop/navigation.js

@@ -93,6 +93,27 @@ describe('Oppia static pages tour', function() {
    waitFor.pageToFullyLoad();
  });

+  it('visits the Get started page', function() {


@seanlip, the tests are here.

Can you assert for something that should exist in the page (to make sure it's not serving a 404)?

seanlip · 2019-07-03T18:40:27Z

scripts/start.sh

@@ -89,10 +89,12 @@ if [[ "$FORCE_PROD_MODE" == "True" ]]; then
  constants_env_variable="\"DEV_MODE\": false"
  sed -i.bak -e s/"\"DEV_MODE\": .*"/"$constants_env_variable"/ assets/constants.js
  $PYTHON_CMD scripts/build.py --prod_env --enable_watcher
+  APP_YAML="app.yaml"


APP_YAML_FILEPATH, here and elsewhere

seanlip · 2019-07-03T18:41:50Z

core/tests/protractor_desktop/navigation.js

@@ -93,6 +93,27 @@ describe('Oppia static pages tour', function() {
    waitFor.pageToFullyLoad();
  });

+  it('visits the Get started page', function() {


Can you assert for something that should exist in the page (to make sure it's not serving a 404)?

seanlip · 2019-07-03T18:42:12Z

scripts/build.py

@@ -111,7 +111,12 @@
    'third_party/generated/js/third_party.min.js.map',
    'third_party/generated/webfonts/*',
    '*.bundle.js',
-    '*.bundle.js.map')
+    '*.bundle.js.map',
+    '*/dist/get-started-page.mainpage.html',


Update the comment to explain why these files are present.

seanlip · 2019-07-03T19:02:46Z

app.yaml

@@ -2,9 +2,6 @@ runtime: python27
 api_version: 1


Per our discussion, let's just have one app.yaml file in the codebase so that there's a single source of truth.

@vojtechjelinek FYI we discussed having a template file from which to generate dev/prod, but then we figured that it would be better to just have the "dev" version in the codebase (as we do now) and then do the changes needed to transform it to the "prod" version (like we do with other files/folders). This means that we only have one indirectly-generated file rather than two (which is better, since indirect pipelines like this often lead to errors).

@seanlip, should this file still be in the codebase?

No, I don't think so.

jameesjohn

@seanlip PTAL. Thanks

seanlip

Changes look good to me. I think just removing the app.yaml file suffices. Tganks!

jameesjohn · 2019-07-04T21:57:53Z

Done

seanlip · 2019-07-05T01:03:47Z

@Jamesjay4199 Don't forget to revert changes in package-lock.json if they are not intentional.

@apb7 PTAL as codeowner, thanks!

seanlip · 2019-07-05T01:04:04Z

Also @Jamesjay4199 please note that the lint and an e2e test is failing.

apb7

LGTM from code owner's perspective. Thanks!

…serving4

jameesjohn · 2019-07-06T22:20:21Z

@seanlip, I think this is ready to be merged

seanlip

Yep, looks good. Thanks!

create render template method

0bc53bb

jameesjohn added the CHANGELOG: Updates to static pages label Jun 19, 2019

jameesjohn requested a review from vojtechjelinek June 19, 2019 09:54

jameesjohn requested a review from seanlip as a code owner June 19, 2019 09:54

vojtechjelinek suggested changes Jun 19, 2019

View reviewed changes

core/controllers/base.py Outdated Show resolved Hide resolved

core/controllers/base.py Outdated Show resolved Hide resolved

vojtechjelinek assigned jameesjohn Jun 19, 2019

address review comments

df266a2

jameesjohn requested a review from vojtechjelinek June 21, 2019 10:43

jameesjohn removed their assignment Jun 21, 2019

oppiabot bot added the PR: don't merge - HAS MERGE CONFLICTS label Jun 21, 2019

Merge branch 'develop' of https://github.com/oppia/oppia into static-…

8b63d34

…serving4

vojtechjelinek approved these changes Jun 21, 2019

View reviewed changes

core/controllers/base.py Outdated Show resolved Hide resolved

address review comments

9106197

oppiabot bot removed the PR: don't merge - HAS MERGE CONFLICTS label Jun 21, 2019

fix error due to merge

6d2cdb9

jameesjohn assigned seanlip Jun 21, 2019

seanlip requested changes Jun 22, 2019

View reviewed changes

core/controllers/base.py Outdated Show resolved Hide resolved

core/controllers/base.py Outdated Show resolved Hide resolved

seanlip reviewed Jun 22, 2019

View reviewed changes

address review comments

77fc41d

seanlip assigned jameesjohn and unassigned seanlip Jun 23, 2019

serve statics files from app.yaml

e3f24b7

jameesjohn assigned aks681 and unassigned vojtechjelinek Jun 30, 2019

seanlip requested changes Jun 30, 2019

View reviewed changes

scripts/build.py Outdated Show resolved Hide resolved

scripts/build.py Outdated Show resolved Hide resolved

jameesjohn added 3 commits July 2, 2019 14:50

Merge branch 'develop' of https://github.com/oppia/oppia into static-…

a209b4e

…serving4

create dev app.yaml and prod app.yaml as well as write tests for the …

7ccbf19

…pages

address review comments and revert package-lock.json

04bd338

jameesjohn commented Jul 3, 2019

View reviewed changes

seanlip requested changes Jul 3, 2019

View reviewed changes

seanlip assigned apb7 and unassigned aks681 Jul 4, 2019

jameesjohn added 2 commits July 4, 2019 22:11

address review comments

f27d23b

revert change in constants

c6617b5

jameesjohn commented Jul 4, 2019

View reviewed changes

seanlip reviewed Jul 4, 2019

View reviewed changes

remove app.yaml

81922b9

seanlip approved these changes Jul 5, 2019

View reviewed changes

seanlip assigned jameesjohn Jul 5, 2019

apb7 approved these changes Jul 5, 2019

View reviewed changes

apb7 removed their assignment Jul 5, 2019

jameesjohn added 5 commits July 5, 2019 13:31

work on failing e2e tests

9104bca

fix lint issues

abddd81

revert package-lock.json

33c6f79

Merge branch 'develop' of https://github.com/oppia/oppia into static-…

d99be7c

…serving4

remove other variants of splash page

bd7eca5

seanlip approved these changes Jul 6, 2019

View reviewed changes

seanlip merged commit 82d1217 into oppia:develop Jul 6, 2019

Serve static pages from app.yaml #6974

Serve static pages from app.yaml #6974

Conversation

jameesjohn commented Jun 19, 2019

Explanation

Checklist

vojtechjelinek left a comment

Choose a reason for hiding this comment

codecov bot commented Jun 19, 2019 • edited Loading

Codecov Report

oppiabot bot commented Jun 21, 2019

vojtechjelinek left a comment

Choose a reason for hiding this comment

seanlip left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanlip commented Jun 23, 2019 via email

jameesjohn commented Jun 23, 2019

seanlip commented Jun 23, 2019

jameesjohn commented Jun 23, 2019

jameesjohn commented Jun 23, 2019

seanlip commented Jun 23, 2019

seanlip left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jameesjohn left a comment

Choose a reason for hiding this comment

seanlip left a comment

Choose a reason for hiding this comment

jameesjohn commented Jul 4, 2019

seanlip commented Jul 5, 2019

seanlip commented Jul 5, 2019

apb7 left a comment

Choose a reason for hiding this comment

jameesjohn commented Jul 6, 2019

seanlip left a comment

Choose a reason for hiding this comment

codecov bot commented Jun 19, 2019 •

edited

Loading

seanlip left a comment •

edited

Loading