Fix #7004: Move jinja dependencies_html to webpack #7350

jameesjohn · 2019-08-10T12:37:59Z

Explanation

Fixes #7004: This PR removed dependencies html by sending them to the pages that need it via ajax (Exploration editor and exploration player).
Pages that load the question interactions (Review_tests, practice session) do not actually have any dependencies so they were just removed.
Skill editor and topic editor require just one dependency: codemirror and it is required via webpack to the pages.

UI-CodeMirror package has some known issues when it comes to minification and the package is no longer maintained, so I forked the repo, fixed the issue there and used it here instead.

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 "PROJECT: ..." label (Please add this label for the first-pass review of the PR).
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.

…ncies-html-removal

jameesjohn · 2019-08-10T12:43:37Z

Note: I am still testing topic editor and skill editor pages. However, I think this should start getting reviewed.

seanlip · 2019-08-10T21:06:35Z

Please note that most of the automated CI checks are failing.

vojtechjelinek

Overall looks good. If I understand this correctly we load most of the dependencies by AJAX except for the ones that are needed on angular module initialization.

core/templates/dev/head/pages/exploration-editor-page/exploration-editor-page.controller.ts

core/templates/dev/head/pages/creator-dashboard-page/creator-dashboard-page.scripts.ts

core/controllers/topic_editor.py

codecov · 2019-08-11T22:28:23Z

Codecov Report

Merging #7350 into develop will decrease coverage by 11.83%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##           develop    #7350       +/-   ##
============================================
- Coverage       83%   71.17%   -11.83%     
============================================
  Files         1064      836      -228     
  Lines        57131    33630    -23501     
  Branches      2660     2655        -5     
============================================
- Hits         47420    23936    -23484     
+ Misses        9067     9050       -17     
  Partials       644      644

Flag	Coverage Δ
#backend	`?`
#frontend	`71.17% <ø> (-0.3%)`	⬇️

Impacted Files	Coverage Δ
...d/domain/collection/collection-domain.constants.ts	`100% <0%> (ø)`	⬆️
.../head/pages/library-page/library-page.constants.ts	`100% <0%> (ø)`	⬆️
.../components/state-editor/state-editor.constants.ts	`100% <0%> (ø)`	⬆️
...main/topic_viewer/topic-viewer-domain.constants.ts	`100% <0%> (ø)`	⬆️
.../templates/dev/head/services/services.constants.ts	`100% <0%> (ø)`	⬆️
...s/interactions/interactions-extension.constants.ts	`100% <0%> (ø)`	⬆️
.../head/domain/question/question-domain.constants.ts	`100% <0%> (ø)`	⬆️
...s/story-editor-page/story-editor-page.constants.ts	`100% <0%> (ø)`	⬆️
...dashboard-page/creator-dashboard-page.constants.ts	`100% <0%> (ø)`	⬆️
...s/topic-editor-page/topic-editor-page.constants.ts	`100% <0%> (ø)`	⬆️
... and 242 more

codecov · 2019-08-11T22:28:23Z

Codecov Report

Merging #7350 into develop will decrease coverage by 0.02%.
The diff coverage is 100%.

@@             Coverage Diff             @@
##           develop    #7350      +/-   ##
===========================================
- Coverage    83.34%   83.32%   -0.02%     
===========================================
  Files         1089     1089              
  Lines        62879    62818      -61     
  Branches      3620     3619       -1     
===========================================
- Hits         52401    52340      -61     
- Misses        9323     9324       +1     
+ Partials      1155     1154       -1

Flag	Coverage Δ
#backend	`100% <ø> (ø)`	⬇️
#frontend	`73.05% <100%> (+0.01%)`	⬆️

Impacted Files	Coverage Δ
core/controllers/editor.py	`100% <ø> (ø)`	⬆️
core/controllers/base.py	`100% <ø> (ø)`	⬆️
.../directives/OppiaInteractiveLogicProofDirective.ts	`13.16% <ø> (+0.77%)`	⬆️
core/controllers/review_tests.py	`100% <ø> (ø)`	⬆️
core/controllers/skill_editor.py	`100% <ø> (ø)`	⬆️
core/controllers/reader.py	`100% <ø> (ø)`	⬆️
core/controllers/practice_sessions.py	`100% <ø> (ø)`	⬆️
core/controllers/topic_editor.py	`100% <ø> (ø)`	⬆️
core/controllers/creator_dashboard.py	`100% <ø> (ø)`	⬆️
...d-editors/schema-based-unicode-editor.directive.ts	`13.64% <100%> (+2.01%)`	⬆️
... and 15 more

oppiabot · 2019-08-12T07:52:27Z

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!

jameesjohn · 2019-08-12T15:46:02Z

If I understand this correctly we load most of the dependencies by AJAX except for the ones that are needed on angular module initialization.

Yes.

jameesjohn · 2019-08-25T15:16:46Z

Comments have been addressed. PTAL @seanlip.

oppiabot · 2019-08-25T15:22:42Z

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!

…ncies-html-removal

oppiabot · 2019-08-25T16:45:48Z

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!

seanlip

LGTM. Please fix merge conflicts.

…ncies-html-removal

jameesjohn · 2019-08-25T17:26:22Z

LGTM. Please fix merge conflicts.

Done

oppiabot · 2019-08-26T08:10:18Z

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!

…ncies-html-removal

seanlip · 2019-08-26T18:27:33Z

@Jamesjay4199 Lint tests fail, and I think this is introduced by your PR. Please check.

…ncies-html-removal

jameesjohn · 2019-08-27T06:17:57Z

@seanlip, I've fixed the lint issue.

seanlip · 2019-08-27T06:50:10Z

Sounds good, I'll merge, thanks.

Also, @lilithxxx -- FYI, I'm really starting to doubt codecov's reliability as a check. See this screenshot:

jameesjohn added 5 commits August 4, 2019 22:49

remove dependencies_html from review_tests and practice_session

24a1bbe

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

c7b8d6f

…ncies-html-removal

remove dependencies_html from topic editor and skill editor

5fa9bf0

removed dependencies_html from remaining pages

50afee5

add file overview for new files

2f669a6

jameesjohn added CHANGELOG: Code health labels Aug 10, 2019

jameesjohn requested review from aks681, DubeySandeep, nithusha21, seanlip, sophiewu6, vinitamurthi and vojtechjelinek as code owners August 10, 2019 12:38

reorder require block

e58bddd

jameesjohn assigned vojtechjelinek and jameesjohn Aug 10, 2019

work on failing fe test

f73b89f

jameesjohn requested a review from kevinlee12 as a code owner August 10, 2019 12:58

vojtechjelinek suggested changes Aug 11, 2019

View reviewed changes

work on failing BE test

1bb0dc8

fix lint issues

da7b5c0

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

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

jameesjohn changed the title ~~Remove jinja dependencies_html~~ Fix #7004: Move jinja dependencies_html to webpack Aug 25, 2019

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

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

9c1f0cf

…ncies-html-removal

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

jameesjohn requested a review from seanlip August 25, 2019 15:57

jameesjohn assigned seanlip and unassigned jameesjohn Aug 25, 2019

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

seanlip approved these changes Aug 25, 2019

View reviewed changes

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

cace26c

…ncies-html-removal

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

seanlip added the PR: LGTM label Aug 25, 2019

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

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

76e2f10

…ncies-html-removal

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

fix lint issue

5834593

jameesjohn added 2 commits August 26, 2019 19:54

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

218069f

…ncies-html-removal

fix lint issue

c2436e8

seanlip merged commit 9f127ab into oppia:develop Aug 27, 2019

jameesjohn mentioned this pull request Aug 29, 2019

Error: [$injector:unpr] issue #7460

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #7004: Move jinja dependencies_html to webpack #7350

Fix #7004: Move jinja dependencies_html to webpack #7350

jameesjohn commented Aug 10, 2019 •

edited

Loading

jameesjohn commented Aug 10, 2019

seanlip commented Aug 10, 2019

vojtechjelinek left a comment

codecov bot commented Aug 11, 2019

codecov bot commented Aug 11, 2019 •

edited

Loading

oppiabot bot commented Aug 12, 2019

jameesjohn commented Aug 12, 2019

jameesjohn commented Aug 25, 2019

oppiabot bot commented Aug 25, 2019

oppiabot bot commented Aug 25, 2019

seanlip left a comment

jameesjohn commented Aug 25, 2019

oppiabot bot commented Aug 26, 2019

seanlip commented Aug 26, 2019

jameesjohn commented Aug 27, 2019

seanlip commented Aug 27, 2019

Fix #7004: Move jinja dependencies_html to webpack #7350

Fix #7004: Move jinja dependencies_html to webpack #7350

Conversation

jameesjohn commented Aug 10, 2019 • edited Loading

Explanation

Checklist

jameesjohn commented Aug 10, 2019

seanlip commented Aug 10, 2019

vojtechjelinek left a comment

Choose a reason for hiding this comment

codecov bot commented Aug 11, 2019

Codecov Report

codecov bot commented Aug 11, 2019 • edited Loading

Codecov Report

oppiabot bot commented Aug 12, 2019

jameesjohn commented Aug 12, 2019

jameesjohn commented Aug 25, 2019

oppiabot bot commented Aug 25, 2019

oppiabot bot commented Aug 25, 2019

seanlip left a comment

Choose a reason for hiding this comment

jameesjohn commented Aug 25, 2019

oppiabot bot commented Aug 26, 2019

seanlip commented Aug 26, 2019

jameesjohn commented Aug 27, 2019

seanlip commented Aug 27, 2019

jameesjohn commented Aug 10, 2019 •

edited

Loading

codecov bot commented Aug 11, 2019 •

edited

Loading