(Partially) Fix #2100: add cache slugs. #2208

gvishal · 2016-07-02T20:52:47Z

Fix #2100: add cache slugs for.

codecov-io · 2016-07-02T21:53:38Z

Current coverage is 48.00% (diff: 15.46%)

Merging #2208 into develop will increase coverage by <.01%

@@            develop      #2208   diff @@
==========================================
  Files           185        190     +5   
  Lines         15852      15914    +62   
  Methods        2720       2733    +13   
  Messages          0          0          
  Branches       2689       2693     +4   
==========================================
+ Hits           7609       7639    +30   
- Misses         8243       8275    +32   
  Partials          0          0

Powered by Codecov. Last update e8a2de2...3b83d27

seanlip · 2016-07-03T04:35:43Z

.gitignore

@@ -14,3 +14,7 @@ venv/
 .DS_Store
 .idea
 .vagrant/*
+
+# required for cache slugs


I think you can add more detail to this comment -- please give enough detail so that someone who is not familiar with all our discussions knows what's going on. It's OK for the comment to span multiple lines.

seanlip · 2016-07-03T04:55:50Z

Hi @gvishal, this is a good first pass; I've made some comments. Let me know if anything's unclear!

One question: did you run the backend tests at all before you committed -- or test this manually? The Travis checks all seem to be failing. I would strongly advise at least testing things manually before you submit a PR; it helps you fix accidental errors.

…e level html files and corresponding directives/controllers to use it, addressed review comments

gvishal · 2016-07-03T17:00:03Z

@seanlip those tests failed since cache_slug was defined outside get_cache_slug() and it was working fine locally AFAIR despite not marking it global, fixed that error now. PTAL!

maitbayev · 2016-07-04T13:50:02Z

.gitignore

+# Oppia uses cache slugs for various resources and we need separate resource
+# directories for dev and prod. Resource directories for prod are generated
+# via build.py and should not be committed to the repo.
+static/prod


Already there in build.py.

gvishal · 2016-07-09T06:59:38Z

@bbriggs @wxyxinyu I implemented a basic POC for the new approach. PTAL! Thanks!

Added methods to return static resource url. Modified app.yaml.

…d js files.

bbriggs · 2016-07-11T12:35:46Z

Still reviewing. Lots of changes to catch up on.

seanlip · 2016-07-21T09:25:52Z

gulpfile.js

 var generatedTargetDir = path.join(
  'third_party', 'generated',
-  isMinificationNeeded ? 'prod' : 'dev');
+  isMinificationNeeded ? 'prod' : cacheSlugDev);


I'm confused about this. This refers to the files generated in /third_party/generated. I don't see why anything needs to change here, nor what it has to do with the cache slug?

This is because we need to make it uniform to use cache slugs. eg. Right now we have urls like /third_party/generated/css/file.css else we'll have /third_party/generated/dev/css/file.css, which is a problem.

Oh, sorry -- I seem to have missed this comment, or read an older version of it.

Yep, sounds like there's a good reason for this. Can you add a (long) comment explaining in detail why we choose '' (to someone who's never heard of cache slugs)? That would make this more maintainable going forward.

Thanks!

seanlip · 2016-07-21T09:40:23Z

Hi Vishal, I did a fairly careful check of this, and I also ran it with --prod_env. It looks generally good, I have a bunch of comments which I've noted inline.

Two other issues:

You'll need to resolve the lint issues, as discussed on chat.
If you run with --prod_env and go to the splash page on chrome, you'll find that the buttons look weird -- they're pushed off to the left. I tracked the issue down to CSS minification -- it appears that calc(50% + 200px) is a valid CSS value but calc(50%+200px) (without the spaces) is not. Could you adjust the CSS minification to fix this?

Also, I noticed some other issues like missing images for interactions and gadgets. Please make sure you test your work by going to the various pages and making sure that things load properly -- don't depend on the reviewer to find them. I may well have missed others. If you want to be thorough, also look at the requests that are being made to the server (in the terminal), and make sure that they're going to the right places; make a note as well of the still-uncached ones, since this is what you'll need to fix in the next stage.

Thanks!

Also, /cc @BenHenning for info.

gvishal · 2016-07-21T13:48:22Z

@seanlip For the css issue we can do double math. See: yui/yuicompressor#59 (comment). Or, write a custom fn which modifies the minified css See: yui/yuicompressor#59 (comment)

seanlip · 2016-07-21T19:36:37Z

Hi @gvishal, replied to your comments!

Re the CSS issue, let's do double math for now. Could you please fix that in your PR, and also add a comment to reference the YUI thread in each of the places where you do double math? Thanks!

seanlip · 2016-07-21T19:37:29Z

gulpfile.js

 var generatedTargetDir = path.join(
  'third_party', 'generated',
-  isMinificationNeeded ? 'prod' : cacheSlugDev);
+  isMinificationNeeded ? 'prod' : '');


Why not keep as 'dev' (like it was before)? Otherwise you kind of end up with a weird dir structure: prod becomes a sibling of css, fonts, js.

Sorry, ignore. Wrote this before seeing your other comment.

seanlip · 2016-07-22T09:34:46Z

core/templates/dev/head/dashboard/dashboard.html

@@ -325,5 +325,6 @@ <h2 class="activity-title protractor-test-exp-summary-tile-title">
    {{ include_js_file('components/SharingLinksDirective.js') }}
    {{ include_js_file('dashboard/Dashboard.js') }}
    {{ include_js_file('domain/dashboard/DashboardBackendApiService.js') }}
+    {{ include_js_file('domain/utilities/UrlInterpolationService.js') }}


@gvishal I thought you were dropping this import from this and all other pages where it isn't needed?

seanlip · 2016-07-22T09:35:55Z

Just one comment, but otherwise this looks ready for merge. Two things:

(a) did you test the new CSS on prod, and does it look correct?
(b) did you figure out the lint issue?

Thanks!

gvishal · 2016-07-22T13:34:17Z

Css works fine.
It's the js files wherein the linter is failing. Log: http://pastebin.com/rJa1XqGj.

Update: The linter is successfully able to lint both py + js files, but gets stuck processing scripts/gulp-start-gae-devserver.js. Probably waiting for the processes to return. See: https://gist.github.com/gvishal/268f43b01646aefe816d68e25031cfd6

seanlip · 2016-07-22T18:10:47Z

Aha. You could set the linter to ignore the build/ directory? There's no
need to lint generated files.

As a note, in develop, there should only be around 244 files to lint.

On Friday, July 22, 2016, Vishal Gupta notifications@github.com wrote:

Css works fine.

Linter gets stuck processing files from 251 to 291. Log:
http://pastebin.com/rJa1XqGj

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#2208 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AKFeyoFPArcAfWrnBSehjXrahLw7NxBMks5qYMbagaJpZM4JDv96
.

gvishal · 2016-07-22T19:06:18Z

I was thinking about the same thing, but build/ wouldn't be generated on travis would it?

Also, there are 339 js files in my branch.

Here's travis logs for pylint on a separate PR: https://travis-ci.org/oppia/oppia/jobs/146661059

It's an issue with waiting for threads to return.

seanlip · 2016-07-22T19:57:49Z

Hm, yeah, I don't know why build/ is being generated on travis. Also good call, I was only referring to py files when I said 244.

Not sure why the js files are causing issues. Is it the same file each time? What if you run lint on only that file in isolation? (Not sure if it's the specific file, or the number of files, that's the problem.)

seanlip · 2016-07-23T07:29:53Z

LGTM. Let's hope the travis checks pass, and then we can merge this!

* Added cache slugs for images and modified all references to the images. * Added interpolation service to get static image url, modified non-page level html files and corresponding directives/controllers to use it, addressed review comments * Modified /script tags * Added cache slugs for i18n * Fixed failing test and added i18n/prod to git ignore. * Cache slugs new approach POC * Modified references for images, scripts and i18n. Added methods to return static resource url. Modified app.yaml. * Modified gulp file and references to oppia.css and third_party css and js files. * Added slugs to extensions * Fixed test for get_cache_slug * Modified build.py and deploy.py * Addressed review comments * Modify imports * Modified src to ng-src and other review comments fixed. * Modified build.py * Minor change * Removed feconf.CACHE_SLUG_DEV and refactored * Added random cache slug * Refactored code. * Modified references for extensions, added extensions to build process and refactored methods in UrlInterpolationService * Fixed bug * Addressed review comments. * Fix failing tests * Fixed bug. * Added tests for urlinterpolationservice. * Added interpolation for interactions and logic_proof.html * Addressed review comments * Addressed review comments * Removed unnecessary include from templates * Modified precommit linter script to fix endless waiting * Commented script and added assets/scripts to exclusions.

Added cache slugs for images and modified all references to the images.

126df37

seanlip reviewed Jul 3, 2016
View reviewed changes

Added interpolation service to get static image url, modified non-pag…

8125aa9

…e level html files and corresponding directives/controllers to use it, addressed review comments

Modified /script tags

e0f0ff3

gvishal changed the title ~~(Partially) Fix #2100: add cache slugs for images.~~ (Partially) Fix #2100: add cache slugs for images and scripts. Jul 3, 2016

Added cache slugs for i18n

7e5a68f

gvishal changed the title ~~(Partially) Fix #2100: add cache slugs for images and scripts.~~ (Partially) Fix #2100: add cache slugs. Jul 3, 2016

maitbayev reviewed Jul 4, 2016
View reviewed changes

Fixed failing test and added i18n/prod to git ignore.

5be7fdd

526avijitgupta added the PR: don't merge - IN REVIEW label Jul 5, 2016

Cache slugs new approach POC

d9e18a4

gvishal added 3 commits July 9, 2016 16:29

Modified references for images, scripts and i18n.

35bb24b

Added methods to return static resource url. Modified app.yaml.

Modified gulp file and references to oppia.css and third_party css an…

10df46d

…d js files.

Added slugs to extensions

77da86a

seanlip reviewed Jul 21, 2016
View reviewed changes

Addressed review comments

8cb8624

seanlip reviewed Jul 21, 2016
View reviewed changes

Addressed review comments

eeb9b5b

seanlip reviewed Jul 22, 2016
View reviewed changes

Removed unnecessary include from templates

74781cb

gvishal added 3 commits July 23, 2016 12:14

Merge branch 'develop' into implement-cache-slugs-2100

238c896

Modified precommit linter script to fix endless waiting

ad26ae9

Commented script and added assets/scripts to exclusions.

3b83d27

seanlip added PR: LGTM and removed PR: don't merge - IN REVIEW labels Jul 23, 2016

gvishal added PR: don't merge - IN REVIEW and removed PR: LGTM labels Jul 23, 2016

seanlip added PR: LGTM and removed PR: don't merge - IN REVIEW labels Jul 23, 2016

seanlip merged commit 3adb944 into oppia:develop Jul 23, 2016

gvishal mentioned this pull request Jul 23, 2016

Serving oppia.css #2206

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(Partially) Fix #2100: add cache slugs. #2208

(Partially) Fix #2100: add cache slugs. #2208

gvishal commented Jul 2, 2016 •

edited

Loading

codecov-io commented Jul 2, 2016 •

edited

Loading

seanlip Jul 3, 2016

gvishal Jul 3, 2016

seanlip commented Jul 3, 2016 •

edited

Loading

gvishal commented Jul 3, 2016

maitbayev Jul 4, 2016

gvishal Jul 8, 2016

gvishal commented Jul 9, 2016

bbriggs commented Jul 11, 2016

seanlip Jul 21, 2016

gvishal Jul 21, 2016 •

edited

Loading

seanlip Jul 21, 2016

gvishal Jul 22, 2016

seanlip commented Jul 21, 2016

gvishal commented Jul 21, 2016 •

edited

Loading

seanlip commented Jul 21, 2016

seanlip Jul 21, 2016 •

edited

Loading

seanlip Jul 21, 2016

gvishal Jul 22, 2016

seanlip Jul 22, 2016

gvishal Jul 22, 2016

seanlip commented Jul 22, 2016

gvishal commented Jul 22, 2016 •

edited

Loading

seanlip commented Jul 22, 2016

gvishal commented Jul 22, 2016 •

edited

Loading

seanlip commented Jul 22, 2016 •

edited

Loading

seanlip commented Jul 23, 2016

(Partially) Fix #2100: add cache slugs. #2208

(Partially) Fix #2100: add cache slugs. #2208

Conversation

gvishal commented Jul 2, 2016 • edited Loading

codecov-io commented Jul 2, 2016 • edited Loading

Current coverage is 48.00% (diff: 15.46%)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanlip commented Jul 3, 2016 • edited Loading

gvishal commented Jul 3, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gvishal commented Jul 9, 2016

bbriggs commented Jul 11, 2016

Choose a reason for hiding this comment

gvishal Jul 21, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanlip commented Jul 21, 2016

gvishal commented Jul 21, 2016 • edited Loading

seanlip commented Jul 21, 2016

seanlip Jul 21, 2016 • 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

seanlip commented Jul 22, 2016

gvishal commented Jul 22, 2016 • edited Loading

seanlip commented Jul 22, 2016

gvishal commented Jul 22, 2016 • edited Loading

seanlip commented Jul 22, 2016 • edited Loading

seanlip commented Jul 23, 2016

gvishal commented Jul 2, 2016 •

edited

Loading

codecov-io commented Jul 2, 2016 •

edited

Loading

seanlip commented Jul 3, 2016 •

edited

Loading

gvishal Jul 21, 2016 •

edited

Loading

gvishal commented Jul 21, 2016 •

edited

Loading

seanlip Jul 21, 2016 •

edited

Loading

gvishal commented Jul 22, 2016 •

edited

Loading

gvishal commented Jul 22, 2016 •

edited

Loading

seanlip commented Jul 22, 2016 •

edited

Loading