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.

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

Create a subtopic page viewer #7354

Merged
merged 21 commits into from
Aug 12, 2019
Merged

Conversation

aks681
Copy link
Contributor

@aks681 aks681 commented Aug 10, 2019

This PR refines the UI of the topic viewer to some extent and creates the subtopic page viewer, which can be accessed from the topic viewer.

Screenshot (an example with all the RTE elements):
image

@oppiabot
Copy link

oppiabot bot commented Aug 10, 2019

Hi, @aks681. This pull request does not have a "CHANGELOG: ..." label as mentioned in the PR checkbox list. Please add this label. PRs without this label will not be merged. If you are unsure of which label to add, please ask the reviewers for guidance. Thanks!

@oppiabot
Copy link

oppiabot bot commented Aug 10, 2019

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

@seanlip
Copy link
Member

seanlip commented Aug 10, 2019

Hm. @aks681, can you put the content of the subtopic page on a white card please? It looks quite awkward on the grey background.

(Also please note merge conflict.)

Copy link
Member

@seanlip seanlip left a comment

Choose a reason for hiding this comment

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

Thanks @aks681, just a couple quick comments, will take another pass later!

class SubtopicPageDataHandler(base.BaseHandler):
"""Manages the data that needs to be displayed to a learner on the
subtopic page.
"""
GET_HANDLER_ERROR_RETURN_TYPE = feconf.HANDLER_TYPE_JSON

@acl_decorators.can_access_subtopic_viewer_page
def get(self, topic_id, subtopic_id):
def get(self, topic_name, subtopic_id):
Copy link
Member

Choose a reason for hiding this comment

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

Just checking -- why the switch?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since this page is accessed from the topic viewer, which has its url as /topic/<topicName>, it felt right to have the url for the subtopic pages also as /subtopic/<topicName>/<subtopicId>

"I18N_TOPIC_VIEWER_PRACTICE": "Title shown in a tab header in the topic's viewer page. - Clicking on this tab header shows a list of the subtopics that lie within the topic.\n{{Identical|Practice}}",
"I18N_TOPIC_VIEWER_STORY": "Title shown in a tab header in the topic's viewer page. - Clicking on this tab header shows a list of the stories that lie within the topic.\n{{Identical|Story}}",
"I18N_TOPIC_VIEWER_PLAY": "Title shown in a tab header in the topic's viewer page. - Clicking on this tab header shows a list of the stories that lie within the topic.\n{{Identical|Play}}",
"I18N_TOPIC_VIEWER_REVIEW": "Title shown in a tab header in the topic's viewer page. - Clicking on this tab header shows a list of the practice questions that can be used to review the topic.\n{{Identical|Review}}",
Copy link
Member

Choose a reason for hiding this comment

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

I think you swapped Review and Train accidentally. Train is for the practice questions, and Review is for the subtopic page. (Don't use "review" in the Train description, to avoid confusion.)

Also, why do you ned the "Identical" tags at the end?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was there in the previous items and so I carried it over, will remove it,

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

@aks681
Copy link
Contributor Author

aks681 commented Aug 10, 2019

Changed the viewer to have a white background and updated screenshot.

Copy link
Member

@seanlip seanlip left a comment

Choose a reason for hiding this comment

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

Thanks @aks681! Did a full pass.

assets/i18n/qqq.json Outdated Show resolved Hide resolved
core/controllers/acl_decorators.py Outdated Show resolved Hide resolved
core/controllers/resources.py Outdated Show resolved Hide resolved
core/controllers/subtopic_viewer.py Show resolved Hide resolved
class SubtopicPageDataHandler(base.BaseHandler):
"""Manages the data that needs to be displayed to a learner on the
subtopic page.
"""
GET_HANDLER_ERROR_RETURN_TYPE = feconf.HANDLER_TYPE_JSON

@acl_decorators.can_access_subtopic_viewer_page
def get(self, topic_id, subtopic_id):
def get(self, topic_name, subtopic_id):
"""Handles GET requests."""
Copy link
Member

Choose a reason for hiding this comment

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

Add args section to docstring. In this case I think it's important because you want to mention that the given subtopic ID must exist in the subtopic.

Alternatively, take a belt-and-suspenders approach and, in this method, handle the case of the subtopic not existing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will add the docstring, though the check for the existence of the subtopic is done in the decorator itself.

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 (adding docstring).

@@ -167,8 +167,9 @@ angular.module('oppia').factory('UrlInterpolationService', [
* Given an story id returns the complete url path to that image.
*/
getStoryUrl: function(storyId) {
validateResourcePath(storyId);
return '/story' + storyId;
var returnUrl = '/story/' + storyId;
Copy link
Member

Choose a reason for hiding this comment

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

Consider instead calling interpolateUrl() here, with the appropriate URL template. I think it encodes URI components.

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, deleted this function and created a new URL template that can be used with interpolateUrl anywhere,

SubtopicViewerBackendApiService, UrlService, FATAL_ERROR_CODES) {
var ctrl = this;

ctrl.checkMobileView = function() {
Copy link
Member

Choose a reason for hiding this comment

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

Use a suitable method in WindowDimensionsService for things like this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Used WindowDimensionsService.getWidth inside this.

return;
}

var windowWidth = $(window).width();
Copy link
Member

Choose a reason for hiding this comment

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

Don't just copy code wholesale from the library page -- pull out common code into a common directive and reuse it here. (Remember the DRY principle.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created issue #7358.

@codecov
Copy link

codecov bot commented Aug 11, 2019

Codecov Report

Merging #7354 into develop will decrease coverage by 11.49%.
The diff coverage is 95.38%.

@@             Coverage Diff              @@
##           develop    #7354       +/-   ##
============================================
- Coverage       83%   71.51%   -11.49%     
============================================
  Files         1064      866      -198     
  Lines        57131    34097    -23034     
  Branches      2660     2664        +4     
============================================
- Hits         47421    24384    -23037     
- Misses        9066     9067        +1     
- Partials       644      646        +2
Flag Coverage Δ
#backend ?
#frontend 71.51% <95.38%> (+0.03%) ⬆️
Impacted Files Coverage Δ
core/templates/dev/head/app.constants.ts 100% <ø> (ø) ⬆️
...ad/domain/utilities/UrlInterpolationServiceSpec.ts 100% <ø> (ø) ⬆️
...v/head/domain/utilities/UrlInterpolationService.ts 90.54% <100%> (+0.13%) ⬆️
...opic_viewer/SubtopicViewerBackendApiServiceSpec.ts 100% <100%> (ø)
core/templates/dev/head/services/ContextService.ts 77.11% <100%> (-1.9%) ⬇️
...tes/dev/head/services/contextual/UrlServiceSpec.ts 100% <100%> (ø) ⬆️
...ubtopic_viewer/subtopic-viewer-domain.constants.ts 100% <100%> (ø)
...mplates/dev/head/services/contextual/UrlService.ts 100% <100%> (ø) ⬆️
...subtopic_viewer/SubtopicViewerBackendApiService.ts 80% <80%> (ø)
... and 200 more

@codecov
Copy link

codecov bot commented Aug 11, 2019

Codecov Report

Merging #7354 into develop will increase coverage by 0.03%.
The diff coverage is 98.54%.

@@             Coverage Diff             @@
##           develop    #7354      +/-   ##
===========================================
+ Coverage    83.06%   83.09%   +0.03%     
===========================================
  Files         1064     1068       +4     
  Lines        57708    57822     +114     
  Branches      2723     2727       +4     
===========================================
+ Hits         47931    48043     +112     
  Misses        9082     9082              
- Partials       695      697       +2
Flag Coverage Δ
#backend 99.99% <100%> (ø) ⬆️
#frontend 71.82% <97.96%> (+0.06%) ⬆️
Impacted Files Coverage Δ
core/templates/dev/head/app.constants.ts 100% <ø> (ø) ⬆️
...ad/domain/utilities/UrlInterpolationServiceSpec.ts 100% <ø> (ø) ⬆️
...v/head/domain/utilities/UrlInterpolationService.ts 90.14% <ø> (-0.27%) ⬇️
.../topic_viewer/topic-viewer-domain.constants.ajs.ts 100% <ø> (ø) ⬆️
main.py 100% <ø> (ø) ⬆️
...pic_viewer/subtopic-viewer-domain.constants.ajs.ts 100% <100%> (ø)
.../templates/dev/head/services/ContextServiceSpec.ts 100% <100%> (ø) ⬆️
core/controllers/acl_decorators.py 100% <100%> (ø) ⬆️
core/controllers/resources.py 100% <100%> (ø) ⬆️
core/controllers/topic_viewer.py 100% <100%> (ø) ⬆️
... and 21 more

Copy link
Member

@seanlip seanlip left a comment

Choose a reason for hiding this comment

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

Just two more small things otherwise LGTM. Thanks!

entity_id: str. The id of the entity.
page_context: str. The context of the page where the asset is
required.
page_identifier: str. The unique identifier for the particular
Copy link
Member

Choose a reason for hiding this comment

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

Please change this to be more exhaustive. In particular: make a list here of all the allowed entity type, and for each, specify what the entity ID represents.

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

entity_id = topic.id
else:
Copy link
Member

Choose a reason for hiding this comment

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

Make this "elif page_context == one of the allowed entity types"

and add an "else throw InvalidInputException(...)" at the end

(and test these)

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

@seanlip
Copy link
Member

seanlip commented Aug 11, 2019

@vojtechjelinek @nithusha21 @ankita240796 @DubeySandeep PTAL as codeowners. (This needs to go in ASAP)

Thanks!

@seanlip
Copy link
Member

seanlip commented Aug 11, 2019

Hi @aks681 PTAL at the code coverage checks, thanks!

Copy link
Contributor

@vojtechjelinek vojtechjelinek left a comment

Choose a reason for hiding this comment

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

LGTM from code owner's perspective. Thanks!

Copy link
Member

@DubeySandeep DubeySandeep left a comment

Choose a reason for hiding this comment

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

@aks681, PR, LGTM! (I've left a few comments PTAL!)

.github/CODEOWNERS Outdated Show resolved Hide resolved
raise self.PageNotFoundException

topic_rights = topic_services.get_topic_rights(topic.id)
if topic_rights is None or not topic_rights.topic_is_published:
Copy link
Member

@DubeySandeep DubeySandeep Aug 11, 2019

Choose a reason for hiding this comment

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

The topic editor or admin cannot see the viewer page until the topic is released/published? (Do we have such preview tab in the subtopic editor page?)

I'm not sure about the viewer page flow so maybe this comment is irrelevant.

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, in the sense that once the topic is published, all subtopic pages in it can be viewed. Since the subtopics just contain an RTE, it doesn't require manual publishing for each one.

core/controllers/subtopic_viewer.py Show resolved Hide resolved
'components/common-layout-directives/common-elements/' +
'background-banner.directive.ts');
require('directives/angular-html-bind.directive.ts');
require('directives/mathjax-bind.directive.ts');
Copy link
Member

Choose a reason for hiding this comment

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

Add a new line between directive and service import.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean directives and domain? Done

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 Aug 12, 2019
Copy link
Contributor

@ankita240796 ankita240796 left a comment

Choose a reason for hiding this comment

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

LGTM from codeowners perspective, thanks @aks681!

@ankita240796 ankita240796 removed their assignment Aug 12, 2019
@ankita240796 ankita240796 merged commit e63191b into oppia:develop Aug 12, 2019
@aks681 aks681 deleted the subtopic-page-viewer branch August 12, 2019 13:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants