Fix #4076: Update end exploration (within collection) headers to better illustrate that the user should return to the collection #4130

BenHenning · 2017-12-01T01:52:27Z

New behavior:

When ending an exploration within the context of a collection:

When ending an exploration outside the context of a collection:

Changes illustrated in text form:

Remove 'Go to Library' button
Use 'Next Lesson' instead of 'Suggested next' for explorations ended in the context of a collection
Added 'Return to Library' link for explorations ended outside a collection
Updated cases for headers & links to be more consistent

Note: This also updates the demo collection to be a bit more usable in testing scenarios (and adds 2 new simple explorations specifically for that purpose).

Note2: FYI that #4131 is tracking the incorrect globe placement; that doesn't seem to have been caused by these changes.

collection can be fully completed for testing purposes.

collection and to match the provided mocks.

with return to library text.

seanlip

LGTM. Thanks! Planning to add any e2e tests to prevent regression?

BenHenning · 2017-12-01T01:58:27Z

I'm not opposed, but what did you have in mind? We don't currently have screendiff testing, and since this PR does not include functional changes I'm not sure what meaningful aspect of this change we would test.

seanlip · 2017-12-01T02:00:52Z

I think just checking that the correct text/items appear? Not so much in terms of layout/UI but that, e.g., as a logged out user coming to the end of an exp in a collection, the correct exp summary tile is recommended next and the correct link is shown below it.

seanlip · 2017-12-01T06:45:19Z

Hi @BenHenning -- e2e tests are failing persistently. Not exactly sure what's going on, but I'm going to restart them. If the failure persists, please could you take a look at it? Thanks!

seanlip · 2017-12-01T06:46:26Z

Backend tests are also failing.

for the new strings that were added).

BenHenning · 2017-12-02T02:36:11Z

I fixed the backend test failures, but I have no clue yet why the protractor test is failing.

BenHenning · 2017-12-02T02:36:59Z

There's apparently some jquery error being printed out on the collection viewer page:

jquery.min.js:3 Error: attribute d: Unexpected end of attribute. Expected number, "… 280, 250 300 S ".

Odd thing is, I didn't change anything related to that page in this PR.

BenHenning · 2017-12-02T02:40:26Z

This seems to be an existing issue exposed by the changes to the example collection yaml file. This is probably an existing bug in the viewer that somehow wasn't hit before, and it's not yet clear what's actually broken.

BenHenning · 2017-12-02T02:41:05Z

Note that hand-made collections don't seem to have this error in prod (e.g. https://www.oppia.org/collection/4UgTQUc1tala)

BenHenning · 2017-12-02T02:42:41Z

This appears to be an error with the path generation in the collection player. Not yet clear why the new collection structure is causing this.

BenHenning · 2017-12-02T02:45:46Z

Apparently the control points for the Bezier curve aren't being correctly computed when there are 3 nodes:

https://github.com/oppia/oppia/blob/develop/core/templates/dev/head/pages/collection_player/CollectionPlayer.js#L152

sParameterExtension is empty string after that loop since i is starting at 1 and 3/2=1

BenHenning · 2017-12-02T02:46:57Z

/cc @kevintab95 original code introduced in #2526

seanlip · 2017-12-02T02:47:02Z

Does that happen with manually-created 3-node collections as well? I'm surprised we haven't run into that problem before in practice.

BenHenning · 2017-12-02T02:54:04Z

Haven't checked, but I'm working around this by adding a 4th test exploration to the demo collection. We can follow up with this issue separately.

BenHenning · 2017-12-02T02:58:38Z

Yes, this does repro with a manually-created 3-node collection as well.

BenHenning · 2017-12-02T03:06:02Z

Filed #4141 to track this issue (and #4140 which I found when trying to repro the issue with a manually-created collection).

viewer.

BenHenning · 2017-12-02T03:09:04Z

Issues fixed & repushed. Waiting on Travis now.

PTAL since there are a few new things that have been changed.

codecov-io · 2017-12-02T03:09:06Z

Codecov Report

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

@@           Coverage Diff            @@
##           develop    #4130   +/-   ##
========================================
  Coverage    43.88%   43.88%           
========================================
  Files          360      359    -1     
  Lines        22401    22401           
  Branches      3559     3559           
========================================
  Hits          9831     9831           
  Misses       12570    12570

Impacted Files	Coverage Δ
...ev/head/pages/exploration_editor/EditorServices.js	`39.03% <0%> (-0.89%)`	⬇️
...tion_editor/StateEditorTutorialFirstTimeService.js

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 2e41ccc...eee88bc. Read the comment docs.

seanlip · 2017-12-02T03:11:11Z

core/controllers/collection_viewer_test.py

@@ -129,60 +129,58 @@ def test_welcome_collection(self):
        self.assertEqual(
            collection_dict['title'], 'Introduction to Collections in Oppia')

-        # Verify there are 5 explorations in this collection, the initial
+        # Verify there are 3 explorations in this collection, the initial


Ugh, I had updated this once and missed it the second time. :( Done.

seanlip · 2017-12-02T03:12:38Z

core/tests/protractor/collections.js

@@ -33,8 +33,6 @@ describe('Collections', function() {
    adminPage.get();
    adminPage.reloadCollection();
    general.acceptAlert();
-    browser.waitForAngular();


This was just removed because it wasn't necessary? (Or was there some other reason?)

Wasn't necessary; reloading a collection automatically reloads the explorations it refers to (otherwise there would be a load error).

Apparently this is necessary for the collection editor test. :) I didn't read through it carefully enough, and since I was testing the player test in isolation I didn't realize removing this lines broke the other test.

seanlip · 2017-12-02T07:32:52Z

Hi @BenHenning -- e2e tests still failing, unfortunately!

BenHenning · 2017-12-02T23:52:24Z

Odd, it passed locally when I tried it yesterday. Will look into this.

setup.

BenHenning · 2017-12-03T21:29:11Z

Fixed--readded the setup line to load all explorations.

seanlip · 2017-12-03T21:53:01Z

Thanks! Still LGTM.

BenHenning added 4 commits November 30, 2017 17:03

Add new example explorations & update demo collection to ensure that the

27397f1

collection can be fully completed for testing purposes.

Update end of exploration content based on whether you're in a

b4dca19

collection and to match the provided mocks.

Remove Go to Library button & make 'back to collection' text consistent

977d6a2

with return to library text.

Remove rermaining 'I18N_PLAYER_GO_TO_LIBRARY' references.

6efc2ae

BenHenning requested a review from seanlip December 1, 2017 01:54

BenHenning assigned seanlip Dec 1, 2017

seanlip approved these changes Dec 1, 2017

View reviewed changes

seanlip added the PR: LGTM label Dec 1, 2017

BenHenning added 2 commits December 1, 2017 18:22

Fix broken backend tests (including ensuring that qqq.json is populated

f142826

for the new strings that were added).

Merge branch 'develop' into update-end-exp-collection-header

db7a419

BenHenning mentioned this pull request Dec 2, 2017

Collections with 3 nodes have an error when computing path in collection player #4141

Closed

Follow-up fixes and added a workaround to a bug in the collection

f6316c1

viewer.

seanlip reviewed Dec 2, 2017

View reviewed changes

seanlip approved these changes Dec 2, 2017

View reviewed changes

Address reviewer comment.

f43900d

Re-added necessary lines for the collections protractor test during

dbb7416

setup.

Merge branch 'develop' into update-end-exp-collection-header

eee88bc

BenHenning merged commit 4cf1580 into develop Dec 4, 2017

BenHenning deleted the update-end-exp-collection-header branch December 4, 2017 02:28

BenHenning mentioned this pull request Dec 16, 2017

The music interaction appears to be broken #4216

Closed

This was referenced Jan 11, 2018

[2.6.0] [iOS] Completing an exploration within a collection does not show proper 'Return to collection' ending #4493

Closed

[2.6.0] [Web] Finishing exploration in collection with no later explorations should not show unrelated explorations #4494

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #4076: Update end exploration (within collection) headers to better illustrate that the user should return to the collection #4130

Fix #4076: Update end exploration (within collection) headers to better illustrate that the user should return to the collection #4130

BenHenning commented Dec 1, 2017 •

edited

Loading

seanlip left a comment

BenHenning commented Dec 1, 2017 •

edited

Loading

seanlip commented Dec 1, 2017 •

edited

Loading

seanlip commented Dec 1, 2017

seanlip commented Dec 1, 2017

BenHenning commented Dec 2, 2017

BenHenning commented Dec 2, 2017

BenHenning commented Dec 2, 2017

BenHenning commented Dec 2, 2017

BenHenning commented Dec 2, 2017

BenHenning commented Dec 2, 2017

BenHenning commented Dec 2, 2017

seanlip commented Dec 2, 2017

BenHenning commented Dec 2, 2017

BenHenning commented Dec 2, 2017

BenHenning commented Dec 2, 2017

BenHenning commented Dec 2, 2017

codecov-io commented Dec 2, 2017 •

edited

Loading

seanlip Dec 2, 2017

BenHenning Dec 2, 2017

seanlip Dec 2, 2017

BenHenning Dec 2, 2017

BenHenning Dec 3, 2017

seanlip commented Dec 2, 2017

BenHenning commented Dec 2, 2017 •

edited

Loading

BenHenning commented Dec 3, 2017

seanlip commented Dec 3, 2017

Fix #4076: Update end exploration (within collection) headers to better illustrate that the user should return to the collection #4130

Fix #4076: Update end exploration (within collection) headers to better illustrate that the user should return to the collection #4130

Conversation

BenHenning commented Dec 1, 2017 • edited Loading

seanlip left a comment

Choose a reason for hiding this comment

BenHenning commented Dec 1, 2017 • edited Loading

seanlip commented Dec 1, 2017 • edited Loading

seanlip commented Dec 1, 2017

seanlip commented Dec 1, 2017

BenHenning commented Dec 2, 2017

BenHenning commented Dec 2, 2017

BenHenning commented Dec 2, 2017

BenHenning commented Dec 2, 2017

BenHenning commented Dec 2, 2017

BenHenning commented Dec 2, 2017

BenHenning commented Dec 2, 2017

seanlip commented Dec 2, 2017

BenHenning commented Dec 2, 2017

BenHenning commented Dec 2, 2017

BenHenning commented Dec 2, 2017

BenHenning commented Dec 2, 2017

codecov-io commented Dec 2, 2017 • edited Loading

Codecov Report

seanlip Dec 2, 2017

Choose a reason for hiding this comment

BenHenning Dec 2, 2017

Choose a reason for hiding this comment

seanlip Dec 2, 2017

Choose a reason for hiding this comment

BenHenning Dec 2, 2017

Choose a reason for hiding this comment

BenHenning Dec 3, 2017

Choose a reason for hiding this comment

seanlip commented Dec 2, 2017

BenHenning commented Dec 2, 2017 • edited Loading

BenHenning commented Dec 3, 2017

seanlip commented Dec 3, 2017

BenHenning commented Dec 1, 2017 •

edited

Loading

BenHenning commented Dec 1, 2017 •

edited

Loading

seanlip commented Dec 1, 2017 •

edited

Loading

codecov-io commented Dec 2, 2017 •

edited

Loading

BenHenning commented Dec 2, 2017 •

edited

Loading