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.

Sign up for GitHub

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

Jump to bottom

Fix part of #6732: Migrate interactions and classifiers to webpack #6939

Merged
merged 17 commits into from
Jun 20, 2019
Merged

Fix part of #6732: Migrate interactions and classifiers to webpack #6939

merged 17 commits into from
Jun 20, 2019

Conversation

vojtechjelinek
Copy link
Contributor

@vojtechjelinek vojtechjelinek commented Jun 15, 2019 •
edited
Loading

Explanation

Fixes part of #6732: Migrate interactions and classifiers to webpack.

Part of this was done by @YashJipkate, thanks!

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 vojtechjelinek requested a review from seanlip June 15, 2019 22:57
@vojtechjelinek
Copy link
Contributor Author

@kevinlee12 Can you QA this for me?

@kevinlee12
Copy link
Contributor

kevinlee12 commented Jun 15, 2019 via email

kevinlee12
kevinlee12 suggested changes Jun 16, 2019
View reviewed changes
Copy link
Contributor

@kevinlee12 kevinlee12 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Hi @vojtechjelinek, here are the errors that I see below, I didn't find any other issues.

Case 1:

Unknown provider: EndExplorationRulesServiceProvider <- EndExplorationRulesService

  1. Load the Solar System interaction
  2. Open the Solar system interaction
  3. Go to the editor for the Solar System interaction and observe the above error.

Case 2:

Error: Text Input state name with unicode ął does not exist.
getStateStats@http://localhost:8181/dist/exploration_editor.bundle.js:31638:27
  1. Load "Test of all interactions`.
  2. Go to the editor of that interaction
  3. Go to the Item Selection Multi card and observe the error above.

@seanlip
Copy link
Member

seanlip commented Jun 16, 2019

Hi @vojtechjelinek, looks like the backend and e2e tests are failing too, in addition to @kevinlee12's comments. Could you PTAL? Happy to review once those are fixed, thanks!

@seanlip
Copy link
Member

seanlip commented Jun 17, 2019

Adding codeowners for review since I will probably be delayed in being able to merge this.

vinitamurthi
vinitamurthi approved these changes Jun 17, 2019
View reviewed changes
Copy link
Contributor

@vinitamurthi vinitamurthi left a comment

Choose a reason for hiding this comment

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

LGTM for practice-sessions!

nithusha21
nithusha21 approved these changes Jun 18, 2019
View reviewed changes
Copy link
Contributor

@nithusha21 nithusha21 left a comment

Choose a reason for hiding this comment

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

LGTM as a codeowner

@nithusha21 nithusha21 removed their assignment Jun 18, 2019
@kevinlee12 kevinlee12 removed their assignment Jun 19, 2019
@kevinlee12
Copy link
Contributor

@vojtechjelinek, fyi there are two e2e tests that are failing, you might have get the latest from develop.

prasanna08
prasanna08 approved these changes Jun 19, 2019
View reviewed changes
Copy link
Contributor

@prasanna08 prasanna08 left a comment

Choose a reason for hiding this comment

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

LGTM as long as all dependencies (prediction services) are imported in frontend.

@vinitamurthi vinitamurthi removed their assignment Jun 19, 2019
@kevinlee12
Copy link
Contributor

Hi @apb7, would you have any idea why this error (https://travis-ci.org/oppia/oppia/jobs/547760215#L922) is happening?

@kevinlee12
Copy link
Contributor

I ran the production test for the collection e2e tests offline and it works, my guess is that we need to remove the cache.

@YashJipkate
Copy link
Contributor

HI @kevinlee12, That error has been floating around in various PRs and it does not relate to the PR. It is something about some third party library which has changed. We had a discussion on this over mail when the Travis broke for develop with the same error. I will cc you in that thread so you get an idea.
Meanwhile, I have restarted the tests which should make it pass (it passes this way, if it fails, we restart again :P)
Thanks!

@kevinlee12
Copy link
Contributor

Hmm, okay. I've restarted the test after you restarted it. If it fails, it would have failed 3 times in a row (since I restarted the test earlier today), so something else will have to be done.

@kevinlee12 kevinlee12 requested a review from seanlip June 20, 2019 18:55
@seanlip seanlip merged commit 1f65946 into oppia:develop Jun 20, 2019
@jameesjohn jameesjohn mentioned this pull request Aug 10, 2019
Merged
9 tasks
@vojtechjelinek vojtechjelinek deleted the webpack-extensions-interactions branch January 26, 2020 18:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@YashJipkate YashJipkate YashJipkate left review comments

@aks681 aks681 aks681 approved these changes

@nithusha21 nithusha21 nithusha21 approved these changes

@vinitamurthi vinitamurthi vinitamurthi approved these changes

@seanlip seanlip seanlip approved these changes

@prasanna08 prasanna08 prasanna08 approved these changes

@kevinlee12 kevinlee12 kevinlee12 approved these changes

@ankita240796 ankita240796 Awaiting requested review from ankita240796

@apb7 apb7 Awaiting requested review from apb7

@DubeySandeep DubeySandeep Awaiting requested review from DubeySandeep

Assignees

@vojtechjelinek vojtechjelinek

@seanlip seanlip

@ankita240796 ankita240796

@DubeySandeep DubeySandeep

@aks681 aks681

@apb7 apb7

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.