Fix #3677: Changes refreshSettingTab function to fix the loading issue with the e2e test. #7526

Showtim3 · 2019-08-31T21:23:46Z

Explanation

Fix #3677: So the the settingsTab.template.html changes are inspired from PR 6755, and I have wrote a e2e test for the same.

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 -m scripts.pre_commit_linter 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.

oppiabot · 2019-09-01T02:49:57Z

Assigning @nithusha21 for the first-pass review of this pull request. Thanks!

kevinlee12 · 2019-09-01T02:50:12Z

@Showtim3 please don't force push :)

codecov · 2019-09-01T03:12:42Z

Codecov Report

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

@@             Coverage Diff              @@
##           develop    #7526       +/-   ##
============================================
+ Coverage    73.27%   83.43%   +10.16%     
============================================
  Files          893     1099      +206     
  Lines        39176    63195    +24019     
  Branches      3617     3617               
============================================
+ Hits         28704    52723    +24019     
  Misses        9316     9316               
  Partials      1156     1156

Flag	Coverage Δ
#backend	`100% <ø> (?)`
#frontend	`73.27% <ø> (ø)`	⬆️

Impacted Files	Coverage Δ
core/domain/user_jobs_continuous.py	`100% <0%> (ø)`
extensions/issues/base.py	`100% <0%> (ø)`
core/domain/state_domain.py	`100% <0%> (ø)`
extensions/rich_text_components/components.py	`100% <0%> (ø)`
core/domain/skill_domain.py	`100% <0%> (ø)`
core/domain/visualization_registry.py	`100% <0%> (ø)`
core/storage/suggestion/gae_models.py	`100% <0%> (ø)`
extensions/visualizations/models.py	`100% <0%> (ø)`
core/controllers/cron.py	`100% <0%> (ø)`
core/platform/image/gae_image_services.py	`100% <0%> (ø)`
... and 196 more

codecov · 2019-09-01T03:12:44Z

Codecov Report

Merging #7526 into develop will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff            @@
##           develop    #7526   +/-   ##
========================================
  Coverage    83.63%   83.63%           
========================================
  Files         1104     1104           
  Lines        64007    64007           
  Branches      3621     3621           
========================================
  Hits         53530    53530           
  Misses        9318     9318           
  Partials      1159     1159

Flag	Coverage Δ
#backend	`100% <ø> (ø)`	⬆️
#frontend	`73.3% <ø> (ø)`	⬆️

Showtim3 · 2019-09-01T06:12:01Z

@kevinlee12 hey m sorry for that, It's a habit i picked up at work to have maximum one commit per PR. Anyways, I'll keep that in mind.

kevinlee12 · 2019-09-01T17:28:00Z

Okay, just make sure to be careful in the future.

oppiabot · 2019-09-04T01:38:42Z

Hi @Showtim3. 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!

nithusha21

Overall LGTM from my perspective. not very sure about the changes to settingsTab.js however!

nithusha21 · 2019-09-04T19:41:08Z

core/tests/protractor_utils/ExplorationEditorPage.js

+      expInput.sendKeys(element, protractor.Key.ENTER);
+    });
+
+    const saveChangesButton = element(by.css(


This block is not very readable. Can you add newlines in between logical block of statements?

nithusha21 · 2019-09-04T19:42:18Z

core/tests/protractor_utils/workflow.js

@@ -139,6 +139,18 @@ var createAndPublishExploration = function(
  publishExploration();
 };

+var createAddAndPublish = function(


Pick a more descriptive name. Create, add (what?) and publish what? I think it should be something like createAddIneractionAndPublishExp()

nithusha21 · 2019-09-04T19:43:23Z

@DubeySandeep can you PTAL as a codeowner. I am not sure about the changes to your files!

kevinlee12

I have one nit, @nithusha21 covered most of it.

kevinlee12 · 2019-09-05T03:09:50Z

core/templates/dev/head/pages/exploration-editor-page/settings-tab/settings-tab.directive.ts

          };

          ctrl.refreshSettingsTab = function() {
            // Ensure that ExplorationStatesService has been initialized before
-            // getting the state names from it. Otherwise, navigating to the
+            // getting the state names from it. (Otherwise, navigating to the


DubeySandeep

LGTM for the code owner file.

Showtim3 added CHANGELOG: Testing coverage labels Aug 31, 2019

Showtim3 requested review from DubeySandeep, kevinlee12, nithusha21, seanlip and vojtechjelinek as code owners August 31, 2019 21:23

Showtim3 closed this Aug 31, 2019

Showtim3 deleted the reflectEdtiorChanges branch August 31, 2019 21:32

Showtim3 restored the reflectEdtiorChanges branch August 31, 2019 21:34

Showtim3 reopened this Aug 31, 2019

e2e test to reflect exploration details in setting too

3339971

Showtim3 force-pushed the reflectEdtiorChanges branch from ecbf6c3 to 3339971 Compare August 31, 2019 21:35

kevinlee12 added the PROJECT: QA team -- @nithusha21 label Sep 1, 2019

oppiabot bot assigned nithusha21 Sep 1, 2019

vojtechjelinek removed their request for review September 2, 2019 08:40

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

nithusha21 approved these changes Sep 4, 2019

View reviewed changes

nithusha21 assigned kevinlee12 and DubeySandeep and unassigned nithusha21 Sep 4, 2019

kevinlee12 approved these changes Sep 5, 2019

View reviewed changes

kevinlee12 removed their assignment Sep 5, 2019

DubeySandeep approved these changes Sep 7, 2019

View reviewed changes

DubeySandeep assigned Showtim3 and unassigned DubeySandeep Sep 7, 2019

Showtim3 added 3 commits September 8, 2019 16:25

Pulling develop

ee23b97

pull develop

7b2b190

Pulled develop and implemented suggestions

0b9252d

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

Removing extra parenthesis in comments

0b273fc

kevinlee12 merged commit 2125d5f into oppia:develop Sep 8, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #3677: Changes refreshSettingTab function to fix the loading issue with the e2e test. #7526

Fix #3677: Changes refreshSettingTab function to fix the loading issue with the e2e test. #7526

Showtim3 commented Aug 31, 2019

oppiabot bot commented Sep 1, 2019

kevinlee12 commented Sep 1, 2019

codecov bot commented Sep 1, 2019

codecov bot commented Sep 1, 2019 •

edited

Loading

Showtim3 commented Sep 1, 2019

kevinlee12 commented Sep 1, 2019

oppiabot bot commented Sep 4, 2019

nithusha21 left a comment

nithusha21 Sep 4, 2019

Showtim3 Sep 8, 2019

nithusha21 Sep 4, 2019

Showtim3 Sep 8, 2019

Showtim3 Sep 8, 2019

nithusha21 commented Sep 4, 2019

kevinlee12 left a comment

kevinlee12 Sep 5, 2019

Showtim3 Sep 8, 2019

Showtim3 Sep 8, 2019

DubeySandeep left a comment

Fix #3677: Changes refreshSettingTab function to fix the loading issue with the e2e test. #7526

Fix #3677: Changes refreshSettingTab function to fix the loading issue with the e2e test. #7526

Conversation

Showtim3 commented Aug 31, 2019

Explanation

Checklist

oppiabot bot commented Sep 1, 2019

kevinlee12 commented Sep 1, 2019

codecov bot commented Sep 1, 2019

Codecov Report

codecov bot commented Sep 1, 2019 • edited Loading

Codecov Report

Showtim3 commented Sep 1, 2019

kevinlee12 commented Sep 1, 2019

oppiabot bot commented Sep 4, 2019

nithusha21 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

nithusha21 commented Sep 4, 2019

kevinlee12 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

DubeySandeep left a comment

Choose a reason for hiding this comment

codecov bot commented Sep 1, 2019 •

edited

Loading