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

fix part of #3954 - LearnerDashboardPage.js #4138

Merged
merged 9 commits into from
Dec 4, 2017
Merged

fix part of #3954 - LearnerDashboardPage.js #4138

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
3eb5ecd
fix part of #3954 - Learner Dashboard Page
aks681 Dec 1, 2017
191a1ed
fixed lint errors
aks681 Dec 1, 2017
268a80e
removed LEARNER_DASHBOARD_URL from general.js
aks681 Dec 1, 2017
111b314
fixes to learnerDashboard.js
aks681 Dec 1, 2017
19bc125
some fixes and more functions added
aks681 Dec 3, 2017
4f4f6e9
made the review changes
aks681 Dec 3, 2017
9603377
changed page object names
aks681 Dec 4, 2017
00ced47
added subscription and feedback sections
aks681 Dec 4, 2017
4c3195c
made the review changes
aks681 Dec 4, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
some fixes and more functions added
  • Loading branch information
@aks681
aks681 committed Dec 3, 2017
commit 19bc125f9e942bbb6a5c8d79b0b26728e7a131c1
35 changes: 21 additions & 14 deletions core/tests/protractor/learnerDashboard.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,9 +60,16 @@ describe('Learner dashboard functionality', function() {
browser.waitForAngular();
element(by.css('.protractor-test-create-activity')).click();
// Create new collection.
learnerDashboardPage.createExploration();
element(by.css('.protractor-test-create-collection')).click();
browser.waitForAngular();
learnerDashboardPage.publishExploration();
collectionEditor.addExistingExploration('14');
collectionEditor.saveDraft();
collectionEditor.closeSaveModal();
collectionEditor.publishCollection();
collectionEditor.setTitle('Test Collection');
collectionEditor.setObjective('This is a test collection.');
collectionEditor.setCategory('Algebra');
collectionEditor.saveChanges();
browser.waitForAngular();
users.logout();
});
Expand Down Expand Up @@ -90,7 +97,7 @@ describe('Learner dashboard functionality', function() {
player.submitAnswer('Continue', null);
learnerDashboardPage.get();
browser.waitForAngular();
learnerDashboardPage.completedProfileTest();
learnerDashboardPage.navigateToCompletedSection();
Copy link
Member

Choose a reason for hiding this comment

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

Just to check -- is this completed collections, completed explorations, or something else?

Copy link
Contributor Author

@aks681 aks681 Dec 4, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I think its a section of completed explorations and collections. In LearnerDashboard.js, of /learner_dashboard it is called above the subsection that calls both of them.

browser.waitForAngular();
libraryPage.expectExplorationToBeVisible('About Oppia');
users.logout();
Expand Down Expand Up @@ -131,11 +138,11 @@ describe('Learner dashboard functionality', function() {
browser.waitForAngular();
general.waitForSystem();
browser.ignoreSynchronization = false;
learnerDashboardPage.incompleteCollectionSection();
learnerDashboardPage.navigateToIncompleteCollection();
Copy link
Member

Choose a reason for hiding this comment

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

navigateToIncompleteCollectionSection (you're navigating to a section, not to a collection)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ya, I thought about this, but thought name might become too long. Will change that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

browser.waitForAngular();
Copy link
Member

Choose a reason for hiding this comment

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

It might be better to put these browser.waitForAngular() calls at the end of their respective functions in the page object, if they're always going to be called. This will also help simplify the main protractor test.

Ditto for general.waitForSystem(), where relevant.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If there are are other calls to learnerDashboardPage that are repeating for all such instances, shall I put it inside page object also?

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I don't understand. Could you elaborate / give an example?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

after both instances of navigateToIncompleteCollectionsSection(), expectTitleOfSummaryTileToMatch() is called, so can I put this call as this.expectTitleOfSummaryTileToMatch() inside the navigateToIncompleteCollectionsSection() function?

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I see. No, don't do that.

The way to think about this is that these are the actions a user would do. A user wouldn't really be doing stuff at the level of "wait for angular". But they may take the actions "navigate" and "expect title to match" independently, so these should be independent actions in the page object.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

general.waitForSystem();
expect(
learnerDashboardPage.summaryTile()
learnerDashboardPage.openSummaryTile()
Copy link
Member

Choose a reason for hiding this comment

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

open...() doesn't have a connotation of "return a value". Maybe it should be "getTitleOfSummaryTile()" or similar, though that's just an example -- you'd need to check what exactly is being retrieved here and include that in the method name.

Copy link
Contributor Author

@aks681 aks681 Dec 3, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, that is what is being retrieved here, will change that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

).toMatch('Test Collection');

// Go to the test collection.
Expand All @@ -157,14 +164,14 @@ describe('Learner dashboard functionality', function() {
learnerDashboardPage.get();
browser.waitForAngular();
general.waitForSystem();
learnerDashboardPage.completedProfileTest();
learnerDashboardPage.navigateToCompletedSection();
browser.waitForAngular();
general.waitForSystem();
learnerDashboardPage.completedCollectionSection();
learnerDashboardPage.navigateToCompletedCollection();
browser.waitForAngular();
general.waitForSystem();
expect(
learnerDashboardPage.summaryTile()
learnerDashboardPage.openSummaryTile()
).toMatch('Test Collection');
users.logout();

Expand Down Expand Up @@ -192,11 +199,11 @@ describe('Learner dashboard functionality', function() {
learnerDashboardPage.get();
browser.waitForAngular();
general.waitForSystem();
learnerDashboardPage.incompleteCollectionSection();
learnerDashboardPage.navigateToIncompleteCollection();
browser.waitForAngular();
general.waitForSystem();
expect(
learnerDashboardPage.summaryTile()
learnerDashboardPage.openSummaryTile()
).toMatch('Test Collection');
users.logout();
});
Expand All @@ -207,17 +214,17 @@ describe('Learner dashboard functionality', function() {
// Subscribe to both the creators.
browser.get('/profile/creator1learnerDashboard');
browser.waitForAngular();
learnerDashboardPage.clickSubscriptionButton();
element(by.css('.protractor-test-subscription-button')).click();
browser.get('/profile/creator2learnerDashboard');
browser.waitForAngular();
learnerDashboardPage.clickSubscriptionButton();
element(by.css('.protractor-test-subscription-button')).click();

// Both creators should be present in the subscriptions section of the
// dashboard.
learnerDashboardPage.get();
browser.waitForAngular();
general.waitForSystem();
element(by.css('.protractor-test-subscriptions-section')).click();
learnerDashboardPage.navigateToSubscriptionsSection();
browser.waitForAngular();
expect(element.all(by.css(
Copy link
Member

Choose a reason for hiding this comment

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

I guess this stuff should also be factored into the learner dashboard page?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As it was just after a learnerDashboard.get(), I assumed it to be a part of learner dashboard itself.

Copy link
Member

Choose a reason for hiding this comment

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

That's the point I was making, though. If it's part of the learner dashboard page, shouldn't it be refactored into the LearnerDashboardPage object?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, thats what I did. Is there something more?

Copy link
Member

Choose a reason for hiding this comment

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

What I'm saying is that this line (expect(element.all(by.css(...))).toMatch(...);) should be wrapped in a method on the LearnerDashboardPage object, so that this line changes to something like learnerDashboardPage.someMethodName('creator...'). It doesn't look like that's been done.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, ok, will add that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

'.protractor-test-subscription-name')).first().getText()).toMatch(
Expand All @@ -241,7 +248,7 @@ describe('Learner dashboard functionality', function() {
player.submitFeedback(feedback);
learnerDashboardPage.get();
browser.waitForAngular();
element(by.css('.protractor-test-feedback-section')).click();
learnerDashboardPage.navigateToFeedbackSection();
browser.waitForAngular();
expect(element.all(by.css(
Copy link
Member

Choose a reason for hiding this comment

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

This also seems like something that should be part of the LearnerDashboardPage object?

Basically, anything that involves fiddling around with elements should be part of the page object, so that the protractor test itself doesn't have to mess around with the details.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I added that in the learnerDashboardPage object already. Is there something else?

Copy link
Member

Choose a reason for hiding this comment

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

I don't think it's done, or rather, if it is, then this line should be learnerDashboardPage.something(...). See previous comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, saw it, will add that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

'.protractor-test-feedback-exploration')).first().getText()).toMatch(
Expand Down
47 changes: 18 additions & 29 deletions core/tests/protractor_utils/LearnerDashboardPage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,57 +21,46 @@ var collectionEditor = require('./collectionEditor.js');

var LearnerDashboardPage = function() {
var LEARNER_DASHBOARD_URL = '/learner_dashboard';
var subscriptionButton =
element(by.css('.protractor-test-subscription-button'));
var dashboardLink = element(by.css('.protractor-test-dashboard-link'));
var createCollection = element(by.css('.protractor-test-create-collection'));
var profileTestCompleted =
var completedSection =
element(by.css('.protractor-test-completed-section'));
var feedbackSection =
element(by.css('.protractor-test-feedback-section'));
var completedCollection =
Copy link
Member

Choose a reason for hiding this comment

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

These are sections; their names should end with Section. Ditto below.

Copy link
Contributor Author

@aks681 aks681 Dec 3, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

ok, will change those.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

element(by.css('.protractor-test-completed-collection-section'));
var incompleteCollection =
element(by.css('.protractor-test-incomplete-collection-section'));
var summary =
var subscriptionsSection =
element(by.css('.protractor-test-subscriptions-section'));
var summaryTile =
Copy link
Member

Choose a reason for hiding this comment

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

Is this the tile, or its title? The CSS name doesn't seem to match the variable name.

It's important to be precise when naming, otherwise developers will get confused.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, misread the name, will change that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

element.all(by.css('.protractor-test-collection-summary-tile-title'));

this.get = function() {
return browser.get(LEARNER_DASHBOARD_URL);
};

this.clickSubscriptionButton = function() {
subscriptionButton.click();
this.navigateToCompletedSection = function() {
completedSection.click();
}
Copy link
Member

Choose a reason for hiding this comment

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

Missing semicolons, here and below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, didn't see those. Will add them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done


this.createExploration = function() {
createCollection.click();
this.navigateToIncompleteCollection = function() {
incompleteCollection.click();
}

this.completedProfileTest = function() {
profileTestCompleted.click();
this.navigateToCompletedCollection = function() {
completedCollection.click();
}

this.incompleteCollectionSection = function() {
incompleteCollection.click();
this.navigateToFeedbackSection = function() {
feedbackSection.click();
}

this.completedCollectionSection = function() {
completedCollection.click();
this.navigateToSubscriptionsSection = function() {
subscriptionsSection.click();
}

this.summaryTile = function() {
return summary.first().getText();
this.openSummaryTile = function() {
return summaryTile.first().getText();
}

this.publishExploration = function() {
collectionEditor.addExistingExploration('14');
collectionEditor.saveDraft();
collectionEditor.closeSaveModal();
collectionEditor.publishCollection();
collectionEditor.setTitle('Test Collection');
collectionEditor.setObjective('This is a test collection.');
collectionEditor.setCategory('Algebra');
collectionEditor.saveChanges();
};
};

exports.LearnerDashboardPage = LearnerDashboardPage;