Fix #6962: Remove jinja interaction templates and integrate css file into webpack #7286

jameesjohn · 2019-08-03T20:30:35Z

Explanation

This PR removes the use of the jinja interaction template construct which currently holds css imports.
The css files are imported into their respective bundles via the webpack css-loader and style loader and hence fixes #6962

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.

…player

kevinlee12 · 2019-08-04T03:10:29Z

@Jamesjay4199, ptal at the frontend tests, they are failing.

codecov · 2019-08-04T12:51:50Z

Codecov Report

Merging #7286 into develop will increase coverage by 9.39%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           develop   #7286      +/-   ##
==========================================
+ Coverage     89.4%   98.8%   +9.39%     
==========================================
  Files         1208     381     -827     
  Lines        98111   64554   -33557     
  Branches      2637       0    -2637     
==========================================
- Hits         87719   63781   -23938     
+ Misses        9755     773    -8982     
+ Partials       637       0     -637

Flag	Coverage Δ
#backend	`?`
#frontend	`?`

Impacted Files	Coverage Δ
core/controllers/reader.py	`100% <ø> (ø)`	⬆️
core/controllers/practice_sessions.py	`100% <ø> (ø)`	⬆️
core/controllers/review_tests.py	`100% <ø> (ø)`	⬆️
core/controllers/creator_dashboard.py	`95.91% <ø> (+0.64%)`	⬆️
core/controllers/topic_editor.py	`100% <ø> (ø)`	⬆️
core/controllers/skill_editor.py	`100% <ø> (ø)`	⬆️
core/controllers/editor.py	`100% <ø> (ø)`	⬆️
core/domain/interaction_registry.py	`97.36% <0%> (-2.64%)`	⬇️
extensions/interactions/base.py	`97.75% <0%> (-2.25%)`	⬇️
main.py	`87.61% <0%> (-0.12%)`	⬇️
... and 853 more

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 fb601c5...723ffac. Read the comment docs.

codecov · 2019-08-04T12:51:50Z

Codecov Report

Merging #7286 into develop will decrease coverage by 0.01%.
The diff coverage is 100%.

@@             Coverage Diff             @@
##           develop    #7286      +/-   ##
===========================================
- Coverage    82.54%   82.53%   -0.01%     
===========================================
  Files         1042     1042              
  Lines        57560    57558       -2     
  Branches      2643     2643              
===========================================
- Hits         47511    47505       -6     
- Misses        9407     9411       +4     
  Partials       642      642

Flag	Coverage Δ
#backend	`98.41% <ø> (-0.01%)`	⬇️
#frontend	`71.07% <100%> (ø)`	⬆️

Impacted Files	Coverage Δ
core/controllers/reader.py	`100% <ø> (ø)`	⬆️
core/controllers/practice_sessions.py	`100% <ø> (ø)`	⬆️
core/controllers/review_tests.py	`100% <ø> (ø)`	⬆️
core/controllers/creator_dashboard.py	`100% <ø> (ø)`	⬆️
core/controllers/topic_editor.py	`100% <ø> (ø)`	⬆️
core/controllers/skill_editor.py	`100% <ø> (ø)`	⬆️
core/controllers/editor.py	`100% <ø> (ø)`	⬆️
...eractions/ItemSelectionInput/ItemSelectionInput.ts	`100% <100%> (ø)`	⬆️
...ns/interactions/MusicNotesInput/MusicNotesInput.ts	`100% <100%> (ø)`	⬆️
...actions/MultipleChoiceInput/MultipleChoiceInput.ts	`100% <100%> (ø)`	⬆️
... and 9 more

vojtechjelinek

Thanks! One comment.

webpack.dev.config.ts

…tion-template-removal

vinitamurthi

Looks good! Just want to confirm that practice sessions and review tests were manually tested as well?

jameesjohn · 2019-08-06T14:48:06Z

@Jamesjay4199 please confirm: you tested each interaction manually, including submitting answers to it in both the editor and player pages?

Yes @seanlip, I created an exploration with multiple choice and I played it without any change in style.

Looks good! Just want to confirm that practice sessions and review tests were manually tested as well?

@vinitamurthi, the interaction that got affected in review_test and practice_session is the multiple-choice input and I tested it while testing the exploration editor and player and it worked fine.

jameesjohn · 2019-08-06T20:41:04Z

PTAL @nithusha21, @aks681 as code owner. thanks

aks681 · 2019-08-07T16:58:34Z

@Jamesjay4199 Did you try creating 'questions' in the topic and skill editors?

jameesjohn · 2019-08-07T22:04:34Z

@Jamesjay4199 Did you try creating 'questions' in the topic and skill editors?

No I did not.
I will do this and give you a response asap.

nithusha21 · 2019-08-09T04:00:43Z

Yes @seanlip, I created an exploration with multiple choice and I played it without any change in style.

I don't think that is enough. You would need to manually try every single one of the interactions as you have touched all those files. (The easiest, in dev env, is probably to load all_interactions exploration, and view it in both explore/ and create/ views).

nithusha21

Approving as I am going to be out this weekend and don't want this to block you. Once you confirm testing with all the interactions, I am ok with this PR.

jameesjohn · 2019-08-09T10:33:20Z

The easiest, in dev env, is probably to load all_interactions exploration, and view it in both explore/ and create/ views

@nithusha21, I tried loading all_interactions exploration, but I had an error: Full proto too large to save, cleared variables.
Other explorations loaded successfully and I tested them and they worked as expected.

jameesjohn · 2019-08-09T11:42:13Z

@Jamesjay4199 Did you try creating 'questions' in the topic and skill editors?

I've tried it and it's working fine.
Thanks

seanlip · 2019-08-10T00:35:08Z

@nithusha21, I tried loading all_interactions exploration, but I had an error: Full proto too large to save, cleared variables.

Does this error occur repeatedly if you try loading again? Normally that error message you mentioned is transient.

Other explorations loaded successfully and I tested them and they worked as expected.

Could you catalog which interactions you've tested, and then manually test the remaining ones (if you can't load the "all interactions" exploration? You need to make sure no bugs get through and that you've manually examined the entire surface area.

jameesjohn · 2019-08-10T12:48:28Z

Could you catalog which interactions you've tested.

Okay. But, this PR is more of loading css with webpack since the bulk of the work was already done by @vojtechjelinek in #6939

seanlip · 2019-08-10T21:05:49Z

Please still test all interactions comprehensively, and report the results back explicitly. We have had regressions in the past with the PRs for this project, and we should take steps to avoid that in the future.

jameesjohn · 2019-08-10T22:58:35Z

Okay.
I should be done with the testing tomorrow.

vojtechjelinek · 2019-08-12T06:58:13Z

I confirm that I have tested this PR, I tried the all interactions explorations and everything works correctly.

seanlip · 2019-08-12T07:51:19Z

Thanks! Merging.

jameesjohn added 4 commits July 31, 2019 15:50

remove interaction templates from exploration editor and exploration …

6f3884e

…player

install style loader and css loader

1687e52

remove interaction template from other pages

46345c7

use absolute path for require

52c0086

jameesjohn added CHANGELOG: Code health labels Aug 3, 2019

jameesjohn requested a review from vojtechjelinek August 3, 2019 20:30

jameesjohn requested review from aks681, DubeySandeep, kevinlee12, nithusha21, seanlip, sophiewu6 and vinitamurthi as code owners August 3, 2019 20:30

jameesjohn assigned vojtechjelinek Aug 3, 2019

work on failing FE test

723ffac

vojtechjelinek approved these changes Aug 4, 2019

View reviewed changes

webpack.dev.config.ts Show resolved Hide resolved

vojtechjelinek assigned jameesjohn, kevinlee12, seanlip, vinitamurthi and nithusha21 and unassigned jameesjohn and vojtechjelinek Aug 4, 2019

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

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

0cd2ec6

…tion-template-removal

vinitamurthi approved these changes Aug 5, 2019

View reviewed changes

vinitamurthi removed their assignment Aug 5, 2019

sophiewu6 approved these changes Aug 5, 2019

View reviewed changes

sophiewu6 removed their assignment Aug 5, 2019

jameesjohn added 2 commits August 6, 2019 15:52

remove unused html files

3fcc0f5

work on failing BE test

b9be945

aks681 approved these changes Aug 7, 2019

View reviewed changes

nithusha21 approved these changes Aug 9, 2019

View reviewed changes

nithusha21 removed their assignment Aug 9, 2019

seanlip assigned jameesjohn and unassigned aks681 Aug 10, 2019

jameesjohn changed the title ~~Remove jinja interaction templates~~ Remove jinja interaction templates and integrate css file into webpack Aug 11, 2019

jameesjohn changed the title ~~Remove jinja interaction templates and integrate css file into webpack~~ Fix #6962: Remove jinja interaction templates and integrate css file into webpack Aug 11, 2019

seanlip merged commit 68c4a4a into oppia:develop Aug 12, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #6962: Remove jinja interaction templates and integrate css file into webpack #7286

Fix #6962: Remove jinja interaction templates and integrate css file into webpack #7286

jameesjohn commented Aug 3, 2019 •

edited

Loading

kevinlee12 commented Aug 4, 2019

codecov bot commented Aug 4, 2019

codecov bot commented Aug 4, 2019 •

edited

Loading

vojtechjelinek left a comment

vinitamurthi left a comment

jameesjohn commented Aug 6, 2019

jameesjohn commented Aug 6, 2019

aks681 commented Aug 7, 2019

jameesjohn commented Aug 7, 2019

nithusha21 commented Aug 9, 2019

nithusha21 left a comment

jameesjohn commented Aug 9, 2019

jameesjohn commented Aug 9, 2019

seanlip commented Aug 10, 2019 •

edited

Loading

jameesjohn commented Aug 10, 2019

seanlip commented Aug 10, 2019

jameesjohn commented Aug 10, 2019

vojtechjelinek commented Aug 12, 2019

seanlip commented Aug 12, 2019

Fix #6962: Remove jinja interaction templates and integrate css file into webpack #7286

Fix #6962: Remove jinja interaction templates and integrate css file into webpack #7286

Conversation

jameesjohn commented Aug 3, 2019 • edited Loading

Explanation

Checklist

kevinlee12 commented Aug 4, 2019

codecov bot commented Aug 4, 2019

Codecov Report

codecov bot commented Aug 4, 2019 • edited Loading

Codecov Report

vojtechjelinek left a comment

Choose a reason for hiding this comment

vinitamurthi left a comment

Choose a reason for hiding this comment

jameesjohn commented Aug 6, 2019

jameesjohn commented Aug 6, 2019

aks681 commented Aug 7, 2019

jameesjohn commented Aug 7, 2019

nithusha21 commented Aug 9, 2019

nithusha21 left a comment

Choose a reason for hiding this comment

jameesjohn commented Aug 9, 2019

jameesjohn commented Aug 9, 2019

seanlip commented Aug 10, 2019 • edited Loading

jameesjohn commented Aug 10, 2019

seanlip commented Aug 10, 2019

jameesjohn commented Aug 10, 2019

vojtechjelinek commented Aug 12, 2019

seanlip commented Aug 12, 2019

jameesjohn commented Aug 3, 2019 •

edited

Loading

codecov bot commented Aug 4, 2019 •

edited

Loading

seanlip commented Aug 10, 2019 •

edited

Loading