Project 2 Issue #5671 #5705

arod0214 · 2018-09-27T23:56:44Z

Explanation

Checklist

The PR title starts with "Fix #bugnum: ", followed by a short, clear summary of the changes.
The PR explanation includes the words "Fixes #bugnum: ...".
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 follows the style guide.
The PR is assigned to an appropriate reviewer.
- If you're a new contributor, please ask on Gitter for someone to assign a reviewer.
- 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.

aks681

Looks good! Minor comments.

aks681 · 2018-09-28T20:31:39Z

assets/constants.js

@@ -464,7 +464,7 @@ var constants = {
    "\\u001b", "\\u001c", "\\u001d", "\\u001e", "\\u001f"
  ],

-  "ENABLE_NEW_STRUCTURES": false,
+  "ENABLE_NEW_STRUCTURES": true,


Can you disable this?

aks681 · 2018-09-28T20:33:49Z

feconf.py

@@ -367,7 +367,7 @@ def get_empty_ratings():
 ENABLE_MAINTENANCE_MODE = False

 # Disables all the new structures' pages, till they are completed.
-ENABLE_NEW_STRUCTURES = False
+ENABLE_NEW_STRUCTURES = True


Can you merge with the most recent version in develop? SInce, this is no longer used.

@arod0214 FYI, the instructions for doing this are at step 5 in this section. Let us know if you need any help!

aks681 · 2018-09-28T20:34:49Z

core/templates/dev/head/components/top_navigation_bar/top_navigation_bar_directive.html

@@ -121,6 +121,7 @@
          <a ng-click="closeSubmenuIfNotMobile($event)" href="/learner_dashboard"
             class="protractor-test-dashboard-link protractor-test-learner-dashboard-link">
            <span translate="I18N_TOPNAV_LEARNER_DASHBOARD"></span>
+            <span><[isAdmin]> | <[isTopicManager]> | <[newStructuresEnabled]></span>


I am not sure what this is intended for, was it meant for debugging? In which case, can you remove this?

seanlip · 2018-09-29T00:13:10Z

core/templates/dev/head/pages/topic_editor/TopicEditorNavbarDirective.js

@@ -58,7 +58,11 @@ oppia.directive('topicEditorNavbar', [
              function() {
                $scope.topicRights.markTopicAsPublished();
                TopicEditorStateService.setTopicRights($scope.topicRights);
-              });
+              }).then(function(){


Linting nit, here and elsewhere: add space before {

@arod0214 did you see this comment?

seanlip · 2018-09-29T00:14:04Z

feconf.py

@@ -367,7 +367,7 @@ def get_empty_ratings():
 ENABLE_MAINTENANCE_MODE = False

 # Disables all the new structures' pages, till they are completed.
-ENABLE_NEW_STRUCTURES = False
+ENABLE_NEW_STRUCTURES = True


@arod0214 FYI, the instructions for doing this are at step 5 in this section. Let us know if you need any help!

oppiabot · 2018-09-30T04:52:18Z

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

arod0214 · 2018-10-04T20:49:16Z

I'm getting an error that says: Permission denied (publickey). fatal: Could not read from remote repository. Please make sure you have the correct access rights and the repository exists.

…

On Sun, Sep 30, 2018 at 12:52 AM oppiabot[bot] ***@***.***> wrote: Hi @arod0214 <https://github.com/arod0214>. Due to recent changes in the "develop" branch, this PR now has a merge conflict. Please follow this link <https://help.github.com/articles/resolving-a-merge-conflict-using-the-command-line/> if you need help resolving the conflict, so that the PR can be merged. Thanks! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#5705 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AjcWT9cbymroF0GFuAJi4p4p9C4gozFKks5ugE4IgaJpZM4W9oyn> .

seanlip · 2018-10-04T21:46:50Z

Hi @arod0214 -- hm, that's a bit odd. Could you please send the following:

the output of git remote -v
the command you're using for pushing?

Thanks!

seanlip · 2018-10-04T21:47:41Z

Also, just to check, are you following the instructions here (step 5)?

arod0214 · 2018-10-04T21:49:38Z

output: origin https://github.com/arod0214/oppia.git (fetch) origin https://github.com/arod0214/oppia.git (push) upstream git@github.com:oppia/oppia (fetch) upstream git@github.com:oppia/oppia (push) and to push I write git push upstream

…

On Thu, Oct 4, 2018 at 5:47 PM Sean Lip ***@***.***> wrote: Hi @arod0214 <https://github.com/arod0214> -- hm, that's a bit odd. Could you please send the following: - the output of git remote -v - the command you're using for pushing? Thanks! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#5705 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AjcWT96fHar6P7xe_hpWHDqXRpTISR4gks5uhoHLgaJpZM4W9oyn> .

seanlip · 2018-10-04T21:54:25Z

Ah ok, so your remotes are set up correctly. But you'll need to push to origin, not upstream -- that's how GItHub pull requests work.

See the instructions and code snippet in section 5 here -- that should get things sorted.

arod0214 · 2018-10-04T21:59:38Z

I'm sorry. I said the wrong thing. I know I have to push to origin and do the pull requests. I received the permission denied when I was trying to git pull upstream to update my repo with whatever changes that have been made on the oppia repo.

…

On Thu, Oct 4, 2018, 5:54 PM Sean Lip ***@***.***> wrote: Ah ok, so your remotes are set up correctly. But you'll need to push to origin, not upstream -- that's how GItHub pull requests work. See the code snippet in section 5 here <https://github.com/oppia/oppia/wiki/Contributing-code-to-Oppia#instructions-for-making-a-code-change> -- that should get things sorted. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#5705 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AjcWT0G019ACVlFQB8CaEhyquuDA2qiMks5uhoOdgaJpZM4W9oyn> .

seanlip · 2018-10-04T22:12:27Z

Interesting, your upstream is different from mine. Try

    git remote set-url upstream https://github.com/oppia/oppia

and see if it works after that? (If not, please paste the exact "pull" command you're using.)

codecov-io · 2018-10-05T00:49:28Z

Codecov Report

Merging #5705 into develop will decrease coverage by 0.01%.
The diff coverage is 0%.

@@             Coverage Diff             @@
##           develop    #5705      +/-   ##
===========================================
- Coverage    45.74%   45.72%   -0.02%     
===========================================
  Files          515      515              
  Lines        29956    29968      +12     
  Branches      4522     4522              
===========================================
  Hits         13702    13702              
- Misses       16254    16266      +12

Impacted Files	Coverage Δ
...topics_and_skills_dashboard/SkillsListDirective.js	`2.77% <0%> (-0.26%)`	⬇️
...d/pages/topic_editor/TopicEditorNavbarDirective.js	`1.66% <0%> (-0.19%)`	⬇️

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 3f1dca5...3253839. Read the comment docs.

seanlip

Hi @arod0214, took another pass!

Also, one request: please could you reply to all comments, following the instructions in step 5 here? In particular, for comments that you've addressed, please reply "Done." -- this helps keep track of which comments have been addressed and which haven't, and also gives the reviewer the signal that they should look again at the PR. Thanks!

seanlip · 2018-10-05T01:04:31Z

core/templates/dev/head/components/top_navigation_bar/top_navigation_bar_directive.html

@@ -121,6 +121,7 @@
          <a ng-click="closeSubmenuIfNotMobile($event)" href="/learner_dashboard"
             class="protractor-test-dashboard-link protractor-test-learner-dashboard-link">
            <span translate="I18N_TOPNAV_LEARNER_DASHBOARD"></span>
+            <span><[isAdmin]></span>


Should this be removed?

seanlip · 2018-10-05T01:04:46Z

core/templates/dev/head/pages/topic_editor/TopicEditorNavbarDirective.js

@@ -58,7 +58,11 @@ oppia.directive('topicEditorNavbar', [
              function() {
                $scope.topicRights.markTopicAsPublished();
                TopicEditorStateService.setTopicRights($scope.topicRights);
-              });
+              }).then(function(){


@arod0214 did you see this comment?

seanlip · 2018-10-05T01:05:40Z

core/templates/dev/head/pages/topic_editor/TopicEditorNavbarDirective.js

@@ -58,7 +58,11 @@ oppia.directive('topicEditorNavbar', [
              function() {
                $scope.topicRights.markTopicAsPublished();
                TopicEditorStateService.setTopicRights($scope.topicRights);
-              });
+              }).then(function(){
+              var successToast = ('Topic Published.');


Btw, here and elsewhere, you don't need the parentheses surrounding the successs message. Those are only useful when the message spans multiple lines (like line 137 in SkillsListDirective.js).

oppiabot · 2018-10-15T01:08:01Z

Hi @arod0214, I'm going to mark this PR as stale because it hasn't had any updates for 10 days. If no further activity occurs within 7 days, it will be automatically closed so that others can take up the issue.
If you are still working on this PR, please make a follow-up commit within 7 days (and submit it for review, if applicable). Please also let us know if you are stuck so we can help you!

arod0214 · 2018-10-16T21:00:17Z

I have pushed the requested changes.

seanlip

Just one more thing. Otherwise looks great. Thanks!

core/templates/dev/head/pages/topic_editor/TopicEditorNavbarDirective.js

…rective.js

core/templates/dev/head/pages/topics_and_skills_dashboard/SkillsListDirective.js

…lsListDirective.js

seanlip

All good :) Thanks @arod0214 (and congrats on your first PR to Oppia)!

arod0214 · 2018-10-16T22:04:45Z

Thank you!

seanlip · 2018-10-17T21:46:15Z

@arod0214 -- Travis tests show that there's a single lint error left. This is a bit odd, because the linter is supposed to run prior to code submission. Did the linter script pass locally on your machine when you did the last push?

Also, @apb7, I cannot tell from the Travis log what the linter error is. It says "1 JavaScript file failed" but I can't find the error message. Do you have any insight here? /cc @dawsoneliasen

arod0214 · 2018-10-17T22:01:03Z

I had no issues with linting when I pushed.

…

On Wed, Oct 17, 2018 at 5:46 PM Sean Lip ***@***.***> wrote: @arod0214 <https://github.com/arod0214> -- Travis tests show that there's a single lint error left. This is a bit odd, because the linter is supposed to run prior to code submission. Did the linter script pass locally on your machine when you did the last push? Also, @apb7 <https://github.com/apb7>, I cannot tell from the Travis log what the linter error is. It says "1 JavaScript file failed" but I can't find the error message. Do you have any insight here? /cc @dawsoneliasen <https://github.com/dawsoneliasen> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#5705 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AjcWT-nYO9hoUlsOBoAAizA7pX64uhbBks5ul6UwgaJpZM4W9oyn> .

seanlip · 2018-10-17T22:03:25Z

Weird. Thanks for confirming, @arod0214. I'll restart that test; if it fails again, let's see what @apb7 has to say about it (since he looks after the team's dev workflow process).

seanlip · 2018-10-18T16:09:23Z

Hi @apb7 -- the linter check failed again. Could you please help? Thanks!

apb7 · 2018-10-18T16:40:25Z

Looking into this. Thanks for the ping, Sean!

…

On Thu, Oct 18, 2018, 9:39 PM Sean Lip ***@***.***> wrote: Hi @apb7 <https://github.com/apb7> -- the test failed again. Could you please help? Thanks! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#5705 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AXrQuWI74VaVPKwIYDXniyMa3tBJACwLks5umKe-gaJpZM4W9oyn> .

apb7

@arod0214 ptal!

core/templates/dev/head/pages/topic_editor/TopicEditorNavbarDirective.js

seanlip · 2018-10-18T20:19:01Z

Also @apb7 is there a way to figure out this error from looking at the Travis logs? If not could we make this possible? I think that's pretty important.

apb7 · 2018-10-18T20:53:54Z

@seanlip, @dawsoneliasen is working on this, #5748. I will ping him on that PR's thread once. Thanks!

…rective.js

arod0214 · 2018-10-18T22:15:11Z

All done. Let me know if it is all good now.

seanlip · 2018-10-19T01:28:15Z

It looks great. Thanks @arod0214! Congrats again on your first PR to Oppia (properly, this time)!

arod0214 added 7 commits September 27, 2018 16:12

oppia project 2 oppia#5671

0e8b8cf

oppia project 2 oppia#5671

75140b4

oppia project 2 oppia#5671

6721f0b

oppia project 2 oppia#5671

b3b6432

oppia project 2 oppia#5671

21a2f97

oppia project 2 oppia#5671

9c0e672

oppia project 2 oppia#5671

f0717a4

seanlip requested a review from aks681 September 28, 2018 04:58

seanlip assigned aks681 Sep 28, 2018

aks681 reviewed Sep 28, 2018

View reviewed changes

seanlip reviewed Sep 29, 2018

View reviewed changes

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

kevinlee12 assigned arod0214 and unassigned aks681 Oct 2, 2018

requested changes

fe2455f

resolving merge conflicts

b853fe1

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

seanlip requested changes Oct 5, 2018

View reviewed changes

seanlip mentioned this pull request Oct 7, 2018

Fix part of #4374: Comment gets methods in schema_utils.py #5665

Closed

6 tasks

oppiabot bot added the stale label Oct 15, 2018

seanlip reviewed Oct 16, 2018

View reviewed changes

core/templates/dev/head/pages/topic_editor/TopicEditorNavbarDirective.js Outdated Show resolved Hide resolved

seanlip added 4 commits October 16, 2018 17:48

Update core/templates/dev/head/pages/topic_editor/TopicEditorNavbarDi…

774854f

…rective.js

Update core/templates/dev/head/pages/topic_editor/TopicEditorNavbarDi…

20b6c79

…rective.js

Update core/templates/dev/head/pages/topic_editor/TopicEditorNavbarDi…

4752977

…rective.js

Update core/templates/dev/head/pages/topic_editor/TopicEditorNavbarDi…

390d7ad

…rective.js

seanlip reviewed Oct 16, 2018

View reviewed changes

core/templates/dev/head/pages/topics_and_skills_dashboard/SkillsListDirective.js Outdated Show resolved Hide resolved

Update core/templates/dev/head/pages/topics_and_skills_dashboard/Skil…

5e6f16a

…lsListDirective.js

seanlip approved these changes Oct 16, 2018

View reviewed changes

seanlip added the PR: LGTM label Oct 16, 2018

apb7 reviewed Oct 18, 2018

View reviewed changes

core/templates/dev/head/pages/topic_editor/TopicEditorNavbarDirective.js Outdated Show resolved Hide resolved

seanlip reviewed Oct 18, 2018

View reviewed changes

core/templates/dev/head/pages/topic_editor/TopicEditorNavbarDirective.js Outdated Show resolved Hide resolved

seanlip reviewed Oct 18, 2018

View reviewed changes

core/templates/dev/head/pages/topic_editor/TopicEditorNavbarDirective.js Outdated Show resolved Hide resolved

seanlip reviewed Oct 18, 2018

View reviewed changes

core/templates/dev/head/pages/topic_editor/TopicEditorNavbarDirective.js Outdated Show resolved Hide resolved

seanlip added 4 commits October 18, 2018 18:12

Update core/templates/dev/head/pages/topic_editor/TopicEditorNavbarDi…

2b72332

…rective.js

Update core/templates/dev/head/pages/topic_editor/TopicEditorNavbarDi…

29df92f

…rective.js

Update core/templates/dev/head/pages/topic_editor/TopicEditorNavbarDi…

30a4d54

…rective.js

Update core/templates/dev/head/pages/topic_editor/TopicEditorNavbarDi…

3253839

…rective.js

seanlip merged commit b0880a8 into oppia:develop Oct 19, 2018

Project 2 Issue #5671 #5705

Project 2 Issue #5671 #5705

Conversation

arod0214 commented Sep 27, 2018 • edited by seanlip Loading

Explanation

Checklist

aks681 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oppiabot bot commented Sep 30, 2018

arod0214 commented Oct 4, 2018 via email

seanlip commented Oct 4, 2018

seanlip commented Oct 4, 2018

arod0214 commented Oct 4, 2018 via email

seanlip commented Oct 4, 2018 • edited Loading

arod0214 commented Oct 4, 2018 via email

seanlip commented Oct 4, 2018

codecov-io commented Oct 5, 2018 • edited Loading

Codecov Report

seanlip 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

oppiabot bot commented Oct 15, 2018

arod0214 commented Oct 16, 2018

seanlip left a comment

Choose a reason for hiding this comment

seanlip left a comment

Choose a reason for hiding this comment

arod0214 commented Oct 16, 2018

seanlip commented Oct 17, 2018

arod0214 commented Oct 17, 2018 via email

seanlip commented Oct 17, 2018

seanlip commented Oct 18, 2018 • edited Loading

apb7 commented Oct 18, 2018 via email

apb7 left a comment

Choose a reason for hiding this comment

seanlip commented Oct 18, 2018

apb7 commented Oct 18, 2018

arod0214 commented Oct 18, 2018

seanlip commented Oct 19, 2018

arod0214 commented Sep 27, 2018 •

edited by seanlip

Loading

seanlip commented Oct 4, 2018 •

edited

Loading

codecov-io commented Oct 5, 2018 •

edited

Loading

seanlip commented Oct 18, 2018 •

edited

Loading