diff --git a/.gitattributes b/.gitattributes new file mode 100644 index 000000000000..4c96011ccf05 --- /dev/null +++ b/.gitattributes @@ -0,0 +1,42 @@ +# Source files +# ============ +*.pxd text eol=lf +*.py text eol=lf +*.py3 text eol=lf +*.pyw text eol=lf +*.pyx text eol=lf +*.sh text eol=lf +*.js text eol=lf +*.txt text eol=lf +*.css text eol=lf +*.html text eol=lf +*.md text eol=lf +*.yml text eol=lf +*.yaml text eol=lf +*.md text eol=lf +*.json text eol=lf +*.html text eol=lf +*.txt text eol=lf +*.xml text eol=lf +*.pegjs text eol=lf + +# Misc files +# ========== +.jscsrc text eol=lf +.gitignore text eol=lf +.pylintrc text eol=lf +AUTHORS text eol=lf +CHANGELOG text eol=lf +CONTRIBUTORS text eol=lf +LICENSE text eol=lf +Vagrantfile text eol=lf + + +# Binary files +# ============ +*.db binary +*.p binary +*.pkl binary +*.pyc binary +*.pyd binary +*.pyo binary diff --git a/.travis.yml b/.travis.yml index 9adaadf4e418..e3474732b3ce 100644 --- a/.travis.yml +++ b/.travis.yml @@ -22,14 +22,15 @@ branches: env: matrix: - - RUN_LINT=true - - RUN_FRONTEND_TESTS=true - - RUN_E2E_TESTS_EDITOR=true + - RUN_E2E_TESTS_MAIN_EDITOR=true + - RUN_E2E_TESTS_EDITOR_FEATURES=true - RUN_E2E_TESTS_EXTENSIONS=true - RUN_E2E_TESTS_LIBRARY=true - RUN_E2E_TESTS_MISC=true - RUN_BACKEND_TESTS=true REPORT_BACKEND_COVERAGE=false - RUN_BACKEND_TESTS=true REPORT_BACKEND_COVERAGE=true + - RUN_LINT=true + - RUN_FRONTEND_TESTS=true matrix: allow_failures: # The backend tests, with coverage, take too long to run, so we make this @@ -72,7 +73,8 @@ install: - source scripts/setup_gae.sh || exit 1 script: -- if [ $RUN_E2E_TESTS_EDITOR == 'true' ]; then bash scripts/run_e2e_tests.sh --suite="editor"; fi +- if [ $RUN_E2E_TESTS_MAIN_EDITOR == 'true' ]; then bash scripts/run_e2e_tests.sh --suite="mainEditor"; fi +- if [ $RUN_E2E_TESTS_EDITOR_FEATURES == 'true' ]; then bash scripts/run_e2e_tests.sh --suite="editorFeatures"; fi - if [ $RUN_E2E_TESTS_EXTENSIONS == 'true' ]; then bash scripts/run_e2e_tests.sh --suite="extensions"; fi - if [ $RUN_E2E_TESTS_LIBRARY == 'true' ]; then bash scripts/run_e2e_tests.sh --suite="library"; fi - if [ $RUN_E2E_TESTS_MISC == 'true' ]; then bash scripts/run_e2e_tests.sh --suite="misc"; fi @@ -96,4 +98,4 @@ cache: before_cache: # Delete python bytecode to prevent cache rebuild. - - find third_party -name "*.pyc" -print -delete \ No newline at end of file + - find third_party -name "*.pyc" -print -delete diff --git a/AUTHORS b/AUTHORS index 4736dba50019..e26e65cc215d 100644 --- a/AUTHORS +++ b/AUTHORS @@ -13,10 +13,12 @@ Alex Gower Allan Zhou Andrew Low Andrey Mironyuk +Anmol Shukla Arun Kumar Avijit Gupta <526avijit@gmail.com> Barnabas Makonda Ben Targan +BJ Voth Bolaji Fatade Brenton Briggs Charisse De Torres @@ -36,8 +38,11 @@ Jasper Deng Jaysinh Shukla Jérôme (zolk232) Jerry Chen +John Glennon +Joshua Cano Joshua Lusk Karen Rustad +Kathryn Patterson Kenneth Ho Kerry Wang Kevin Lee @@ -57,6 +62,7 @@ Philip Hayes Prasanna Patil Raine Hoover Rajat Patwa +Rajendra Kadam Reto Brunner Richard Cho Samara Trilling @@ -64,11 +70,14 @@ Santos Hernandez Sanyam Khurana Satwik Kansal Scott Junner +Sebastian Zangaro +Seth Beckman Shafqat Dulal Shouvik Roy Sourav Badami Sreenivasulu Giritheja Tarashish Mishra +Timothy Cyrus Travis Shafer Tuguldur Baigalmaa Umesh Singla diff --git a/CHANGELOG b/CHANGELOG index bd749b4d86a8..e8c1f525003f 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -2,6 +2,92 @@ This file contains a summary of changes to the Oppia code base. For a full chang https://github.com/oppia/oppia/commits/master +v2.3.0 (7 Jun 2016) +------------------- +Site-wide features: +* Add suppport for internationalization. +* Add title and meta descriptions. +* Fix #1845: add a warning if the user has disabled JavaScript. +* Fix #1900: correctly close the sidebar menu on mobile. +* Fix #1842: prevent the profile avatar dropping off the top of the page on small screens. + +Splash/About pages: +* Add new About, Teach and Get Involved pages. +* Reverse the text and update the images in the lower half of the splash page. +* Redirect to the creator dashboard if the user logs in from the splash page. +* Use absolute paths for images on the splash page. + +Library page: +* Add a recently-published group to the library index page. +* Add a top-rated group to the library index page. +* Add a banner and text to the top of the library index page. +* Add a footer to the library index page. +* Filter null explorations from showing up in the library index page. +* Fix #1456: create summary tiles for collections. +* Fix #1860: elongate exploration summary tiles to allow two lines of the exploration title to show. +* Fix #1840: fix the oval around the contributor icons in the exploration summary tiles. +* Remove avatars from the exploration summary tiles due to latency issues. +* Remove the on-hover indicator from the library carousel scroll button. +* Fix #1864: fix carousel behaviour when the user clicks on the scroll button multiple times in rapid succession. +* Fix #1911: fix incorrect carousel shadow height. +* Fix #1918: Prevent the site feedback button from being hidden in the library index page. +* Add new subject icons for Spanish and Gaulish. + +Exploration editor: +* Add functionality for autosaving drafts. +* Allow jumping to a particular state in preview mode. +* Prompt for missing metadata fields just before publication. +* Streamline the saving flow for the various state properties. +* Refactor the parameter metadata computation into a separate service. +* Fix #1877: prevent the 'add new response' modal from exiting when a creator clicks outside it. +* Fix #1829: in the history tab, reduce the space between the committer's username and the exploration version. +* Fix #1894: prevent the category dropdown list from unnecessarily scrolling to the bottom. +* Remove the underline button from the rich-text editor toolbar. +* Remove the "nominate exploration" button from the editor settings page. +* Remove the 'missing objective' warning. + +Collection editor: +* Introduce a new collection editor (currently in beta). + +Creator dashboard: +* Rename 'My Explorations' to 'Creator Dashboard'. +* Fix #1954: add collection tiles to the creator dashboard. +* Fix #1696: introduce a modal for choosing which type of activity to create. +* Fix #1705: remove the old exploration creation modal. + +Learner page: +* Fix #1489: make the learner view responsive. +* Fix #1881: remove unnecessary scroll bar. +* Fix #1814: remove unnecessary button styling and arrows in the tutor card. +* Fix #1815: place the tutor avatar above the supplemental card. +* Fix #1818: show the help card at the bottom of the viewport if the interaction is too tall. +* Fix jerky animation for showing previous answers. +* Fix #1816: fix single-card to double-card animation. +* Fix #1817: center the animation at the Oppia avatar position. + +Interactions: +* ItemSelectionInput: + - Fix #1766: correctly initialize the notEnoughSelections flag. + +Profile page: +* Fix #1793: allow the user to click on their profile image in order to edit it in their Preferences page. +* Fix #1826: show subject interests properly in Firefox. + +Infrastructure and tests: +* Improve application performance by disabling debug data. +* Fix #1540: add tests for detecting bad patterns. +* Add a one-off test case to check the computation of exploration contributor summaries. +* Support setting CHROME_BIN to the Chromium browser path in Unix-based systems. +* Fix #1394: fix Python version check in setup.sh. +* Fix #1848: set the appropriate http_proxy environment variable if the user is on Vagrant. +* Increase the default memory in the Vagrantfile to 2048MB. +* Fix #1836: add a .gitattributes file to set proper EOL terminators. +* Tighten CSRF payload checks to correctly handle empty request data. +* Fix #1757: divide e2e tests into separate suites. +* Fix #1779: split e2e tests into separate suites on Travis-CI. +* Protractor bugfix: scroll to the top of the page before clicking on the state graph. + + v2.2.0 (7 May 2016) ------------------- Overall site design and navigation: @@ -35,7 +121,7 @@ Editor page: * Fix a bug in getEditorTabContext() that resulted in incorrect detection of the editor preview mode. * Fix #1788: correctly align parameter input fields in Firefox. -Learner: +Learner page: * Fix #1497: ensure that the inter-paragraph spacing behaves correctly. * Fix #1678: prevent share links from being squashed in the information card. diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index d8a127a6e90e..2738fa1a1161 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -7,12 +7,12 @@ Thanks for your interest in contributing code or design help to the Oppia projec ## Setting things up 1. Please sign the CLA so that we can accept your contributions. If you're contributing as an individual, use the [individual CLA](https://goo.gl/forms/AttNH80OV0). If your company owns the copyright to your contributions, a company representative should sign the [corporate CLA](https://goo.gl/forms/xDq9gK3Zcv). - 2. Create a new, empty folder called `opensource/` in your computer's home folder. Navigate to it (`cd opensource`), then [fork and clone](https://help.github.com/articles/fork-a-repo/) the Oppia repo so that it gets downloaded into `opensource/oppia`. Then follow the appropriate installation instructions ([Linux](https://github.com/oppia/oppia/wiki/Installing-Oppia-%28Linux%29), [Mac OS](https://github.com/oppia/oppia/wiki/Installing-Oppia-%28Mac-OS%29), [Windows](https://github.com/oppia/oppia/wiki/Installing-Oppia-%28Windows%29)). - 3. Update your GitHub notification settings: + 2. Fill in the [Oppia contributor survey](http://goo.gl/forms/wz1x3bFfpF) to let us know what your interests are. (You can always change your responses later.) + 3. Join the [oppia-dev@](https://groups.google.com/forum/#!forum/oppia-dev) mailing list, and say hi on the [gitter](https://gitter.im/oppia/oppia-chat) chat channel! + 4. While you're waiting for a reply, create a new, empty folder called `opensource/` in your computer's home folder. Navigate to it (`cd opensource`), then [fork and clone](https://help.github.com/articles/fork-a-repo/) the Oppia repo so that it gets downloaded into `opensource/oppia`. Then follow the appropriate installation instructions ([Linux](https://github.com/oppia/oppia/wiki/Installing-Oppia-%28Linux%29), [Mac OS](https://github.com/oppia/oppia/wiki/Installing-Oppia-%28Mac-OS%29), [Windows](https://github.com/oppia/oppia/wiki/Installing-Oppia-%28Windows%29)). + 5. Update your GitHub notification settings: * Go to your settings page (click the Settings option under the profile menu in the top right), then go to 'Notification center' and ensure that everything's as you want it. * Go to the [Oppia repo](https://github.com/oppia/oppia), and click 'Watch' at the top right. Ensure that you're not 'ignoring' the repo, so that you'll be notified when someone replies to a conversation you're part of. - 4. Fill in the [Oppia contributor survey](http://goo.gl/forms/wz1x3bFfpF) to let us know what your interests are. (You can always change your responses later.) - 5. Join the [oppia-dev@](https://groups.google.com/forum/#!forum/oppia-dev) mailing list, and say hi on the [gitter](https://gitter.im/oppia/oppia-chat) chat channel! 6. (Optional) You may also want to set up [automatic auth](https://help.github.com/articles/set-up-git/#next-steps-authenticating-with-github-from-git) so you don't have to type in a username and password each time you commit a change. ## Getting in touch with the team @@ -31,7 +31,7 @@ If you don't have an idea for something to do, try searching the issue tracker f * **[TODO: design (UI/Interaction)](https://github.com/oppia/oppia/labels/TODO%3A%20design%20%28UI%2Finteraction%29)** means that the issue is suitable for new UI designers, interaction designers, artists and writers. In general, it means that UI design help is needed, and the deliverable would usually be a mockup, drawing, prototype or text that conveys what the feature should look like. * **[TODO: design (usability)](https://github.com/oppia/oppia/labels/TODO%3A%20design%20%28usability%29)** means that the issue is suitable for new usability testers. In general, it means that we have one (or more) ideas or designs for a project, and usability testing/research is needed to find out which one to go with. -These issues, also tagged as 'starter projects', are usually local to a small part of the codebase. They tend to be easier, so they give you a chance to get your feet wet without having to understand the entire codebase. If you'd like some help finding an issue to work on, please don't hesitate to reach out to us at the [developers' mailing list](https://groups.google.com/forum/#!forum/oppia-dev) or the [Gitter chat room](https://gitter.im/oppia/oppia-chat). +These issues, also tagged as 'starter projects', are usually local to a small part of the codebase. They tend to be easier, so they give you a chance to get your feet wet without having to understand the entire codebase. If you'd like some help finding an issue to work on, please don't hesitate to reach out to us at the [developers' mailing list](https://groups.google.com/forum/#!forum/oppia-dev) or the [Gitter chat room](https://gitter.im/oppia/oppia-chat)! Also, if the issue you're working on has a grey label of the form "team-name (@team-lead)", please feel free to reach out to that team lead for help or advice on that issue -- they should be able to answer questions about that area of the codebase. For reference, here are descriptions of what the other 'TODO' tags mean: * **[TODO: tech (instructions)](https://github.com/oppia/oppia/labels/TODO%3A%20tech%20%28instructions%29)** means that the overall solution is generally known, but newcomers to the codebase may need additional instructions to be able to implement them. Adding instructions, such as where to make the necessary changes, will help move these issues to the `TODO: code` stage. @@ -43,6 +43,8 @@ For reference, here are descriptions of what the other 'TODO' tags mean: ## Instructions for making a code change +**Working on your first Pull Request?** You can learn how from this free series: [How to Contribute to an Open Source Project on GitHub](https://egghead.io/series/how-to-contribute-to-an-open-source-project-on-github). + *If your change isn't trivial, please [talk to us](https://gitter.im/oppia/oppia-chat) before you start working on it -- this helps avoid duplication of effort, and allows us to offer advice and suggestions. For larger changes, it may be better to first create a short doc outlining a suggested implementation plan, and send it to the dev team for feedback.* Our central development branch is `develop`, which should be clean and ready for release at any time. In general, all changes should be done as feature branches based off of `develop`. (In case you're interested, we mainly use the [Gitflow workflow](https://www.atlassian.com/git/tutorials/comparing-workflows/gitflow-workflow), which also incorporates `master`, `hotfix-` and `release-` branches -- but you don't need to worry about these.) @@ -61,7 +63,7 @@ Here's how to make a one-off code change. (If you're working on a larger feature 3. **Make a commit to your feature branch.** Each commit should be self-contained and have a descriptive commit message that helps other developers understand why the changes were made. * You can refer to relevant issues in the commit message by writing, e.g., "Fixes #105". - * In general, code should be formatted consistently with other code around it; we use Google's [Python](http://google-styleguide.googlecode.com/svn/trunk/pyguide.html) and [JavaScript](https://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml) style guides as a reference. If you use [Sublime Text](http://www.sublimetext.com/), consider installing the SublimeLinter, [SublimeLinter-jscs](https://github.com/SublimeLinter/SublimeLinter-jscs) and [SublimeLinter-pylint](https://github.com/SublimeLinter/SublimeLinter-pylint) plugins, following the instructions on their respective pages. Please also ensure that your code follows [these additional style rules](https://github.com/oppia/oppia/blob/develop/CONTRIBUTING.md#style-rules). + * In general, code should be formatted consistently with other code around it; we use Google's [Python](https://google.github.io/styleguide/pyguide.html) and [JavaScript](https://google.github.io/styleguide/javascriptguide.xml) style guides as a reference. If you use [Sublime Text](http://www.sublimetext.com/), consider installing the SublimeLinter, [SublimeLinter-jscs](https://github.com/SublimeLinter/SublimeLinter-jscs) and [SublimeLinter-pylint](https://github.com/SublimeLinter/SublimeLinter-pylint) plugins, following the instructions on their respective pages. Please also ensure that your code follows [these additional style rules](https://github.com/oppia/oppia/blob/develop/CONTRIBUTING.md#style-rules). * Please ensure that the code you write is well-tested. * Before making the commit, start up a local instance of Oppia and do some manual testing in order to check that you haven't broken anything! After that, ensure that the added code has no lint errors and passes all automated tests by running the presubmit script: @@ -123,6 +125,16 @@ General Python - There should be two empty lines before any top-level class or function definition. +- Prefer string interpolation over concatenation -- e.g. prefer: + + ``` + 'My string %s' % varname + ``` +over + + ``` + 'My string ' + varname + ``` - When indenting from an open parenthesis ('('), prefer indenting by 4 rather than indenting from the position of the parenthesis. For example, prefer: ``` @@ -144,6 +156,12 @@ over the last DUPLICATE_EMAIL_INTERVAL_MINS. """ ``` +- Never use backslashes to end a line. It's hard to tell whether they're escaping newlines, spaces, or something else. Use parentheses instead to break the line up, e.g.: + + ``` + my_variable = ( + my_very_long_module_name.my_really_long_function_name()) + ``` JavaScript - We are moving away from using underscores as prefixes for variable names, so, in the future, use `var localVariable` and not `var _localVariable`. Instead, we are adopting the convention that anything declared using `var` is private to the controller/service/etc. If you want a variable to be accessible to the controller, declare it on $scope instead. diff --git a/CONTRIBUTORS b/CONTRIBUTORS index 4f88ef28d347..2ae5b0507d3f 100644 --- a/CONTRIBUTORS +++ b/CONTRIBUTORS @@ -28,11 +28,13 @@ Amit Deutsch Andrew Low Andrey Mironyuk Angela Park +Anmol Shukla Arun Kumar Avijit Gupta <526avijit@gmail.com> Barnabas Makonda Ben Henning Ben Targan +BJ Voth Bolaji Fatade Brenton Briggs Charisse De Torres @@ -53,8 +55,11 @@ Jaysinh Shukla Jeremy Emerson Jérôme (zolk232) Jerry Chen +John Glennon +Joshua Cano Joshua Lusk Karen Rustad +Kathryn Patterson Kenneth Ho Kerry Wang Kevin Lee @@ -79,6 +84,7 @@ Philip Hayes Prasanna Patil Raine Hoover Rajat Patwa +Rajendra Kadam Reinaldo Aguiar Reto Brunner Richard Cho @@ -88,6 +94,8 @@ Sanyam Khurana Satwik Kansal Scott Junner Sean Lip +Sebastian Zangaro +Seth Beckman Shafqat Dulal Shantanu Bhowmik Shouvik Roy @@ -96,6 +104,7 @@ Sreenivasulu Giritheja Stephanie Federwisch Stephen Chiang Tarashish Mishra +Timothy Cyrus Travis Shafer Tuguldur Baigalmaa Umesh Singla diff --git a/Vagrantfile b/Vagrantfile index 6077535b7b63..251b2de8187f 100644 --- a/Vagrantfile +++ b/Vagrantfile @@ -17,7 +17,7 @@ SCRIPT Vagrant.configure(2) do |config| config.vm.provider "virtualbox" do |v| v.customize ["setextradata", :id, "VBoxInternal2/SharedFoldersEnableSymlinksCreate/v-root", "1"] - v.memory = 1024 + v.memory = 2048 end # General-purpose env var to let scripts know we are in Vagrant. config.vm.provision "shell", inline: 'echo "export VAGRANT=true" >> /etc/profile' diff --git a/app.yaml b/app.yaml index 75307ba25848..cb57b5f61d70 100644 --- a/app.yaml +++ b/app.yaml @@ -1,5 +1,5 @@ application: oppiaserver -version: 2-2-0 +version: 2-3-0 runtime: python27 api_version: 1 threadsafe: false @@ -53,6 +53,11 @@ handlers: http_headers: Cache-Control: 'public, max-age=2592000' Vary: Accept-Encoding +- url: /i18n + static_dir: i18n + secure: always + http_headers: + Cache-Control: 'no-cache' - url: /third_party/generated static_dir: third_party/generated secure: always diff --git a/core/controllers/admin_test.py b/core/controllers/admin_test.py index 807ca7dbb5b0..0bd751ce7e08 100644 --- a/core/controllers/admin_test.py +++ b/core/controllers/admin_test.py @@ -14,7 +14,6 @@ """Tests for the admin page.""" -from core.controllers import editor from core.controllers import pages from core.domain import config_domain from core.tests import test_utils @@ -23,7 +22,6 @@ BOTH_MODERATOR_AND_ADMIN_EMAIL = 'moderator.and.admin@example.com' BOTH_MODERATOR_AND_ADMIN_USERNAME = 'moderatorandadm1n' -SITE_FORUM_URL = 'siteforum.url' class AdminIntegrationTest(test_utils.GenericTestBase): @@ -77,13 +75,13 @@ def test_change_configuration_property(self): response_dict = self.get_json('/adminhandler') response_config_properties = response_dict['config_properties'] self.assertDictContainsSubset({ - 'value': editor.MODERATOR_REQUEST_FORUM_URL_DEFAULT_VALUE, - }, response_config_properties[editor.MODERATOR_REQUEST_FORUM_URL.name]) + 'value': pages.MODERATOR_REQUEST_FORUM_URL_DEFAULT_VALUE, + }, response_config_properties[pages.MODERATOR_REQUEST_FORUM_URL.name]) payload = { 'action': 'save_config_properties', 'new_config_property_values': { - editor.MODERATOR_REQUEST_FORUM_URL.name: ( + pages.MODERATOR_REQUEST_FORUM_URL.name: ( self.UNICODE_TEST_STRING), } } @@ -93,16 +91,18 @@ def test_change_configuration_property(self): response_config_properties = response_dict['config_properties'] self.assertDictContainsSubset({ 'value': self.UNICODE_TEST_STRING, - }, response_config_properties[editor.MODERATOR_REQUEST_FORUM_URL.name]) + }, response_config_properties[pages.MODERATOR_REQUEST_FORUM_URL.name]) self.logout() def test_change_about_page_config_property(self): """Test that the correct variables show up on the about page.""" + new_contact_email = 'test2@example.com' + # Navigate to the about page. The site name is not set. response = self.testapp.get('/about') - self.assertIn('https://site/forum/url', response.body) - self.assertNotIn(SITE_FORUM_URL, response.body) + self.assertIn('CONTACT_EMAIL_ADDRESS', response.body) + self.assertNotIn(new_contact_email, response.body) self.login(self.ADMIN_EMAIL, is_super_admin=True) response = self.testapp.get('/admin') @@ -110,15 +110,15 @@ def test_change_about_page_config_property(self): self.post_json('/adminhandler', { 'action': 'save_config_properties', 'new_config_property_values': { - pages.SITE_FORUM_URL.name: SITE_FORUM_URL + pages.CONTACT_EMAIL_ADDRESS.name: new_contact_email } }, csrf_token) self.logout() # Navigate to the splash page. The site name is set. response = self.testapp.get('/about') - self.assertNotIn('https://site/forum/url', response.body) - self.assertIn(SITE_FORUM_URL, response.body) + self.assertNotIn('CONTACT_EMAIL_ADDRESS', response.body) + self.assertIn(new_contact_email, response.body) def test_change_rights(self): """Test that the correct role indicators show up on app pages.""" diff --git a/core/controllers/base.py b/core/controllers/base.py index 5ff6d5ea1007..83a66e52d776 100644 --- a/core/controllers/base.py +++ b/core/controllers/base.py @@ -255,6 +255,11 @@ class BaseHandler(webapp2.RequestHandler): # TODO(sll): A weakness of the current approach is that the source and # destination page names have to be the same. Consider fixing this. PAGE_NAME_FOR_CSRF = '' + # Whether the page includes a button for creating explorations. If this is + # set to True, a CSRF token for that button will be generated. This is + # needed because "create exploration" requests can come from multiple + # pages. + PAGE_HAS_CREATE_EXP_REQUEST = False # Whether to redirect requests corresponding to a logged-in user who has # not completed signup in to the signup page. This ensures that logged-in # users have agreed to the latest terms. @@ -280,6 +285,7 @@ def __init__(self, request, response): # pylint: disable=super-init-not-called self.has_seen_editor_tutorial = False self.partially_logged_in = False self.values['profile_picture_data_url'] = None + self.preferred_site_language_code = None if self.user_id: email = current_user_services.get_user_email(self.user) @@ -294,6 +300,8 @@ def __init__(self, request, response): # pylint: disable=super-init-not-called self.user_id = None else: self.username = user_settings.username + self.preferred_site_language_code = ( + user_settings.preferred_site_language_code) self.values['username'] = self.username self.values['profile_picture_data_url'] = ( user_settings.profile_picture_data_url) @@ -328,7 +336,7 @@ def dispatch(self): self.redirect(users.create_logout_url(self.request.uri)) return - if self.payload and self.REQUIRE_PAYLOAD_CSRF_CHECK: + if self.payload is not None and self.REQUIRE_PAYLOAD_CSRF_CHECK: try: if not self.PAGE_NAME_FOR_CSRF: raise Exception('No CSRF page name specified for this ' @@ -428,10 +436,12 @@ def render_template( SIDEBAR_MENU_ADDITIONAL_LINKS.value), 'SITE_FEEDBACK_FORM_URL': SITE_FEEDBACK_FORM_URL.value, 'SITE_NAME': SITE_NAME.value, + 'SUPPORTED_SITE_LANGUAGES': feconf.SUPPORTED_SITE_LANGUAGES, 'SOCIAL_MEDIA_BUTTONS': SOCIAL_MEDIA_BUTTONS.value, 'SYSTEM_USERNAMES': feconf.SYSTEM_USERNAMES, 'user_is_logged_in': user_services.has_fully_registered( self.user_id), + 'preferred_site_language_code': self.preferred_site_language_code }) if 'meta_name' not in values: @@ -449,16 +459,28 @@ def render_template( current_user_services.create_logout_url( redirect_url_on_logout)) else: + target_url = ( + '/' if self.request.uri.endswith(feconf.SPLASH_URL) + else self.request.uri) values['login_url'] = ( - current_user_services.create_login_url(self.request.uri)) + current_user_services.create_login_url(target_url)) # Create a new csrf token for inclusion in HTML responses. This assumes # that tokens generated in one handler will be sent back to a handler # with the same page name. values['csrf_token'] = '' + values['csrf_token_create_exploration'] = '' + values['csrf_token_i18n'] = ( + CsrfTokenManager.create_csrf_token( + self.user_id, feconf.CSRF_PAGE_NAME_I18N)) + if self.REQUIRE_PAYLOAD_CSRF_CHECK and self.PAGE_NAME_FOR_CSRF: values['csrf_token'] = CsrfTokenManager.create_csrf_token( self.user_id, self.PAGE_NAME_FOR_CSRF) + if self.PAGE_HAS_CREATE_EXP_REQUEST: + values['csrf_token_create_exploration'] = ( + CsrfTokenManager.create_csrf_token( + self.user_id, feconf.CSRF_PAGE_NAME_CREATE_EXPLORATION)) self.response.cache_control.no_cache = True self.response.cache_control.must_revalidate = True diff --git a/core/controllers/base_test.py b/core/controllers/base_test.py index 811b05b108ce..6dc987fecfd8 100644 --- a/core/controllers/base_test.py +++ b/core/controllers/base_test.py @@ -17,6 +17,8 @@ """Tests for generic controller behavior.""" import datetime +import json +import os import re import types @@ -26,6 +28,7 @@ from core.tests import test_utils import feconf import main +import utils import webapp2 import webtest @@ -103,7 +106,7 @@ def test_redirect_in_both_logged_in_and_logged_out_states(self): self.login('user@example.com') response = self.testapp.get('/') self.assertEqual(response.status_int, 302) - self.assertIn('my_explorations', response.headers['location']) + self.assertIn('dashboard', response.headers['location']) self.logout() @@ -200,8 +203,7 @@ class FakeAboutPage(base.BaseHandler): def get(self): """Handles GET requests.""" self.values.update({ - 'CONTACT_EMAIL_ADDRESS': ['<[angular_tag]>'], - 'SITE_FORUM_URL': 'x{{51 * 3}}y', + 'CONTACT_EMAIL_ADDRESS': ['<[angular_tag]> x{{51 * 3}}y'], }) self.render_template('pages/about.html') @@ -253,3 +255,25 @@ def test_logout_page(self): self.assertTrue( datetime.datetime.utcnow() > datetime.datetime.strptime( expiry_date[1], '%a, %d %b %Y %H:%M:%S GMT',)) + + +class I18nDictsTest(test_utils.GenericTestBase): + + def _extract_keys_from_json_file(self, filename): + return sorted(json.loads(utils.get_file_contents( + os.path.join(os.getcwd(), 'i18n', filename) + )).keys()) + + def test_i18n_keys(self): + """Tests that all JSON files in i18n.js have the same set of keys.""" + master_key_list = self._extract_keys_from_json_file('en.json') + self.assertGreater(len(master_key_list), 0) + + filenames = os.listdir(os.path.join(os.getcwd(), 'i18n')) + for filename in filenames: + if filename == 'en.json': + continue + + key_list = self._extract_keys_from_json_file(filename) + # All other JSON files should have a subset of the keys in en.json. + self.assertEqual(len(set(key_list) - set(master_key_list)), 0) diff --git a/core/controllers/collection_editor.py b/core/controllers/collection_editor.py index 74fe6767c47f..4d0f9ac70dca 100644 --- a/core/controllers/collection_editor.py +++ b/core/controllers/collection_editor.py @@ -20,7 +20,9 @@ from core.domain import collection_services from core.domain import config_domain from core.domain import rights_manager +from core.domain import summary_services from core.platform import models +import feconf import utils current_user_services = models.Registry.import_current_user_services() @@ -95,6 +97,8 @@ class CollectionEditorHandler(base.BaseHandler): class CollectionEditorPage(CollectionEditorHandler): """The editor page for a single collection.""" + PAGE_HAS_CREATE_EXP_REQUEST = True + # TODO(bhenning): Implement read-only version of the editor. Until that # exists, ensure the user has proper permission to edit this collection # before seeing the editor. @@ -118,13 +122,20 @@ def get(self, collection_id): rights_manager.ACTIVITY_TYPE_COLLECTION, collection_id)) self.values.update({ - 'is_private': rights_manager.is_collection_private(collection_id), 'can_edit': can_edit, - 'collection_id': collection.id, - 'title': collection.title, 'can_unpublish': rights_manager.Actor( self.user_id).can_unpublish( - rights_manager.ACTIVITY_TYPE_COLLECTION, collection_id) + rights_manager.ACTIVITY_TYPE_COLLECTION, collection_id), + 'collection_id': collection.id, + 'is_private': rights_manager.is_collection_private(collection_id), + 'nav_mode': feconf.NAV_MODE_CREATE, + 'title': collection.title, + 'SHOW_COLLECTION_NAVIGATION_TAB_HISTORY': ( + feconf.SHOW_COLLECTION_NAVIGATION_TAB_HISTORY), + 'SHOW_COLLECTION_NAVIGATION_TAB_FEEDBACK': ( + feconf.SHOW_COLLECTION_NAVIGATION_TAB_FEEDBACK), + 'SHOW_COLLECTION_NAVIGATION_TAB_STATS': ( + feconf.SHOW_COLLECTION_NAVIGATION_TAB_STATS) }) self.render_template('collection_editor/collection_editor.html') @@ -165,7 +176,7 @@ def put(self, collection_id): # Retrieve the updated collection. collection_dict = ( - collection_services.get_learner_collection_dict_by_id( + summary_services.get_learner_collection_dict_by_id( collection_id, self.user_id, allow_invalid_explorations=True)) # Send the updated collection back to the frontend. @@ -212,4 +223,3 @@ def put(self, collection_id): 'rights': rights_manager.get_collection_rights( collection_id).to_dict() }) - diff --git a/core/controllers/collection_viewer.py b/core/controllers/collection_viewer.py index 36d8ccb966d1..bde2a101da99 100644 --- a/core/controllers/collection_viewer.py +++ b/core/controllers/collection_viewer.py @@ -18,6 +18,7 @@ from core.domain import collection_services from core.domain import config_domain from core.domain import rights_manager +from core.domain import summary_services from core.platform import models import utils @@ -86,7 +87,7 @@ def get(self, collection_id): try: collection_dict = ( - collection_services.get_learner_collection_dict_by_id( + summary_services.get_learner_collection_dict_by_id( collection_id, self.user_id, allow_invalid_explorations=allow_invalid_explorations)) except Exception as e: diff --git a/core/controllers/collection_viewer_test.py b/core/controllers/collection_viewer_test.py index cf76bbf765fe..b7c2b3e26998 100644 --- a/core/controllers/collection_viewer_test.py +++ b/core/controllers/collection_viewer_test.py @@ -146,7 +146,7 @@ def test_welcome_collection(self): playthrough_dict = collection_dict['playthrough_dict'] self.assertEqual( - playthrough_dict['next_exploration_ids'], ['13', '4', '14']) + playthrough_dict['next_exploration_ids'], ['13']) self.assertEqual(playthrough_dict['completed_exploration_ids'], ['0']) # Completing the 'Solar System' exploration results in no branching. @@ -158,7 +158,7 @@ def test_welcome_collection(self): playthrough_dict = collection_dict['playthrough_dict'] self.assertEqual( - playthrough_dict['next_exploration_ids'], ['4', '14']) + playthrough_dict['next_exploration_ids'], ['4']) self.assertEqual( playthrough_dict['completed_exploration_ids'], ['0', '13']) @@ -172,7 +172,7 @@ def test_welcome_collection(self): playthrough_dict = collection_dict['playthrough_dict'] self.assertEqual( - playthrough_dict['next_exploration_ids'], ['4', '15']) + playthrough_dict['next_exploration_ids'], ['4']) self.assertEqual( playthrough_dict['completed_exploration_ids'], ['0', '13', '14']) diff --git a/core/controllers/home.py b/core/controllers/dashboard.py similarity index 76% rename from core/controllers/home.py rename to core/controllers/dashboard.py index f2305a96c40b..3335c6bf8476 100644 --- a/core/controllers/home.py +++ b/core/controllers/dashboard.py @@ -48,7 +48,8 @@ def get(self): 'You do not have the credentials to access this page.') elif user_services.has_fully_registered(self.user_id): self.values.update({ - 'nav_mode': feconf.NAV_MODE_HOME, + 'meta_description': feconf.DASHBOARD_PAGE_DESCRIPTION, + 'nav_mode': feconf.NAV_MODE_DASHBOARD, }) self.render_template( 'dashboard/notifications_dashboard.html', @@ -106,10 +107,11 @@ def get(self): self.render_json(self.values) -class MyExplorationsPage(base.BaseHandler): - """Page showing the user's explorations.""" +class DashboardPage(base.BaseHandler): + """Page showing the user's creator dashboard.""" PAGE_NAME_FOR_CSRF = 'dashboard' + PAGE_HAS_CREATE_EXP_REQUEST = True @base.require_user def get(self): @@ -118,7 +120,7 @@ def get(self): 'You do not have the credentials to access this page.') elif user_services.has_fully_registered(self.user_id): self.values.update({ - 'nav_mode': feconf.NAV_MODE_HOME, + 'nav_mode': feconf.NAV_MODE_DASHBOARD, 'can_create_collections': ( self.username in config_domain.WHITELISTED_COLLECTION_EDITOR_USERNAMES.value @@ -126,39 +128,46 @@ def get(self): 'allow_yaml_file_upload': ALLOW_YAML_FILE_UPLOAD.value, }) self.render_template( - 'dashboard/my_explorations.html', redirect_url_on_logout='/') + 'dashboard/dashboard.html', redirect_url_on_logout='/') else: self.redirect(utils.set_url_query_parameter( - feconf.SIGNUP_URL, 'return_url', '/my_explorations')) + feconf.SIGNUP_URL, 'return_url', feconf.DASHBOARD_URL)) -class MyExplorationsHandler(base.BaseHandler): - """Provides data for the user's explorations page.""" +class DashboardHandler(base.BaseHandler): + """Provides data for the user's creator dashboard page.""" def get(self): """Handles GET requests.""" if self.user_id is None: raise self.PageNotFoundException - subscribed_summaries = ( - exp_services.get_exploration_summaries_matching_ids( - subscription_services.get_exploration_ids_subscribed_to( - self.user_id))) - def _get_intro_card_color(category): return ( feconf.CATEGORIES_TO_COLORS[category] if category in feconf.CATEGORIES_TO_COLORS else feconf.DEFAULT_COLOR) + subscribed_exploration_summaries = ( + exp_services.get_exploration_summaries_matching_ids( + subscription_services.get_exploration_ids_subscribed_to( + self.user_id))) + subscribed_collection_summaries = ( + collection_services.get_collection_summaries_matching_ids( + subscription_services.get_collection_ids_subscribed_to( + self.user_id))) + explorations_list = [] + collections_list = [] - for exp_summary in subscribed_summaries: + for exp_summary in subscribed_exploration_summaries: if exp_summary is None: continue feedback_thread_analytics = feedback_services.get_thread_analytics( exp_summary.id) + # TODO(sll): Reuse _get_displayable_exp_summary_dicts() in + # summary_services, instead of replicating it like this. explorations_list.append({ 'id': exp_summary.id, 'title': exp_summary.title, @@ -171,7 +180,6 @@ def _get_intro_card_color(category): exp_summary.exploration_model_created_on), 'status': exp_summary.status, 'community_owned': exp_summary.community_owned, - 'is_editable': True, 'thumbnail_icon_url': ( utils.get_thumbnail_icon_url_for_category( exp_summary.category)), @@ -189,8 +197,36 @@ def _get_intro_card_color(category): key=lambda x: (x['num_open_threads'], x['last_updated']), reverse=True) + if (self.username in + config_domain.WHITELISTED_COLLECTION_EDITOR_USERNAMES.value): + for collection_summary in subscribed_collection_summaries: + if collection_summary is None: + continue + + # TODO(sll): Reuse _get_displayable_collection_summary_dicts() + # in summary_services, instead of replicating it like this. + collections_list.append({ + 'id': collection_summary.id, + 'title': collection_summary.title, + 'category': collection_summary.category, + 'objective': collection_summary.objective, + 'language_code': collection_summary.language_code, + 'last_updated': utils.get_time_in_millisecs( + collection_summary.collection_model_last_updated), + 'created_on': utils.get_time_in_millisecs( + collection_summary.collection_model_created_on), + 'status': collection_summary.status, + 'community_owned': collection_summary.community_owned, + 'thumbnail_icon_url': ( + utils.get_thumbnail_icon_url_for_category( + collection_summary.category)), + 'thumbnail_bg_color': utils.get_hex_color_for_category( + collection_summary.category), + }) + self.values.update({ 'explorations_list': explorations_list, + 'collections_list': collections_list, }) self.render_json(self.values) @@ -221,27 +257,16 @@ def get(self): class NewExploration(base.BaseHandler): """Creates a new exploration.""" - PAGE_NAME_FOR_CSRF = 'dashboard' + PAGE_NAME_FOR_CSRF = feconf.CSRF_PAGE_NAME_CREATE_EXPLORATION @base.require_fully_signed_up def post(self): """Handles POST requests.""" - title = self.payload.get('title') - category = self.payload.get('category') - objective = self.payload.get('objective') - language_code = self.payload.get('language_code') - - if not title: - raise self.InvalidInputException('No title supplied.') - if not category: - raise self.InvalidInputException('No category chosen.') - if not language_code: - raise self.InvalidInputException('No language chosen.') + title = self.payload.get('title', feconf.DEFAULT_EXPLORATION_TITLE) new_exploration_id = exp_services.get_new_exploration_id() exploration = exp_domain.Exploration.create_default_exploration( - new_exploration_id, title, category, - objective=objective, language_code=language_code) + new_exploration_id, title=title) exp_services.save_new_exploration(self.user_id, exploration) self.render_json({ @@ -257,19 +282,9 @@ class NewCollection(base.BaseHandler): @base.require_fully_signed_up def post(self): """Handles POST requests.""" - title = self.payload.get('title') - category = self.payload.get('category') - objective = self.payload.get('objective') - # TODO(bhenning): Implement support for language codes in collections. - - if not title: - raise self.InvalidInputException('No title supplied.') - if not category: - raise self.InvalidInputException('No category chosen.') - new_collection_id = collection_services.get_new_collection_id() collection = collection_domain.Collection.create_default_collection( - new_collection_id, title, category, objective=objective) + new_collection_id) collection_services.save_new_collection(self.user_id, collection) self.render_json({ @@ -285,20 +300,12 @@ class UploadExploration(base.BaseHandler): @base.require_fully_signed_up def post(self): """Handles POST requests.""" - title = self.payload.get('title') - category = self.payload.get('category') yaml_content = self.request.get('yaml_file') - if not title: - raise self.InvalidInputException('No title supplied.') - if not category: - raise self.InvalidInputException('No category chosen.') - new_exploration_id = exp_services.get_new_exploration_id() if ALLOW_YAML_FILE_UPLOAD.value: exp_services.save_new_exploration_from_yaml_and_assets( - self.user_id, yaml_content, title, category, - new_exploration_id, []) + self.user_id, yaml_content, new_exploration_id, []) self.render_json({ EXPLORATION_ID_KEY: new_exploration_id }) diff --git a/core/controllers/dashboard_test.py b/core/controllers/dashboard_test.py new file mode 100644 index 000000000000..601748caeb57 --- /dev/null +++ b/core/controllers/dashboard_test.py @@ -0,0 +1,648 @@ +# Copyright 2014 The Oppia Authors. All Rights Reserved. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS-IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +"""Tests for the creator dashboard and the notifications dashboard.""" + +from core.controllers import dashboard +from core.domain import config_services +from core.domain import event_services +from core.domain import feedback_domain +from core.domain import feedback_services +from core.domain import rating_services +from core.domain import rights_manager +from core.domain import stats_jobs_continuous_test +from core.domain import user_jobs_continuous +from core.domain import user_jobs_continuous_test +from core.platform import models +from core.tests import test_utils +import feconf + +(user_models, stats_models) = models.Registry.import_models( + [models.NAMES.user, models.NAMES.statistics]) +taskqueue_services = models.Registry.import_taskqueue_services() + + +class HomePageTest(test_utils.GenericTestBase): + + def test_logged_out_homepage(self): + """Test the logged-out version of the home page.""" + response = self.testapp.get('/') + + self.assertEqual(response.status_int, 302) + self.assertIn('splash', response.headers['location']) + response.follow().mustcontain( + 'I18N_LIBRARY_PAGE_TITLE', 'I18N_SIDEBAR_ABOUT_LINK', + 'I18N_TOPNAV_SIGN_IN', no=['I18N_TOPNAV_LOGOUT']) + + def test_notifications_dashboard_redirects_for_logged_out_users(self): + """Test the logged-out view of the notifications dashboard.""" + response = self.testapp.get('/notifications_dashboard') + self.assertEqual(response.status_int, 302) + # This should redirect to the login page. + self.assertIn('signup', response.headers['location']) + self.assertIn('notifications_dashboard', response.headers['location']) + + self.login('reader@example.com') + response = self.testapp.get('/notifications_dashboard') + # This should redirect the user to complete signup. + self.assertEqual(response.status_int, 302) + self.logout() + + def test_logged_in_notifications_dashboard(self): + """Test the logged-in view of the notifications dashboard.""" + self.signup(self.EDITOR_EMAIL, self.EDITOR_USERNAME) + + self.login(self.EDITOR_EMAIL) + response = self.testapp.get('/notifications_dashboard') + self.assertEqual(response.status_int, 200) + # The string I18N_TOPNAV_SIGN_IN cannot be part of the inner text of + # the tag, but can appear as an attribute value. + response.mustcontain( + 'I18N_TOPNAV_NOTIFICATIONS', 'I18N_TOPNAV_LOGOUT', + self.get_expected_logout_url('/'), + no=['>I18N_TOPNAV_SIGN_IN<', self.get_expected_login_url('/')]) + self.logout() + + +class DashboardStatisticsTest(test_utils.GenericTestBase): + OWNER_EMAIL_1 = 'owner1@example.com' + OWNER_USERNAME_1 = 'owner1' + OWNER_EMAIL_2 = 'owner2@example.com' + OWNER_USERNAME_2 = 'owner2' + + EXP_ID_1 = 'exp_id_1' + EXP_TITLE_1 = 'Exploration title 1' + EXP_ID_2 = 'exp_id_2' + EXP_TITLE_2 = 'Exploration title 2' + + EXP_DEFAULT_VERSION = 1 + + USER_SESSION_ID = 'session1' + USER_IMPACT_SCORE_DEFAULT = 0.0 + + def setUp(self): + super(DashboardStatisticsTest, self).setUp() + self.signup(self.OWNER_EMAIL_1, self.OWNER_USERNAME_1) + self.signup(self.OWNER_EMAIL_2, self.OWNER_USERNAME_2) + + self.owner_id_1 = self.get_user_id_from_email(self.OWNER_EMAIL_1) + self.owner_id_2 = self.get_user_id_from_email(self.OWNER_EMAIL_2) + + def _record_start(self, exp_id, exp_version, state): + """Record start event to an exploration. + Completing the exploration is not necessary here since the total_plays + are currently being counted taking into account only the # of starts. + """ + event_services.StartExplorationEventHandler.record( + exp_id, exp_version, state, self.USER_SESSION_ID, {}, + feconf.PLAY_TYPE_NORMAL) + + def _rate_exploration(self, exp_id, ratings): + """Create num_ratings ratings for exploration with exp_id, + of values from ratings. + """ + # Each user id needs to be unique since each user can only give an + # exploration one rating. + user_ids = ['user%d' % i for i in range(len(ratings))] + self.process_and_flush_pending_tasks() + for ind, user_id in enumerate(user_ids): + rating_services.assign_rating_to_exploration( + user_id, exp_id, ratings[ind]) + self.process_and_flush_pending_tasks() + + def _run_user_stats_aggregator_job(self): + (user_jobs_continuous_test.ModifiedUserStatsAggregator. + start_computation()) + self.assertEqual( + self.count_jobs_in_taskqueue( + queue_name=taskqueue_services.QUEUE_NAME_DEFAULT), + 1) + self.process_and_flush_pending_tasks() + self.assertEqual( + self.count_jobs_in_taskqueue( + queue_name=taskqueue_services.QUEUE_NAME_DEFAULT), + 0) + self.process_and_flush_pending_tasks() + + def _run_stats_aggregator_jobs(self): + (stats_jobs_continuous_test.ModifiedStatisticsAggregator + .start_computation()) + self.assertEqual( + self.count_jobs_in_taskqueue( + queue_name=taskqueue_services.QUEUE_NAME_DEFAULT), + 1) + self.process_and_flush_pending_tasks() + self.assertEqual( + self.count_jobs_in_taskqueue( + queue_name=taskqueue_services.QUEUE_NAME_DEFAULT), + 0) + self.process_and_flush_pending_tasks() + + def test_stats_no_explorations(self): + self.login(self.OWNER_EMAIL_1) + response = self.get_json(feconf.DASHBOARD_DATA_URL) + self.assertEqual(response['explorations_list'], []) + self._run_user_stats_aggregator_job() + self.assertIsNone(user_models.UserStatsModel.get( + self.owner_id_1, strict=False)) + self.logout() + + def test_one_play_for_single_exploration(self): + exploration = self.save_new_default_exploration( + self.EXP_ID_1, self.owner_id_1, title=self.EXP_TITLE_1) + + self.login(self.OWNER_EMAIL_1) + response = self.get_json(feconf.DASHBOARD_DATA_URL) + self.assertEqual(len(response['explorations_list']), 1) + + exp_version = self.EXP_DEFAULT_VERSION + exp_id = self.EXP_ID_1 + state = exploration.init_state_name + + self._record_start(exp_id, exp_version, state) + self._run_stats_aggregator_jobs() + + self._run_user_stats_aggregator_job() + user_model = user_models.UserStatsModel.get(self.owner_id_1) + self.assertEquals(user_model.total_plays, 1) + self.assertEquals( + user_model.impact_score, self.USER_IMPACT_SCORE_DEFAULT) + self.assertIsNone(user_model.average_ratings) + self.logout() + + def test_one_rating_for_single_exploration(self): + self.save_new_default_exploration( + self.EXP_ID_1, self.owner_id_1, title=self.EXP_TITLE_1) + + self.login(self.OWNER_EMAIL_1) + response = self.get_json(feconf.DASHBOARD_DATA_URL) + self.assertEqual(len(response['explorations_list']), 1) + + exp_id = self.EXP_ID_1 + self._rate_exploration(exp_id, [4]) + + self._run_user_stats_aggregator_job() + user_model = user_models.UserStatsModel.get(self.owner_id_1) + self.assertEquals(user_model.total_plays, 0) + self.assertEquals( + user_model.impact_score, self.USER_IMPACT_SCORE_DEFAULT) + self.assertEquals(user_model.average_ratings, 4) + self.logout() + + def test_one_play_and_rating_for_single_exploration(self): + exploration = self.save_new_default_exploration( + self.EXP_ID_1, self.owner_id_1, title=self.EXP_TITLE_1) + + self.login(self.OWNER_EMAIL_1) + response = self.get_json(feconf.DASHBOARD_DATA_URL) + self.assertEqual(len(response['explorations_list']), 1) + + exp_id = self.EXP_ID_1 + + exp_version = self.EXP_DEFAULT_VERSION + state = exploration.init_state_name + + self._record_start(exp_id, exp_version, state) + self._run_stats_aggregator_jobs() + + self._rate_exploration(exp_id, [3]) + + self._run_user_stats_aggregator_job() + user_model = user_models.UserStatsModel.get(self.owner_id_1) + self.assertEquals(user_model.total_plays, 1) + self.assertEquals( + user_model.impact_score, self.USER_IMPACT_SCORE_DEFAULT) + self.assertEquals(user_model.average_ratings, 3) + self.logout() + + def test_multiple_plays_and_ratings_for_single_exploration(self): + exploration = self.save_new_default_exploration( + self.EXP_ID_1, self.owner_id_1, title=self.EXP_TITLE_1) + + self.login(self.OWNER_EMAIL_1) + response = self.get_json(feconf.DASHBOARD_DATA_URL) + self.assertEqual(len(response['explorations_list']), 1) + + exp_version = self.EXP_DEFAULT_VERSION + exp_id = self.EXP_ID_1 + state = exploration.init_state_name + + self._record_start(exp_id, exp_version, state) + self._record_start(exp_id, exp_version, state) + self._record_start(exp_id, exp_version, state) + self._record_start(exp_id, exp_version, state) + self._run_stats_aggregator_jobs() + + self._rate_exploration(exp_id, [3, 4, 5]) + + self._run_user_stats_aggregator_job() + user_model = user_models.UserStatsModel.get(self.owner_id_1) + self.assertEquals(user_model.total_plays, 4) + self.assertEquals( + user_model.impact_score, self.USER_IMPACT_SCORE_DEFAULT) + self.assertEquals(user_model.average_ratings, 4) + self.logout() + + def test_one_play_and_rating_for_multiple_explorations(self): + exploration_1 = self.save_new_default_exploration( + self.EXP_ID_1, self.owner_id_1, title=self.EXP_TITLE_1) + + self.save_new_default_exploration( + self.EXP_ID_2, self.owner_id_1, title=self.EXP_TITLE_2) + + self.login(self.OWNER_EMAIL_1) + response = self.get_json(feconf.DASHBOARD_DATA_URL) + self.assertEqual(len(response['explorations_list']), 2) + + exp_version = self.EXP_DEFAULT_VERSION + exp_id_1 = self.EXP_ID_1 + state_1 = exploration_1.init_state_name + + self._record_start(exp_id_1, exp_version, state_1) + self._run_stats_aggregator_jobs() + + self._rate_exploration(exp_id_1, [4]) + + self._run_user_stats_aggregator_job() + user_model = user_models.UserStatsModel.get(self.owner_id_1) + self.assertEquals(user_model.total_plays, 1) + self.assertEquals( + user_model.impact_score, self.USER_IMPACT_SCORE_DEFAULT) + self.assertEquals(user_model.average_ratings, 4) + self.logout() + + def test_multiple_plays_and_ratings_for_multiple_explorations(self): + exploration_1 = self.save_new_default_exploration( + self.EXP_ID_1, self.owner_id_1, title=self.EXP_TITLE_1) + + exploration_2 = self.save_new_default_exploration( + self.EXP_ID_2, self.owner_id_1, title=self.EXP_TITLE_2) + + self.login(self.OWNER_EMAIL_1) + response = self.get_json(feconf.DASHBOARD_DATA_URL) + self.assertEqual(len(response['explorations_list']), 2) + + exp_version = self.EXP_DEFAULT_VERSION + + exp_id_1 = self.EXP_ID_1 + state_1 = exploration_1.init_state_name + exp_id_2 = self.EXP_ID_2 + state_2 = exploration_2.init_state_name + + self._record_start(exp_id_1, exp_version, state_1) + self._record_start(exp_id_2, exp_version, state_2) + self._record_start(exp_id_2, exp_version, state_2) + self._run_stats_aggregator_jobs() + + self._rate_exploration(exp_id_1, [4]) + self._rate_exploration(exp_id_2, [3]) + + self._run_user_stats_aggregator_job() + user_model = user_models.UserStatsModel.get(self.owner_id_1) + self.assertEquals(user_model.total_plays, 3) + self.assertEquals( + user_model.impact_score, self.USER_IMPACT_SCORE_DEFAULT) + self.assertEquals(user_model.average_ratings, 3.5) + self.logout() + + def test_stats_for_single_exploration_with_multiple_owners(self): + exploration = self.save_new_default_exploration( + self.EXP_ID_1, self.owner_id_1, title=self.EXP_TITLE_1) + + rights_manager.assign_role_for_exploration( + self.owner_id_1, self.EXP_ID_1, self.owner_id_2, + rights_manager.ROLE_OWNER) + + self.login(self.OWNER_EMAIL_1) + response = self.get_json(feconf.DASHBOARD_DATA_URL) + self.assertEqual(len(response['explorations_list']), 1) + + exp_version = self.EXP_DEFAULT_VERSION + exp_id = self.EXP_ID_1 + state = exploration.init_state_name + + self._record_start(exp_id, exp_version, state) + self._record_start(exp_id, exp_version, state) + self._run_stats_aggregator_jobs() + + self._rate_exploration(exp_id, [3, 4, 5]) + self.logout() + + self.login(self.OWNER_EMAIL_2) + response = self.get_json(feconf.DASHBOARD_DATA_URL) + self.assertEqual(len(response['explorations_list']), 1) + + self._rate_exploration(exp_id, [3, 4, 5]) + + self._run_user_stats_aggregator_job() + + user_model_1 = user_models.UserStatsModel.get( + self.owner_id_1) + self.assertEquals(user_model_1.total_plays, 2) + self.assertEquals( + user_model_1.impact_score, self.USER_IMPACT_SCORE_DEFAULT) + self.assertEquals(user_model_1.average_ratings, 4) + + user_model_2 = user_models.UserStatsModel.get( + self.owner_id_2) + self.assertEquals(user_model_2.total_plays, 2) + self.assertEquals( + user_model_2.impact_score, self.USER_IMPACT_SCORE_DEFAULT) + self.assertEquals(user_model_2.average_ratings, 4) + self.logout() + + def test_stats_for_multiple_explorations_with_multiple_owners(self): + exploration_1 = self.save_new_default_exploration( + self.EXP_ID_1, self.owner_id_1, title=self.EXP_TITLE_1) + exploration_2 = self.save_new_default_exploration( + self.EXP_ID_2, self.owner_id_1, title=self.EXP_TITLE_2) + + rights_manager.assign_role_for_exploration( + self.owner_id_1, self.EXP_ID_1, self.owner_id_2, + rights_manager.ROLE_OWNER) + rights_manager.assign_role_for_exploration( + self.owner_id_1, self.EXP_ID_2, self.owner_id_2, + rights_manager.ROLE_OWNER) + + self.login(self.OWNER_EMAIL_2) + response = self.get_json(feconf.DASHBOARD_DATA_URL) + self.assertEqual(len(response['explorations_list']), 2) + + exp_version = self.EXP_DEFAULT_VERSION + + exp_id_1 = self.EXP_ID_1 + state_1 = exploration_1.init_state_name + exp_id_2 = self.EXP_ID_2 + state_2 = exploration_2.init_state_name + + self._record_start(exp_id_1, exp_version, state_1) + self._record_start(exp_id_1, exp_version, state_1) + self._record_start(exp_id_2, exp_version, state_2) + self._record_start(exp_id_2, exp_version, state_2) + self._run_stats_aggregator_jobs() + + self._rate_exploration(exp_id_1, [3]) + self._rate_exploration(exp_id_2, [2]) + + self._run_user_stats_aggregator_job() + user_model_2 = user_models.UserStatsModel.get(self.owner_id_2) + self.assertEquals(user_model_2.total_plays, 4) + self.assertEquals( + user_model_2.impact_score, self.USER_IMPACT_SCORE_DEFAULT) + self.assertEquals(user_model_2.average_ratings, 2.5) + self.logout() + + self.login(self.OWNER_EMAIL_1) + response = self.get_json(feconf.DASHBOARD_DATA_URL) + self.assertEqual(len(response['explorations_list']), 2) + + user_model_1 = user_models.UserStatsModel.get(self.owner_id_1) + self.assertEquals(user_model_1.total_plays, 4) + self.assertEquals( + user_model_1.impact_score, self.USER_IMPACT_SCORE_DEFAULT) + self.assertEquals(user_model_1.average_ratings, 2.5) + self.logout() + +class DashboardHandlerTest(test_utils.GenericTestBase): + + COLLABORATOR_EMAIL = 'collaborator@example.com' + COLLABORATOR_USERNAME = 'collaborator' + + EXP_ID = 'exp_id' + EXP_TITLE = 'Exploration title' + + def setUp(self): + super(DashboardHandlerTest, self).setUp() + self.signup(self.OWNER_EMAIL, self.OWNER_USERNAME) + self.signup(self.COLLABORATOR_EMAIL, self.COLLABORATOR_USERNAME) + self.signup(self.VIEWER_EMAIL, self.VIEWER_USERNAME) + + self.owner_id = self.get_user_id_from_email(self.OWNER_EMAIL) + self.collaborator_id = self.get_user_id_from_email( + self.COLLABORATOR_EMAIL) + self.viewer_id = self.get_user_id_from_email(self.VIEWER_EMAIL) + + def test_no_explorations(self): + self.login(self.OWNER_EMAIL) + response = self.get_json(feconf.DASHBOARD_DATA_URL) + self.assertEqual(response['explorations_list'], []) + self.logout() + + def test_managers_can_see_explorations(self): + self.save_new_default_exploration( + self.EXP_ID, self.owner_id, title=self.EXP_TITLE) + self.set_admins([self.OWNER_USERNAME]) + + self.login(self.OWNER_EMAIL) + response = self.get_json(feconf.DASHBOARD_DATA_URL) + self.assertEqual(len(response['explorations_list']), 1) + self.assertEqual( + response['explorations_list'][0]['status'], + rights_manager.ACTIVITY_STATUS_PRIVATE) + + rights_manager.publish_exploration(self.owner_id, self.EXP_ID) + response = self.get_json(feconf.DASHBOARD_DATA_URL) + self.assertEqual(len(response['explorations_list']), 1) + self.assertEqual( + response['explorations_list'][0]['status'], + rights_manager.ACTIVITY_STATUS_PUBLIC) + + rights_manager.publicize_exploration(self.owner_id, self.EXP_ID) + response = self.get_json(feconf.DASHBOARD_DATA_URL) + self.assertEqual(len(response['explorations_list']), 1) + self.assertEqual( + response['explorations_list'][0]['status'], + rights_manager.ACTIVITY_STATUS_PUBLICIZED) + self.logout() + + def test_collaborators_can_see_explorations(self): + self.save_new_default_exploration( + self.EXP_ID, self.owner_id, title=self.EXP_TITLE) + rights_manager.assign_role_for_exploration( + self.owner_id, self.EXP_ID, self.collaborator_id, + rights_manager.ROLE_EDITOR) + self.set_admins([self.OWNER_USERNAME]) + + self.login(self.COLLABORATOR_EMAIL) + response = self.get_json(feconf.DASHBOARD_DATA_URL) + self.assertEqual(len(response['explorations_list']), 1) + self.assertEqual( + response['explorations_list'][0]['status'], + rights_manager.ACTIVITY_STATUS_PRIVATE) + + rights_manager.publish_exploration(self.owner_id, self.EXP_ID) + response = self.get_json(feconf.DASHBOARD_DATA_URL) + self.assertEqual(len(response['explorations_list']), 1) + self.assertEqual( + response['explorations_list'][0]['status'], + rights_manager.ACTIVITY_STATUS_PUBLIC) + + rights_manager.publicize_exploration(self.owner_id, self.EXP_ID) + response = self.get_json(feconf.DASHBOARD_DATA_URL) + self.assertEqual(len(response['explorations_list']), 1) + self.assertEqual( + response['explorations_list'][0]['status'], + rights_manager.ACTIVITY_STATUS_PUBLICIZED) + + self.logout() + + def test_viewer_cannot_see_explorations(self): + self.save_new_default_exploration( + self.EXP_ID, self.owner_id, title=self.EXP_TITLE) + rights_manager.assign_role_for_exploration( + self.owner_id, self.EXP_ID, self.viewer_id, + rights_manager.ROLE_VIEWER) + self.set_admins([self.OWNER_USERNAME]) + + self.login(self.VIEWER_EMAIL) + response = self.get_json(feconf.DASHBOARD_DATA_URL) + self.assertEqual(response['explorations_list'], []) + + rights_manager.publish_exploration(self.owner_id, self.EXP_ID) + response = self.get_json(feconf.DASHBOARD_DATA_URL) + self.assertEqual(response['explorations_list'], []) + + rights_manager.publicize_exploration(self.owner_id, self.EXP_ID) + response = self.get_json(feconf.DASHBOARD_DATA_URL) + self.assertEqual(response['explorations_list'], []) + self.logout() + + def test_can_see_feedback_thread_counts(self): + self.save_new_default_exploration( + self.EXP_ID, self.owner_id, title=self.EXP_TITLE) + + self.login(self.OWNER_EMAIL) + + response = self.get_json(feconf.DASHBOARD_DATA_URL) + self.assertEqual(len(response['explorations_list']), 1) + self.assertEqual( + response['explorations_list'][0]['num_open_threads'], 0) + self.assertEqual( + response['explorations_list'][0]['num_total_threads'], 0) + + def mock_get_thread_analytics(unused_exploration_id): + return feedback_domain.FeedbackAnalytics(self.EXP_ID, 2, 3) + + with self.swap( + feedback_services, 'get_thread_analytics', + mock_get_thread_analytics): + + response = self.get_json(feconf.DASHBOARD_DATA_URL) + self.assertEqual(len(response['explorations_list']), 1) + self.assertEqual( + response['explorations_list'][0]['num_open_threads'], 2) + self.assertEqual( + response['explorations_list'][0]['num_total_threads'], 3) + + self.logout() + + +class NotificationsDashboardHandlerTest(test_utils.GenericTestBase): + + DASHBOARD_DATA_URL = '/notificationsdashboardhandler/data' + + def setUp(self): + super(NotificationsDashboardHandlerTest, self).setUp() + self.signup(self.VIEWER_EMAIL, self.VIEWER_USERNAME) + self.viewer_id = self.get_user_id_from_email(self.VIEWER_EMAIL) + + def _get_recent_notifications_mock_by_viewer(self, unused_user_id): + """Returns a single feedback thread by VIEWER_ID.""" + return (100000, [{ + 'activity_id': 'exp_id', + 'activity_title': 'exp_title', + 'author_id': self.viewer_id, + 'last_updated_ms': 100000, + 'subject': 'Feedback Message Subject', + 'type': feconf.UPDATE_TYPE_FEEDBACK_MESSAGE, + }]) + + def _get_recent_notifications_mock_by_anonymous_user(self, unused_user_id): + """Returns a single feedback thread by an anonymous user.""" + return (200000, [{ + 'activity_id': 'exp_id', + 'activity_title': 'exp_title', + 'author_id': None, + 'last_updated_ms': 100000, + 'subject': 'Feedback Message Subject', + 'type': feconf.UPDATE_TYPE_FEEDBACK_MESSAGE, + }]) + + def test_author_ids_are_handled_correctly(self): + """Test that author ids are converted into author usernames + and that anonymous authors are handled correctly. + """ + with self.swap( + user_jobs_continuous.DashboardRecentUpdatesAggregator, + 'get_recent_notifications', + self._get_recent_notifications_mock_by_viewer): + + self.login(self.VIEWER_EMAIL) + response = self.get_json(self.DASHBOARD_DATA_URL) + self.assertEqual(len(response['recent_notifications']), 1) + self.assertEqual( + response['recent_notifications'][0]['author_username'], + self.VIEWER_USERNAME) + self.assertNotIn('author_id', response['recent_notifications'][0]) + + with self.swap( + user_jobs_continuous.DashboardRecentUpdatesAggregator, + 'get_recent_notifications', + self._get_recent_notifications_mock_by_anonymous_user): + + self.login(self.VIEWER_EMAIL) + response = self.get_json(self.DASHBOARD_DATA_URL) + self.assertEqual(len(response['recent_notifications']), 1) + self.assertEqual( + response['recent_notifications'][0]['author_username'], '') + self.assertNotIn('author_id', response['recent_notifications'][0]) + + +class CreationButtonsTest(test_utils.GenericTestBase): + + def setUp(self): + super(CreationButtonsTest, self).setUp() + self.signup(self.EDITOR_EMAIL, self.EDITOR_USERNAME) + + def test_new_exploration_ids(self): + """Test generation of exploration ids.""" + self.login(self.EDITOR_EMAIL) + + response = self.testapp.get(feconf.DASHBOARD_URL) + self.assertEqual(response.status_int, 200) + csrf_token = self.get_csrf_token_from_response( + response, token_type=feconf.CSRF_PAGE_NAME_CREATE_EXPLORATION) + exp_a_id = self.post_json( + feconf.NEW_EXPLORATION_URL, {}, csrf_token + )[dashboard.EXPLORATION_ID_KEY] + self.assertEqual(len(exp_a_id), 12) + + self.logout() + + def test_exploration_upload_button(self): + """Test that the exploration upload button appears when appropriate.""" + self.login(self.EDITOR_EMAIL) + + response = self.testapp.get(feconf.DASHBOARD_URL) + self.assertEqual(response.status_int, 200) + response.mustcontain(no=['ng-click="showUploadExplorationModal()"']) + + config_services.set_property( + feconf.SYSTEM_COMMITTER_ID, 'allow_yaml_file_upload', True) + + response = self.testapp.get(feconf.DASHBOARD_URL) + self.assertEqual(response.status_int, 200) + response.mustcontain('ng-click="showUploadExplorationModal()"') + + self.logout() diff --git a/core/controllers/editor.py b/core/controllers/editor.py index cb0da1774418..dac4e97b4d8a 100644 --- a/core/controllers/editor.py +++ b/core/controllers/editor.py @@ -16,6 +16,7 @@ """Controllers for the editor view.""" +import datetime import imghdr import logging @@ -42,6 +43,7 @@ import utils current_user_services = models.Registry.import_current_user_services() +(user_models,) = models.Registry.import_models([models.NAMES.user]) # The frontend template for a new state. It is sent to the frontend when the # exploration editor page is first loaded, so that new states can be @@ -59,14 +61,6 @@ 'unresolved_answers': {}, } -MODERATOR_REQUEST_FORUM_URL_DEFAULT_VALUE = ( - 'https://moderator/request/forum/url') -MODERATOR_REQUEST_FORUM_URL = config_domain.ConfigProperty( - 'moderator_request_forum_url', {'type': 'unicode'}, - 'A link to the forum for nominating explorations to be featured ' - 'in the Oppia library', - default_value=MODERATOR_REQUEST_FORUM_URL_DEFAULT_VALUE) - DEFAULT_TWITTER_SHARE_MESSAGE_EDITOR = config_domain.ConfigProperty( 'default_twitter_share_message_editor', { 'type': 'unicode', @@ -245,7 +239,7 @@ def get(self, exploration_id): interaction_templates), 'interaction_validators_html': jinja2.utils.Markup( interaction_validators_html), - 'moderator_request_forum_url': MODERATOR_REQUEST_FORUM_URL.value, + 'meta_description': feconf.CREATE_PAGE_DESCRIPTION, 'nav_mode': feconf.NAV_MODE_CREATE, 'value_generators_js': jinja2.utils.Markup( get_value_generators_js()), @@ -261,7 +255,7 @@ def get(self, exploration_id): 'TAG_REGEX': feconf.TAG_REGEX, }) - self.render_template('editor/exploration_editor.html') + self.render_template('exploration_editor/exploration_editor.html') class ExplorationHandler(EditorHandler): @@ -269,11 +263,16 @@ class ExplorationHandler(EditorHandler): PAGE_NAME_FOR_CSRF = 'editor' - def _get_exploration_data(self, exploration_id, version=None): + def _get_exploration_data( + self, exploration_id, apply_draft=False, version=None): """Returns a description of the given exploration.""" try: - exploration = exp_services.get_exploration_by_id( - exploration_id, version=version) + if apply_draft: + exploration = exp_services.get_exp_with_draft_applied( + exploration_id, self.user_id) + else: + exploration = exp_services.get_exploration_by_id( + exploration_id, version=version) except: raise self.PageNotFoundException @@ -284,7 +283,15 @@ def _get_exploration_data(self, exploration_id, version=None): stats_services.get_top_unresolved_answers_for_default_rule( exploration_id, state_name)) states[state_name] = state_dict - + exp_user_data = user_models.ExplorationUserDataModel.get( + self.user_id, exploration_id) + draft_changes = (exp_user_data.draft_change_list if exp_user_data + and exp_user_data.draft_change_list else None) + is_version_of_draft_valid = ( + exp_services.is_version_of_draft_valid( + exploration_id, exp_user_data.draft_change_list_exp_version) + if exp_user_data and exp_user_data.draft_change_list_exp_version + else None) editor_dict = { 'category': exploration.category, 'exploration_id': exploration_id, @@ -303,6 +310,8 @@ def _get_exploration_data(self, exploration_id, version=None): 'tags': exploration.tags, 'title': exploration.title, 'version': exploration.version, + 'is_version_of_draft_valid': is_version_of_draft_valid, + 'draft_changes': draft_changes } return editor_dict @@ -312,10 +321,14 @@ def get(self, exploration_id): if not rights_manager.Actor(self.user_id).can_view( rights_manager.ACTIVITY_TYPE_EXPLORATION, exploration_id): raise self.PageNotFoundException - + # 'apply_draft' and 'v'(version) are optional parameters because the + # exploration history tab also uses this handler, and these parameters + # are not used by that tab. version = self.request.get('v', default_value=None) + apply_draft = self.request.get('apply_draft', default_value=False) self.values.update( - self._get_exploration_data(exploration_id, version=version)) + self._get_exploration_data( + exploration_id, apply_draft=apply_draft, version=version)) self.render_json(self.values) @require_editor @@ -604,7 +617,8 @@ def get(self, exploration_id, escaped_state_name): # matched to the classifier individually. answers = stats_services.get_top_state_rule_answers( exploration_id, state_name, [ - exp_domain.DEFAULT_RULESPEC_STR, exp_domain.CLASSIFIER_RULESPEC_STR], + exp_domain.DEFAULT_RULESPEC_STR, + exp_domain.CLASSIFIER_RULESPEC_STR], self.NUMBER_OF_TOP_ANSWERS_PER_RULE) interaction = state.interaction @@ -623,7 +637,8 @@ def get(self, exploration_id, escaped_state_name): trained_answers = set() for answer_group in interaction.answer_groups: for rule_spec in answer_group.rule_specs: - if rule_spec.rule_type == exp_domain.CLASSIFIER_RULESPEC_STR: + if (rule_spec.rule_type == + exp_domain.CLASSIFIER_RULESPEC_STR): trained_answers.update( interaction_instance.normalize_answer(trained) for trained @@ -775,6 +790,7 @@ def post(self, exploration_id): 'Cannot revert to version %s from version %s.' % (revert_to_version, current_version)) + exp_services.discard_draft(exploration_id, self.user_id) exp_services.revert_exploration( self.user_id, exploration_id, current_version, revert_to_version) self.render_json({}) @@ -894,19 +910,9 @@ def post(self, exploration_id): exploration_id) if version != current_exploration.version: - # TODO(sll): Improve this. + # TODO(sll): Improve the handling of merge conflicts. self.render_json({ - 'error': ( - 'Sorry! Someone else has edited and committed changes to ' - 'this exploration while you were editing it. We suggest ' - 'opening another browser tab -- which will load the new ' - 'version of the exploration -- then transferring your ' - 'changes there. We will try to make this easier in the ' - 'future -- we have not done it yet because figuring out ' - 'how to merge different people\'s changes is hard. ' - '(Trying to edit version %s, but the current version is ' - '%s.).' % (version, current_exploration.version) - ) + 'is_version_of_draft_valid': False }) else: utils.recursively_remove_key(change_list, '$$hashKey') @@ -933,3 +939,35 @@ class StartedTutorialEventHandler(EditorHandler): def post(self): """Handles GET requests.""" user_services.record_user_started_state_editor_tutorial(self.user_id) + + +class EditorAutosaveHandler(ExplorationHandler): + """Handles requests from the editor for draft autosave.""" + + @require_editor + def put(self, exploration_id): + """Handles PUT requests for draft updation.""" + # Raise an Exception if the draft change list fails non-strict + # validation. + try: + change_list = self.payload.get('change_list') + version = self.payload.get('version') + exp_services.create_or_update_draft( + exploration_id, self.user_id, change_list, version, + datetime.datetime.utcnow()) + except utils.ValidationError as e: + # We leave any pre-existing draft changes in the datastore. + raise self.InvalidInputException(e) + + # If the value passed here is False, have the user discard the draft + # changes. We save the draft to the datastore even if the version is + # invalid, so that it is available for recovery later. + self.render_json({ + 'is_version_of_draft_valid': exp_services.is_version_of_draft_valid( + exploration_id, version)}) + + @require_editor + def post(self, exploration_id): + """Handles POST request for discarding draft changes.""" + exp_services.discard_draft(exploration_id, self.user_id) + self.render_json({}) diff --git a/core/controllers/editor_test.py b/core/controllers/editor_test.py index 20e26a8422a8..a2524aa635af 100644 --- a/core/controllers/editor_test.py +++ b/core/controllers/editor_test.py @@ -14,10 +14,12 @@ """Tests for the exploration editor page.""" +import datetime import os import StringIO import zipfile +from core.controllers import dashboard from core.controllers import editor from core.domain import config_services from core.domain import event_services @@ -25,9 +27,12 @@ from core.domain import exp_services from core.domain import stats_domain from core.domain import rights_manager +from core.platform import models from core.tests import test_utils import feconf +(user_models,) = models.Registry.import_models([models.NAMES.user]) + class BaseEditorControllerTest(test_utils.GenericTestBase): @@ -75,7 +80,7 @@ def test_editor_page(self): # Check that non-editors can access, but not edit, the editor page. response = self.testapp.get('/create/0') self.assertEqual(response.status_int, 200) - self.assertIn('Welcome to Oppia!', response.body) + self.assertIn('Help others learn new things.', response.body) self.assert_cannot_edit(response.body) # Log in as an editor. @@ -83,7 +88,7 @@ def test_editor_page(self): # Check that it is now possible to access and edit the editor page. response = self.testapp.get('/create/0') - self.assertIn('Welcome to Oppia!', response.body) + self.assertIn('Help others learn new things.', response.body) self.assertEqual(response.status_int, 200) self.assert_can_edit(response.body) self.assertIn('Stats', response.body) @@ -103,6 +108,29 @@ def test_new_state_template(self): new_state_dict['unresolved_answers'] = {} self.assertEqual(new_state_dict, editor.NEW_STATE_TEMPLATE) + def test_that_default_exploration_cannot_be_published(self): + """Test that publishing a default exploration raises an error + due to failing strict validation. + """ + self.login(self.EDITOR_EMAIL) + + response = self.testapp.get(feconf.DASHBOARD_URL) + self.assertEqual(response.status_int, 200) + csrf_token = self.get_csrf_token_from_response( + response, token_type=feconf.CSRF_PAGE_NAME_CREATE_EXPLORATION) + exp_id = self.post_json( + feconf.NEW_EXPLORATION_URL, {}, csrf_token + )[dashboard.EXPLORATION_ID_KEY] + + response = self.testapp.get('/create/%s' % exp_id) + csrf_token = self.get_csrf_token_from_response(response) + rights_url = '%s/%s' % (feconf.EXPLORATION_RIGHTS_PREFIX, exp_id) + self.put_json(rights_url, { + 'is_public': True, + }, csrf_token, expect_errors=True, expected_status_int=400) + + self.logout() + def test_add_new_state_error_cases(self): """Test the error cases for adding a new state to an exploration.""" current_version = 1 @@ -612,7 +640,7 @@ def test_deletion_rights_for_unpublished_exploration(self): """Test rights management for deletion of unpublished explorations.""" unpublished_exp_id = 'unpublished_eid' exploration = exp_domain.Exploration.create_default_exploration( - unpublished_exp_id, 'A title', 'A category') + unpublished_exp_id) exp_services.save_new_exploration(self.owner_id, exploration) rights_manager.assign_role_for_exploration( @@ -641,7 +669,7 @@ def test_deletion_rights_for_published_exploration(self): """Test rights management for deletion of published explorations.""" published_exp_id = 'published_eid' exploration = exp_domain.Exploration.create_default_exploration( - published_exp_id, 'A title', 'A category') + published_exp_id, title='A title', category='A category') exp_services.save_new_exploration(self.owner_id, exploration) rights_manager.assign_role_for_exploration( @@ -1241,3 +1269,167 @@ def test_email_functionality_cannot_be_used_by_non_moderators(self): expected_status_int=401) self.logout() + + +class EditorAutosaveTest(BaseEditorControllerTest): + """Test the handling of editor autosave actions.""" + + EXP_ID1 = '1' + EXP_ID2 = '2' + EXP_ID3 = '3' + NEWER_DATETIME = datetime.datetime.strptime('2017-03-16', '%Y-%m-%d') + OLDER_DATETIME = datetime.datetime.strptime('2015-03-16', '%Y-%m-%d') + DRAFT_CHANGELIST = [{ + 'cmd': 'edit_exploration_property', + 'property_name': 'title', + 'new_value': 'Updated title'}] + NEW_CHANGELIST = [{ + 'cmd': 'edit_exploration_property', + 'property_name': 'title', + 'new_value': 'New title'}] + INVALID_CHANGELIST = [{ + 'cmd': 'edit_exploration_property', + 'property_name': 'title', + 'new_value': 1}] + + def _create_explorations_for_tests(self): + self.save_new_valid_exploration(self.EXP_ID1, self.owner_id) + exploration = exp_services.get_exploration_by_id(self.EXP_ID1) + exploration.add_states(['State A']) + exploration.states['State A'].update_interaction_id('TextInput') + self.save_new_valid_exploration(self.EXP_ID2, self.owner_id) + self.save_new_valid_exploration(self.EXP_ID3, self.owner_id) + + def _create_exp_user_data_model_objects_for_tests(self): + # Explorations with draft set. + user_models.ExplorationUserDataModel( + id='%s.%s' % (self.owner_id, self.EXP_ID1), user_id=self.owner_id, + exploration_id=self.EXP_ID1, + draft_change_list=self.DRAFT_CHANGELIST, + draft_change_list_last_updated=self.NEWER_DATETIME, + draft_change_list_exp_version=1).put() + user_models.ExplorationUserDataModel( + id='%s.%s' % (self.owner_id, self.EXP_ID2), user_id=self.owner_id, + exploration_id=self.EXP_ID2, + draft_change_list=self.DRAFT_CHANGELIST, + draft_change_list_last_updated=self.OLDER_DATETIME, + draft_change_list_exp_version=1).put() + + # Exploration with no draft. + user_models.ExplorationUserDataModel( + id='%s.%s' % (self.owner_id, self.EXP_ID3), user_id=self.owner_id, + exploration_id=self.EXP_ID3).put() + + def setUp(self): + super(EditorAutosaveTest, self).setUp() + self.login(self.OWNER_EMAIL) + self.owner_id = self.get_user_id_from_email(self.OWNER_EMAIL) + self._create_explorations_for_tests() + self._create_exp_user_data_model_objects_for_tests() + + # Generate CSRF token. + response = self.testapp.get('/create/%s' % self.EXP_ID1) + self.csrf_token = self.get_csrf_token_from_response(response) + + def test_exploration_loaded_with_draft_applied(self): + response = self.get_json( + '/createhandler/data/%s' % self.EXP_ID2, {'apply_draft': True}) + # Title updated because chanhe list was applied. + self.assertEqual(response['title'], 'Updated title') + self.assertTrue(response['is_version_of_draft_valid']) + # Draft changes passed to UI. + self.assertEqual(response['draft_changes'], self.DRAFT_CHANGELIST) + + def test_exploration_loaded_without_draft_when_draft_version_invalid(self): + exp_user_data = user_models.ExplorationUserDataModel.get_by_id( + '%s.%s' % (self.owner_id, self.EXP_ID2)) + exp_user_data.draft_change_list_exp_version = 20 + exp_user_data.put() + response = self.get_json( + '/createhandler/data/%s' % self.EXP_ID2, {'apply_draft': True}) + # Title not updated because change list not applied. + self.assertEqual(response['title'], 'A title') + self.assertFalse(response['is_version_of_draft_valid']) + # Draft changes passed to UI even when version is invalid. + self.assertEqual(response['draft_changes'], self.DRAFT_CHANGELIST) + + def test_exploration_loaded_without_draft_as_draft_does_not_exist(self): + response = self.get_json( + '/createhandler/data/%s' % self.EXP_ID3, {'apply_draft': True}) + # Title not updated because change list not applied. + self.assertEqual(response['title'], 'A title') + self.assertIsNone(response['is_version_of_draft_valid']) + # Draft changes None. + self.assertIsNone(response['draft_changes']) + + def test_draft_not_updated_because_newer_draft_exists(self): + payload = { + 'change_list': self.NEW_CHANGELIST, + 'version': 1, + } + response = self.put_json( + '/createhandler/autosave_draft/%s' % self.EXP_ID1, payload, + self.csrf_token) + # Check that draft change list hasn't been updated. + exp_user_data = user_models.ExplorationUserDataModel.get_by_id( + '%s.%s' % (self.owner_id, self.EXP_ID1)) + self.assertEqual( + exp_user_data.draft_change_list, self.DRAFT_CHANGELIST) + self.assertTrue(response['is_version_of_draft_valid']) + + def test_draft_not_updated_validation_error(self): + self.put_json( + '/createhandler/autosave_draft/%s' % self.EXP_ID2, { + 'change_list': self.DRAFT_CHANGELIST, + 'version': 1, + }, self.csrf_token) + response = self.put_json( + '/createhandler/autosave_draft/%s' % self.EXP_ID2, { + 'change_list': self.INVALID_CHANGELIST, + 'version': 2, + }, self.csrf_token, expect_errors=True, expected_status_int=400) + exp_user_data = user_models.ExplorationUserDataModel.get_by_id( + '%s.%s' % (self.owner_id, self.EXP_ID2)) + self.assertEqual( + exp_user_data.draft_change_list, self.DRAFT_CHANGELIST) + self.assertEqual( + response, {'code': 400, + 'error': 'Expected title to be a string, received 1'}) + + def test_draft_updated_version_valid(self): + payload = { + 'change_list': self.NEW_CHANGELIST, + 'version': 1, + } + response = self.put_json( + '/createhandler/autosave_draft/%s' % self.EXP_ID2, payload, + self.csrf_token) + exp_user_data = user_models.ExplorationUserDataModel.get_by_id( + '%s.%s' % (self.owner_id, self.EXP_ID2)) + self.assertEqual(exp_user_data.draft_change_list, self.NEW_CHANGELIST) + self.assertEqual(exp_user_data.draft_change_list_exp_version, 1) + self.assertTrue(response['is_version_of_draft_valid']) + + def test_draft_updated_version_invalid(self): + payload = { + 'change_list': self.NEW_CHANGELIST, + 'version': 10, + } + response = self.put_json( + '/createhandler/autosave_draft/%s' % self.EXP_ID2, payload, + self.csrf_token) + exp_user_data = user_models.ExplorationUserDataModel.get_by_id( + '%s.%s' % (self.owner_id, self.EXP_ID2)) + self.assertEqual(exp_user_data.draft_change_list, self.NEW_CHANGELIST) + self.assertEqual(exp_user_data.draft_change_list_exp_version, 10) + self.assertFalse(response['is_version_of_draft_valid']) + + def test_discard_draft(self): + self.post_json( + '/createhandler/autosave_draft/%s' % self.EXP_ID2, {}, + self.csrf_token) + exp_user_data = user_models.ExplorationUserDataModel.get_by_id( + '%s.%s' % (self.owner_id, self.EXP_ID2)) + self.assertIsNone(exp_user_data.draft_change_list) + self.assertIsNone(exp_user_data.draft_change_list_last_updated) + self.assertIsNone(exp_user_data.draft_change_list_exp_version) diff --git a/core/controllers/feedback.py b/core/controllers/feedback.py index 2b1be47f6272..d8378b7a3993 100644 --- a/core/controllers/feedback.py +++ b/core/controllers/feedback.py @@ -209,3 +209,10 @@ def get(self, exploration_id): self.values.update({'threads': [t.to_dict() for t in threads]}) self.render_json(self.values) + + +class UnsentFeedbackEmailHandler(base.BaseHandler): + """Handler task of sending emails of feedback messages. + This is yet to be implemented.""" + def post(self): + pass diff --git a/core/controllers/feedback_test.py b/core/controllers/feedback_test.py index 1829e39d5918..cdfc072a7e56 100644 --- a/core/controllers/feedback_test.py +++ b/core/controllers/feedback_test.py @@ -246,7 +246,7 @@ def test_post_message_to_existing_thread(self): def test_no_username_shown_for_logged_out_learners(self): new_exp_id = 'new_eid' exploration = exp_domain.Exploration.create_default_exploration( - new_exp_id, 'A title', 'A category') + new_exp_id, title='A title', category='A category') exp_services.save_new_exploration(self.editor_id, exploration) rights_manager.publish_exploration(self.editor_id, new_exp_id) diff --git a/core/controllers/home_test.py b/core/controllers/home_test.py deleted file mode 100644 index 70c57d8ed1c3..000000000000 --- a/core/controllers/home_test.py +++ /dev/null @@ -1,301 +0,0 @@ -# Copyright 2014 The Oppia Authors. All Rights Reserved. -# -# Licensed under the Apache License, Version 2.0 (the "License"); -# you may not use this file except in compliance with the License. -# You may obtain a copy of the License at -# -# http://www.apache.org/licenses/LICENSE-2.0 -# -# Unless required by applicable law or agreed to in writing, software -# distributed under the License is distributed on an "AS-IS" BASIS, -# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -# See the License for the specific language governing permissions and -# limitations under the License. - -"""Tests for the user notification dashboard and 'my explorations' pages.""" - -from core.controllers import home -from core.domain import config_services -from core.domain import feedback_domain -from core.domain import feedback_services -from core.domain import rights_manager -from core.domain import user_jobs_continuous -from core.tests import test_utils -import feconf - - -class HomePageTest(test_utils.GenericTestBase): - - def test_logged_out_homepage(self): - """Test the logged-out version of the home page.""" - response = self.testapp.get('/') - self.assertEqual(response.status_int, 302) - self.assertIn('splash', response.headers['location']) - response.follow().mustcontain( - 'Oppia - Home', 'About', 'Sign in', no=['Logout']) - - def test_notifications_dashboard_redirects_for_logged_out_users(self): - """Test the logged-out view of the notifications dashboard.""" - response = self.testapp.get('/notifications_dashboard') - self.assertEqual(response.status_int, 302) - # This should redirect to the login page. - self.assertIn('signup', response.headers['location']) - self.assertIn('notifications_dashboard', response.headers['location']) - - self.login('reader@example.com') - response = self.testapp.get('/notifications_dashboard') - # This should redirect the user to complete signup. - self.assertEqual(response.status_int, 302) - self.logout() - - def test_logged_in_notifications_dashboard(self): - """Test the logged-in view of the notifications dashboard.""" - self.signup(self.EDITOR_EMAIL, self.EDITOR_USERNAME) - - self.login(self.EDITOR_EMAIL) - response = self.testapp.get('/notifications_dashboard') - self.assertEqual(response.status_int, 200) - response.mustcontain( - 'Notifications', 'Logout', - self.get_expected_logout_url('/'), - no=['Sign in', 'Your personal tutor', - self.get_expected_login_url('/')]) - self.logout() - - -class MyExplorationsHandlerTest(test_utils.GenericTestBase): - - MY_EXPLORATIONS_DATA_URL = '/myexplorationshandler/data' - - COLLABORATOR_EMAIL = 'collaborator@example.com' - COLLABORATOR_USERNAME = 'collaborator' - - EXP_ID = 'exp_id' - EXP_TITLE = 'Exploration title' - - def setUp(self): - super(MyExplorationsHandlerTest, self).setUp() - self.signup(self.OWNER_EMAIL, self.OWNER_USERNAME) - self.signup(self.COLLABORATOR_EMAIL, self.COLLABORATOR_USERNAME) - self.signup(self.VIEWER_EMAIL, self.VIEWER_USERNAME) - - self.owner_id = self.get_user_id_from_email(self.OWNER_EMAIL) - self.collaborator_id = self.get_user_id_from_email( - self.COLLABORATOR_EMAIL) - self.viewer_id = self.get_user_id_from_email(self.VIEWER_EMAIL) - - def test_no_explorations(self): - self.login(self.OWNER_EMAIL) - response = self.get_json(self.MY_EXPLORATIONS_DATA_URL) - self.assertEqual(response['explorations_list'], []) - self.logout() - - def test_managers_can_see_explorations(self): - self.save_new_default_exploration( - self.EXP_ID, self.owner_id, title=self.EXP_TITLE) - self.set_admins([self.OWNER_USERNAME]) - - self.login(self.OWNER_EMAIL) - response = self.get_json(self.MY_EXPLORATIONS_DATA_URL) - self.assertEqual(len(response['explorations_list']), 1) - self.assertEqual( - response['explorations_list'][0]['status'], - rights_manager.ACTIVITY_STATUS_PRIVATE) - - rights_manager.publish_exploration(self.owner_id, self.EXP_ID) - response = self.get_json(self.MY_EXPLORATIONS_DATA_URL) - self.assertEqual(len(response['explorations_list']), 1) - self.assertEqual( - response['explorations_list'][0]['status'], - rights_manager.ACTIVITY_STATUS_PUBLIC) - - rights_manager.publicize_exploration(self.owner_id, self.EXP_ID) - response = self.get_json(self.MY_EXPLORATIONS_DATA_URL) - self.assertEqual(len(response['explorations_list']), 1) - self.assertEqual( - response['explorations_list'][0]['status'], - rights_manager.ACTIVITY_STATUS_PUBLICIZED) - self.logout() - - def test_collaborators_can_see_explorations(self): - self.save_new_default_exploration( - self.EXP_ID, self.owner_id, title=self.EXP_TITLE) - rights_manager.assign_role_for_exploration( - self.owner_id, self.EXP_ID, self.collaborator_id, - rights_manager.ROLE_EDITOR) - self.set_admins([self.OWNER_USERNAME]) - - self.login(self.COLLABORATOR_EMAIL) - response = self.get_json(self.MY_EXPLORATIONS_DATA_URL) - self.assertEqual(len(response['explorations_list']), 1) - self.assertEqual( - response['explorations_list'][0]['status'], - rights_manager.ACTIVITY_STATUS_PRIVATE) - - rights_manager.publish_exploration(self.owner_id, self.EXP_ID) - response = self.get_json(self.MY_EXPLORATIONS_DATA_URL) - self.assertEqual(len(response['explorations_list']), 1) - self.assertEqual( - response['explorations_list'][0]['status'], - rights_manager.ACTIVITY_STATUS_PUBLIC) - - rights_manager.publicize_exploration(self.owner_id, self.EXP_ID) - response = self.get_json(self.MY_EXPLORATIONS_DATA_URL) - self.assertEqual(len(response['explorations_list']), 1) - self.assertEqual( - response['explorations_list'][0]['status'], - rights_manager.ACTIVITY_STATUS_PUBLICIZED) - - self.logout() - - def test_viewer_cannot_see_explorations(self): - self.save_new_default_exploration( - self.EXP_ID, self.owner_id, title=self.EXP_TITLE) - rights_manager.assign_role_for_exploration( - self.owner_id, self.EXP_ID, self.viewer_id, - rights_manager.ROLE_VIEWER) - self.set_admins([self.OWNER_USERNAME]) - - self.login(self.VIEWER_EMAIL) - response = self.get_json(self.MY_EXPLORATIONS_DATA_URL) - self.assertEqual(response['explorations_list'], []) - - rights_manager.publish_exploration(self.owner_id, self.EXP_ID) - response = self.get_json(self.MY_EXPLORATIONS_DATA_URL) - self.assertEqual(response['explorations_list'], []) - - rights_manager.publicize_exploration(self.owner_id, self.EXP_ID) - response = self.get_json(self.MY_EXPLORATIONS_DATA_URL) - self.assertEqual(response['explorations_list'], []) - self.logout() - - def test_can_see_feedback_thread_counts(self): - self.save_new_default_exploration( - self.EXP_ID, self.owner_id, title=self.EXP_TITLE) - - self.login(self.OWNER_EMAIL) - - response = self.get_json(self.MY_EXPLORATIONS_DATA_URL) - self.assertEqual(len(response['explorations_list']), 1) - self.assertEqual( - response['explorations_list'][0]['num_open_threads'], 0) - self.assertEqual( - response['explorations_list'][0]['num_total_threads'], 0) - - def mock_get_thread_analytics(unused_exploration_id): - return feedback_domain.FeedbackAnalytics(self.EXP_ID, 2, 3) - - with self.swap( - feedback_services, 'get_thread_analytics', - mock_get_thread_analytics): - - response = self.get_json(self.MY_EXPLORATIONS_DATA_URL) - self.assertEqual(len(response['explorations_list']), 1) - self.assertEqual( - response['explorations_list'][0]['num_open_threads'], 2) - self.assertEqual( - response['explorations_list'][0]['num_total_threads'], 3) - - self.logout() - - -class NotificationsDashboardHandlerTest(test_utils.GenericTestBase): - - DASHBOARD_DATA_URL = '/notificationsdashboardhandler/data' - - def setUp(self): - super(NotificationsDashboardHandlerTest, self).setUp() - self.signup(self.VIEWER_EMAIL, self.VIEWER_USERNAME) - self.viewer_id = self.get_user_id_from_email(self.VIEWER_EMAIL) - - def _get_recent_notifications_mock_by_viewer(self, unused_user_id): - """Returns a single feedback thread by VIEWER_ID.""" - return (100000, [{ - 'activity_id': 'exp_id', - 'activity_title': 'exp_title', - 'author_id': self.viewer_id, - 'last_updated_ms': 100000, - 'subject': 'Feedback Message Subject', - 'type': feconf.UPDATE_TYPE_FEEDBACK_MESSAGE, - }]) - - def _get_recent_notifications_mock_by_anonymous_user(self, unused_user_id): - """Returns a single feedback thread by an anonymous user.""" - return (200000, [{ - 'activity_id': 'exp_id', - 'activity_title': 'exp_title', - 'author_id': None, - 'last_updated_ms': 100000, - 'subject': 'Feedback Message Subject', - 'type': feconf.UPDATE_TYPE_FEEDBACK_MESSAGE, - }]) - - def test_author_ids_are_handled_correctly(self): - """Test that author ids are converted into author usernames - and that anonymous authors are handled correctly. - """ - with self.swap( - user_jobs_continuous.DashboardRecentUpdatesAggregator, - 'get_recent_notifications', - self._get_recent_notifications_mock_by_viewer): - - self.login(self.VIEWER_EMAIL) - response = self.get_json(self.DASHBOARD_DATA_URL) - self.assertEqual(len(response['recent_notifications']), 1) - self.assertEqual( - response['recent_notifications'][0]['author_username'], - self.VIEWER_USERNAME) - self.assertNotIn('author_id', response['recent_notifications'][0]) - - with self.swap( - user_jobs_continuous.DashboardRecentUpdatesAggregator, - 'get_recent_notifications', - self._get_recent_notifications_mock_by_anonymous_user): - - self.login(self.VIEWER_EMAIL) - response = self.get_json(self.DASHBOARD_DATA_URL) - self.assertEqual(len(response['recent_notifications']), 1) - self.assertEqual( - response['recent_notifications'][0]['author_username'], '') - self.assertNotIn('author_id', response['recent_notifications'][0]) - - -class CreationButtonsTest(test_utils.GenericTestBase): - - def setUp(self): - super(CreationButtonsTest, self).setUp() - self.signup(self.EDITOR_EMAIL, self.EDITOR_USERNAME) - - def test_new_exploration_ids(self): - """Test generation of exploration ids.""" - self.login(self.EDITOR_EMAIL) - - response = self.testapp.get('/my_explorations') - self.assertEqual(response.status_int, 200) - csrf_token = self.get_csrf_token_from_response(response) - exp_a_id = self.post_json(feconf.NEW_EXPLORATION_URL, { - 'title': self.UNICODE_TEST_STRING, - 'category': self.UNICODE_TEST_STRING, - 'objective': 'Learn how to generate exploration ids.', - 'language_code': feconf.DEFAULT_LANGUAGE_CODE - }, csrf_token)[home.EXPLORATION_ID_KEY] - self.assertEqual(len(exp_a_id), 12) - - self.logout() - - def test_exploration_upload_button(self): - """Test that the exploration upload button appears when appropriate.""" - self.login(self.EDITOR_EMAIL) - - response = self.testapp.get('/my_explorations') - self.assertEqual(response.status_int, 200) - response.mustcontain(no=['ng-click="showUploadExplorationModal()"']) - - config_services.set_property( - feconf.SYSTEM_COMMITTER_ID, 'allow_yaml_file_upload', True) - - response = self.testapp.get('/my_explorations') - self.assertEqual(response.status_int, 200) - response.mustcontain('ng-click="showUploadExplorationModal()"') - - self.logout() diff --git a/core/controllers/library.py b/core/controllers/library.py index 81cf26d433fe..c5198c40f034 100644 --- a/core/controllers/library.py +++ b/core/controllers/library.py @@ -19,6 +19,7 @@ import string from core.controllers import base +from core.domain import collection_services from core.domain import exp_services from core.domain import summary_services from core.domain import user_services @@ -31,24 +32,30 @@ current_user_services = models.Registry.import_current_user_services() -def get_matching_exploration_dicts(query_string, search_cursor): - """Given a query string and a search cursor, returns a list of exploration +def get_matching_activity_dicts(query_string, search_cursor): + """Given a query string and a search cursor, returns a list of activity dicts that satisfy the search query. """ + collection_ids, search_cursor = ( + collection_services.get_collection_ids_matching_query( + query_string, cursor=search_cursor)) exp_ids, new_search_cursor = ( exp_services.get_exploration_ids_matching_query( query_string, cursor=search_cursor)) - - explorations_list = ( + activity_list = [] + activity_list = ( + summary_services.get_displayable_collection_summary_dicts_matching_ids( + collection_ids)) + activity_list += ( summary_services.get_displayable_exp_summary_dicts_matching_ids( exp_ids)) - if len(explorations_list) == feconf.DEFAULT_QUERY_LIMIT: + if len(activity_list) == feconf.DEFAULT_QUERY_LIMIT: logging.error( - '%s explorations were fetched to load the search results. ' + '%s activities were fetched to load the library page. ' 'You may be running up against the default query limits.' % feconf.DEFAULT_QUERY_LIMIT) - return explorations_list, new_search_cursor + return activity_list, new_search_cursor class LibraryPage(base.BaseHandler): @@ -56,15 +63,23 @@ class LibraryPage(base.BaseHandler): for search results. """ + PAGE_NAME_FOR_CSRF = 'library' + def get(self): """Handles GET requests.""" + search_mode = 'search' in self.request.url + self.values.update({ + 'meta_description': ( + feconf.SEARCH_PAGE_DESCRIPTION if search_mode + else feconf.LIBRARY_PAGE_DESCRIPTION), 'nav_mode': feconf.NAV_MODE_LIBRARY, 'has_fully_registered': bool( self.user_id and user_services.has_fully_registered(self.user_id)), 'LANGUAGE_CODES_AND_NAMES': ( utils.get_all_language_codes_and_names()), + 'search_mode': search_mode, 'SEARCH_DROPDOWN_CATEGORIES': feconf.SEARCH_DROPDOWN_CATEGORIES, }) self.render_template('library/library.html') @@ -78,6 +93,11 @@ def get(self): # TODO(sll): Support index pages for other language codes. summary_dicts_by_category = summary_services.get_library_groups([ feconf.DEFAULT_LANGUAGE_CODE]) + recently_published_summary_dicts = ( + summary_services.get_recently_published_exploration_summary_dicts()) + top_rated_activity_summary_dicts = ( + summary_services.get_top_rated_exploration_summary_dicts( + [feconf.DEFAULT_LANGUAGE_CODE])) featured_activity_summary_dicts = ( summary_services.get_featured_exploration_summary_dicts( [feconf.DEFAULT_LANGUAGE_CODE])) @@ -87,6 +107,18 @@ def get(self): user_settings = user_services.get_user_settings(self.user_id) preferred_language_codes = user_settings.preferred_language_codes + if recently_published_summary_dicts: + summary_dicts_by_category.insert(0, { + 'activity_summary_dicts': recently_published_summary_dicts, + 'categories': [], + 'header': feconf.LIBRARY_CATEGORY_RECENTLY_PUBLISHED, + }) + if top_rated_activity_summary_dicts: + summary_dicts_by_category.insert(0, { + 'activity_summary_dicts': top_rated_activity_summary_dicts, + 'categories': [], + 'header': feconf.LIBRARY_CATEGORY_TOP_RATED_EXPLORATIONS, + }) if featured_activity_summary_dicts: summary_dicts_by_category.insert(0, { 'activity_summary_dicts': featured_activity_summary_dicts, @@ -103,7 +135,7 @@ def get(self): class SearchHandler(base.BaseHandler): - """Provides data for exploration search results.""" + """Provides data for activity search results.""" def get(self): """Handles GET requests.""" @@ -124,11 +156,11 @@ def get(self): 'language_code') search_cursor = self.request.get('cursor', None) - explorations_list, new_search_cursor = get_matching_exploration_dicts( + activity_list, new_search_cursor = get_matching_activity_dicts( query_string, search_cursor) self.values.update({ - 'explorations_list': explorations_list, + 'activity_list': activity_list, 'search_cursor': new_search_cursor, }) diff --git a/core/controllers/library_test.py b/core/controllers/library_test.py index d6f5b8553681..6794889fb67a 100644 --- a/core/controllers/library_test.py +++ b/core/controllers/library_test.py @@ -42,7 +42,7 @@ def test_library_page(self): """Test access to the library page.""" response = self.testapp.get(feconf.LIBRARY_INDEX_URL) self.assertEqual(response.status_int, 200) - response.mustcontain('Library', 'Categories') + response.mustcontain('I18N_LIBRARY_PAGE_TITLE') def test_library_handler_demo_exploration(self): """Test the library data handler on demo explorations.""" @@ -51,7 +51,7 @@ def test_library_handler_demo_exploration(self): 'is_admin': False, 'is_moderator': False, 'is_super_admin': False, - 'explorations_list': [], + 'activity_list': [], 'search_cursor': None, 'profile_picture_data_url': None, }, response_dict) @@ -61,7 +61,7 @@ def test_library_handler_demo_exploration(self): # Load the search results with an empty query. response_dict = self.get_json(feconf.LIBRARY_SEARCH_DATA_URL) - self.assertEqual(len(response_dict['explorations_list']), 1) + self.assertEqual(len(response_dict['activity_list']), 1) self.assertDictContainsSubset({ 'id': '0', 'category': 'Welcome', @@ -69,7 +69,7 @@ def test_library_handler_demo_exploration(self): 'language_code': 'en', 'objective': 'become familiar with Oppia\'s capabilities', 'status': rights_manager.ACTIVITY_STATUS_PUBLIC, - }, response_dict['explorations_list'][0]) + }, response_dict['activity_list'][0]) # Publicize the demo exploration. self.set_admins([self.ADMIN_USERNAME]) @@ -100,7 +100,7 @@ def test_library_handler_demo_exploration(self): # Load the search results with an empty query. response_dict = self.get_json(feconf.LIBRARY_SEARCH_DATA_URL) - self.assertEqual(len(response_dict['explorations_list']), 1) + self.assertEqual(len(response_dict['activity_list']), 1) self.assertDictContainsSubset({ 'id': '0', 'category': 'A new category', @@ -108,7 +108,7 @@ def test_library_handler_demo_exploration(self): 'language_code': 'en', 'objective': 'become familiar with Oppia\'s capabilities', 'status': rights_manager.ACTIVITY_STATUS_PUBLICIZED, - }, response_dict['explorations_list'][0]) + }, response_dict['activity_list'][0]) def test_library_handler_for_created_explorations(self): """Test the library data handler for manually created explirations.""" @@ -120,7 +120,7 @@ def test_library_handler_for_created_explorations(self): 'is_admin': True, 'is_moderator': True, 'is_super_admin': False, - 'explorations_list': [], + 'activity_list': [], 'user_email': self.ADMIN_EMAIL, 'username': self.ADMIN_USERNAME, 'search_cursor': None, @@ -135,7 +135,7 @@ def test_library_handler_for_created_explorations(self): # Test that the private exploration isn't displayed. response_dict = self.get_json(feconf.LIBRARY_SEARCH_DATA_URL) - self.assertEqual(response_dict['explorations_list'], []) + self.assertEqual(response_dict['activity_list'], []) # Create exploration B exploration = self.save_new_valid_exploration( @@ -153,7 +153,7 @@ def test_library_handler_for_created_explorations(self): # Load the search results with an empty query. response_dict = self.get_json(feconf.LIBRARY_SEARCH_DATA_URL) - self.assertEqual(len(response_dict['explorations_list']), 2) + self.assertEqual(len(response_dict['activity_list']), 2) self.assertDictContainsSubset({ 'id': 'B', 'category': 'Category B', @@ -161,7 +161,7 @@ def test_library_handler_for_created_explorations(self): 'language_code': 'en', 'objective': 'Objective B', 'status': rights_manager.ACTIVITY_STATUS_PUBLICIZED, - }, response_dict['explorations_list'][0]) + }, response_dict['activity_list'][0]) self.assertDictContainsSubset({ 'id': 'A', 'category': 'Category A', @@ -169,14 +169,14 @@ def test_library_handler_for_created_explorations(self): 'language_code': 'en', 'objective': 'Objective A', 'status': rights_manager.ACTIVITY_STATUS_PUBLIC, - }, response_dict['explorations_list'][1]) + }, response_dict['activity_list'][1]) # Delete exploration A exp_services.delete_exploration(self.admin_id, 'A') # Load the search results with an empty query. response_dict = self.get_json(feconf.LIBRARY_SEARCH_DATA_URL) - self.assertEqual(len(response_dict['explorations_list']), 1) + self.assertEqual(len(response_dict['activity_list']), 1) self.assertDictContainsSubset({ 'id': 'B', 'category': 'Category B', @@ -184,7 +184,7 @@ def test_library_handler_for_created_explorations(self): 'language_code': 'en', 'objective': 'Objective B', 'status': rights_manager.ACTIVITY_STATUS_PUBLICIZED, - }, response_dict['explorations_list'][0]) + }, response_dict['activity_list'][0]) class CategoryConfigTest(test_utils.GenericTestBase): diff --git a/core/controllers/pages.py b/core/controllers/pages.py index 0fad8e15b4b8..95bb06ae21d6 100644 --- a/core/controllers/pages.py +++ b/core/controllers/pages.py @@ -18,11 +18,13 @@ import urlparse from core.controllers import base -from core.controllers import editor from core.domain import config_domain import feconf +MODERATOR_REQUEST_FORUM_URL_DEFAULT_VALUE = ( + 'https://moderator/request/forum/url') + ABOUT_PAGE_YOUTUBE_VIDEO_ID = config_domain.ConfigProperty( 'about_page_youtube_video_id', {'type': 'unicode'}, 'The (optional) video id for the About page', @@ -40,6 +42,11 @@ 'site_forum_url', {'type': 'unicode'}, 'The site forum URL (for links; the Forum page is configured separately)', default_value='https://site/forum/url') +MODERATOR_REQUEST_FORUM_URL = config_domain.ConfigProperty( + 'moderator_request_forum_url', {'type': 'unicode'}, + 'A link to the forum for nominating explorations to be featured ' + 'in the Oppia library', + default_value=MODERATOR_REQUEST_FORUM_URL_DEFAULT_VALUE) class SplashPage(base.BaseHandler): @@ -48,6 +55,7 @@ class SplashPage(base.BaseHandler): def get(self): """Handles GET requests.""" self.values.update({ + 'meta_description': feconf.SPLASH_PAGE_DESCRIPTION, 'nav_mode': feconf.NAV_MODE_SPLASH, }) self.render_template('pages/splash.html') @@ -61,20 +69,46 @@ def get(self): self.values.update({ 'ABOUT_PAGE_YOUTUBE_VIDEO_ID': ABOUT_PAGE_YOUTUBE_VIDEO_ID.value, 'CONTACT_EMAIL_ADDRESS': CONTACT_EMAIL_ADDRESS.value, - 'SITE_FORUM_URL': SITE_FORUM_URL.value, + 'meta_description': feconf.ABOUT_PAGE_DESCRIPTION, 'nav_mode': feconf.NAV_MODE_ABOUT, }) self.render_template('pages/about.html') +class TeachPage(base.BaseHandler): + """Page with information about how to teach on Oppia.""" + + def get(self): + """Handles GET requests.""" + self.values.update({ + 'SITE_FORUM_URL': SITE_FORUM_URL.value, + 'MODERATOR_REQUEST_FORUM_URL': MODERATOR_REQUEST_FORUM_URL.value, + 'nav_mode': feconf.NAV_MODE_TEACH, + }) + self.render_template('pages/teach.html') + + +class ContactPage(base.BaseHandler): + """Page with information about how to contact Oppia.""" + + def get(self): + """Handles GET requests.""" + self.values.update({ + 'CONTACT_EMAIL_ADDRESS': CONTACT_EMAIL_ADDRESS.value, + 'SITE_FORUM_URL': SITE_FORUM_URL.value, + 'nav_mode': feconf.NAV_MODE_CONTACT, + }) + self.render_template('pages/contact.html') + + class ParticipatePage(base.BaseHandler): """Page with information about participating in Oppia.""" def get(self): """Handles GET requests.""" self.values.update({ - 'MODERATOR_REQUEST_FORUM_URL': ( - editor.MODERATOR_REQUEST_FORUM_URL.value), + 'meta_description': feconf.PARTICIPATE_PAGE_DESCRIPTION, + 'MODERATOR_REQUEST_FORUM_URL': MODERATOR_REQUEST_FORUM_URL.value, 'SITE_FORUM_URL': SITE_FORUM_URL.value, 'nav_mode': feconf.NAV_MODE_PARTICIPATE, }) @@ -101,6 +135,7 @@ def get(self): urllib.quote(self.request.uri, safe=''), ) ), + 'meta_description': feconf.FORUM_PAGE_DESCRIPTION, 'on_localhost': netloc.startswith('localhost'), }) self.render_template('pages/forum.html') @@ -114,6 +149,10 @@ def get(self): if not feconf.SHOW_CUSTOM_PAGES: raise self.PageNotFoundException + self.values.update({ + 'meta_description': feconf.TERMS_PAGE_DESCRIPTION, + }) + self.render_template('pages/terms.html') @@ -126,3 +165,11 @@ def get(self): raise self.PageNotFoundException self.render_template('pages/privacy.html') + + +class AboutRedirectPage(base.BaseHandler): + """An page that redirects to the main About page.""" + + def get(self): + """Handles GET requests.""" + self.redirect('/about') diff --git a/core/controllers/pages_test.py b/core/controllers/pages_test.py index ab0d41d0ae0e..18d49f18df56 100644 --- a/core/controllers/pages_test.py +++ b/core/controllers/pages_test.py @@ -24,4 +24,4 @@ def test_about_page(self): response = self.testapp.get('/about') self.assertEqual(response.status_int, 200) self.assertEqual(response.content_type, 'text/html') - response.mustcontain('Credits', 'Contact', 'License') + response.mustcontain('Credits', 'License') diff --git a/core/controllers/profile.py b/core/controllers/profile.py index 7a3656330cd9..f539cdb5657b 100644 --- a/core/controllers/profile.py +++ b/core/controllers/profile.py @@ -89,8 +89,10 @@ def get(self, username): edited_exp_summary_dicts = ( summary_services.get_displayable_exp_summary_dicts_matching_ids( user_contributions.edited_exploration_ids)) + profile_is_of_current_user = (self.username == username) self.values.update({ + 'profile_is_of_current_user': profile_is_of_current_user, 'profile_username': user_settings.username, 'user_bio': user_settings.user_bio, 'subject_interests': user_settings.subject_interests, @@ -115,6 +117,7 @@ class PreferencesPage(base.BaseHandler): def get(self): """Handles GET requests.""" self.values.update({ + 'meta_description': feconf.PREFERENCES_PAGE_DESCRIPTION, 'nav_mode': feconf.NAV_MODE_PROFILE, 'LANGUAGE_CODES_AND_NAMES': ( utils.get_all_language_codes_and_names()), @@ -136,6 +139,8 @@ def get(self): self.user_id) self.values.update({ 'preferred_language_codes': user_settings.preferred_language_codes, + 'preferred_site_language_code': ( + user_settings.preferred_site_language_code), 'profile_picture_data_url': user_settings.profile_picture_data_url, 'user_bio': user_settings.user_bio, 'subject_interests': user_settings.subject_interests, @@ -158,6 +163,9 @@ def put(self): user_services.update_subject_interests(self.user_id, data) elif update_type == 'preferred_language_codes': user_services.update_preferred_language_codes(self.user_id, data) + elif update_type == 'preferred_site_language_code': + user_services.update_preferred_site_language_code( + self.user_id, data) elif update_type == 'profile_picture_data_url': user_services.update_profile_picture_data_url(self.user_id, data) elif update_type == 'email_preferences': @@ -217,6 +225,7 @@ def get(self): return self.values.update({ + 'meta_description': feconf.SIGNUP_PAGE_DESCRIPTION, 'nav_mode': feconf.NAV_MODE_SIGNUP, 'CAN_SEND_EMAILS_TO_USERS': feconf.CAN_SEND_EMAILS_TO_USERS, }) @@ -305,3 +314,17 @@ def post(self): self.render_json({ 'username_is_taken': username_is_taken, }) + + +class SiteLanguageHandler(base.BaseHandler): + """Changes the preferred system language in the user's preferences.""" + + PAGE_NAME_FOR_CSRF = feconf.CSRF_PAGE_NAME_I18N + + def put(self): + """Handles PUT requests.""" + if user_services.has_fully_registered(self.user_id): + site_language_code = self.payload.get('site_language_code') + user_services.update_preferred_site_language_code( + self.user_id, site_language_code) + self.render_json({}) diff --git a/core/controllers/profile_test.py b/core/controllers/profile_test.py index 94d1257dcfae..6e68aae69461 100644 --- a/core/controllers/profile_test.py +++ b/core/controllers/profile_test.py @@ -28,7 +28,8 @@ def test_signup_page_does_not_have_top_right_menu(self): self.login(self.EDITOR_EMAIL) response = self.testapp.get(feconf.SIGNUP_URL) self.assertEqual(response.status_int, 200) - response.mustcontain(no=['Logout', 'Sign in']) + # Sign in can't be inside an html tag, but can appear inside js code + response.mustcontain(no=['Logout', '>Sign in']) self.logout() def test_going_somewhere_else_while_signing_in_logs_user_out(self): @@ -268,6 +269,27 @@ def test_get_profile_picture_valid_username(self): class ProfileDataHandlerTests(test_utils.GenericTestBase): + def test_preference_page_updates(self): + self.signup(self.EDITOR_EMAIL, username=self.EDITOR_USERNAME) + self.login(self.EDITOR_EMAIL) + response = self.testapp.get('/preferences') + csrf_token = self.get_csrf_token_from_response(response) + original_preferences = self.get_json('/preferenceshandler/data') + self.assertEqual( + ['en'], original_preferences['preferred_language_codes']) + self.assertIsNone(original_preferences['preferred_site_language_code']) + self.put_json( + '/preferenceshandler/data', + {'update_type': 'preferred_site_language_code', 'data': 'en'}, + csrf_token=csrf_token) + self.put_json( + '/preferenceshandler/data', + {'update_type': 'preferred_language_codes', 'data': ['de']}, + csrf_token=csrf_token) + new_preferences = self.get_json('/preferenceshandler/data') + self.assertEqual(new_preferences['preferred_language_codes'], ['de']) + self.assertEqual(new_preferences['preferred_site_language_code'], 'en') + def test_profile_data_is_independent_of_currently_logged_in_user(self): self.signup(self.EDITOR_EMAIL, username=self.EDITOR_USERNAME) self.login(self.EDITOR_EMAIL) @@ -432,3 +454,37 @@ def test_edited(self): self.assertEqual( response_dict['edited_exp_summary_dicts'][0]['objective'], 'the objective') + + +class SiteLanguageHandlerTests(test_utils.GenericTestBase): + + def test_save_site_language_handler(self): + """Test the language is saved in the preferences when handler is called. + """ + self.signup(self.EDITOR_EMAIL, self.EDITOR_USERNAME) + language_code = 'es' + self.login(self.EDITOR_EMAIL) + response = self.testapp.get('/preferences') + self.assertEqual(response.status_int, 200) + csrf_token = self.get_csrf_token_from_response(response) + self.put_json('/preferenceshandler/data', { + 'update_type': 'preferred_site_language_code', + 'data': language_code, + }, csrf_token) + + preferences = self.get_json('/preferenceshandler/data') + self.assertIsNotNone(preferences) + self.assertEqual( + preferences['preferred_site_language_code'], language_code) + + self.logout() + + def test_save_site_language_no_user(self): + """The SiteLanguageHandler handler can be called without a user.""" + response = self.testapp.get(feconf.SPLASH_URL) + self.assertEqual(response.status_int, 200) + csrf_token = self.get_csrf_token_from_response( + response, token_type=feconf.CSRF_PAGE_NAME_I18N) + self.put_json(feconf.SITE_LANGUAGE_DATA_URL, { + 'site_language_code': 'es', + }, csrf_token) diff --git a/core/controllers/reader_test.py b/core/controllers/reader_test.py index b77c9cba1a93..6e0cf63341dc 100644 --- a/core/controllers/reader_test.py +++ b/core/controllers/reader_test.py @@ -63,7 +63,7 @@ def test_unpublished_explorations_are_invisible_to_other_editors(self): self.signup(other_editor_email, 'othereditorusername') other_exploration = exp_domain.Exploration.create_default_exploration( - 'eid2', 'A title', 'A category') + 'eid2') exp_services.save_new_exploration( other_editor_email, other_exploration) @@ -127,9 +127,8 @@ def _init_classify_inputs(self, exploration_id): yaml_content = utils.get_file_contents(test_exp_filepath) assets_list = [] exp_services.save_new_exploration_from_yaml_and_assets( - feconf.SYSTEM_COMMITTER_ID, yaml_content, - 'Testing String Classifier', 'Test', - exploration_id, assets_list) + feconf.SYSTEM_COMMITTER_ID, yaml_content, exploration_id, + assets_list) self.exp_id = exploration_id self.exp_state = ( diff --git a/core/domain/collection_domain.py b/core/domain/collection_domain.py index b94dd09d71a4..28830db9f9ff 100644 --- a/core/domain/collection_domain.py +++ b/core/domain/collection_domain.py @@ -22,6 +22,8 @@ """ import copy +import re +import string import feconf import utils @@ -32,6 +34,8 @@ COLLECTION_PROPERTY_TITLE = 'title' COLLECTION_PROPERTY_CATEGORY = 'category' COLLECTION_PROPERTY_OBJECTIVE = 'objective' +COLLECTION_PROPERTY_LANGUAGE_CODE = 'language_code' +COLLECTION_PROPERTY_TAGS = 'tags' COLLECTION_NODE_PROPERTY_PREREQUISITE_SKILLS = 'prerequisite_skills' COLLECTION_NODE_PROPERTY_ACQUIRED_SKILLS = 'acquired_skills' @@ -64,7 +68,8 @@ class CollectionChange(object): COLLECTION_PROPERTIES = ( COLLECTION_PROPERTY_TITLE, COLLECTION_PROPERTY_CATEGORY, - COLLECTION_PROPERTY_OBJECTIVE) + COLLECTION_PROPERTY_OBJECTIVE, COLLECTION_PROPERTY_LANGUAGE_CODE, + COLLECTION_PROPERTY_TAGS) def __init__(self, change_dict): """Initializes an CollectionChange object from a dict. @@ -256,8 +261,8 @@ class Collection(object): """Domain object for an Oppia collection.""" def __init__(self, collection_id, title, category, objective, - schema_version, nodes, version, created_on=None, - last_updated=None): + language_code, tags, schema_version, nodes, version, + created_on=None, last_updated=None): """Constructs a new collection given all the information necessary to represent a collection. @@ -277,6 +282,8 @@ def __init__(self, collection_id, title, category, objective, self.title = title self.category = category self.objective = objective + self.language_code = language_code + self.tags = tags self.schema_version = schema_version self.nodes = nodes self.version = version @@ -289,6 +296,8 @@ def to_dict(self): 'title': self.title, 'category': self.category, 'objective': self.objective, + 'language_code': self.language_code, + 'tags': self.tags, 'schema_version': self.schema_version, 'nodes': [ node.to_dict() for node in self.nodes @@ -297,9 +306,12 @@ def to_dict(self): @classmethod def create_default_collection( - cls, collection_id, title, category, objective): + cls, collection_id, title=feconf.DEFAULT_COLLECTION_TITLE, + category=feconf.DEFAULT_COLLECTION_CATEGORY, + objective=feconf.DEFAULT_COLLECTION_OBJECTIVE, + language_code=feconf.DEFAULT_LANGUAGE_CODE): return cls( - collection_id, title, category, objective, + collection_id, title, category, objective, language_code, [], feconf.CURRENT_COLLECTION_SCHEMA_VERSION, [], 0) @classmethod @@ -309,6 +321,7 @@ def from_dict( collection = cls( collection_dict['id'], collection_dict['title'], collection_dict['category'], collection_dict['objective'], + collection_dict['language_code'], collection_dict['tags'], collection_dict['schema_version'], [], collection_version, collection_created_on, collection_last_updated) @@ -328,7 +341,15 @@ def to_yaml(self): return utils.yaml_from_dict(collection_dict) @classmethod - def from_yaml(cls, collection_id, yaml_content): + def _convert_v1_dict_to_v2_dict(cls, collection_dict): + """Converts a v1 collection dict into a v2 collection dict.""" + collection_dict['schema_version'] = 2 + collection_dict['language_code'] = feconf.DEFAULT_LANGUAGE_CODE + collection_dict['tags'] = [] + return collection_dict + + @classmethod + def _migrate_to_latest_yaml_version(cls, yaml_content): try: collection_dict = utils.dict_from_yaml(yaml_content) except Exception as e: @@ -337,6 +358,25 @@ def from_yaml(cls, collection_id, yaml_content): 'a zip file. The YAML parser returned the following error: %s' % e) + collection_schema_version = collection_dict.get('schema_version') + if collection_schema_version is None: + raise Exception('Invalid YAML file: no schema version specified.') + if not (1 <= collection_schema_version + <= feconf.CURRENT_COLLECTION_SCHEMA_VERSION): + raise Exception( + 'Sorry, we can only process v1 to v%s collection YAML files at ' + 'present.' % feconf.CURRENT_COLLECTION_SCHEMA_VERSION) + + if collection_schema_version == 1: + collection_dict = cls._convert_v1_dict_to_v2_dict(collection_dict) + collection_schema_version = 2 + + return collection_dict + + @classmethod + def from_yaml(cls, collection_id, yaml_content): + collection_dict = cls._migrate_to_latest_yaml_version(yaml_content) + collection_dict['id'] = collection_id return Collection.from_dict(collection_dict) @@ -356,8 +396,7 @@ def exploration_ids(self): """Returns a list of all the exploration IDs that are part of this collection. """ - return [ - node.exploration_id for node in self.nodes] + return [node.exploration_id for node in self.nodes] @property def init_exploration_ids(self): @@ -415,6 +454,12 @@ def update_category(self, category): def update_objective(self, objective): self.objective = objective + def update_language_code(self, language_code): + self.language_code = language_code + + def update_tags(self, tags): + self.tags = tags + def _find_node(self, exploration_id): for ind, node in enumerate(self.nodes): if node.exploration_id == exploration_id: @@ -454,22 +499,63 @@ def validate(self, strict=True): if not isinstance(self.title, basestring): raise utils.ValidationError( 'Expected title to be a string, received %s' % self.title) - utils.require_valid_name(self.title, 'the collection title') + utils.require_valid_name( + self.title, 'the collection title', allow_empty=True) if not isinstance(self.category, basestring): raise utils.ValidationError( 'Expected category to be a string, received %s' % self.category) - utils.require_valid_name(self.category, 'the collection category') + utils.require_valid_name( + self.category, 'the collection category', allow_empty=True) if not isinstance(self.objective, basestring): raise utils.ValidationError( 'Expected objective to be a string, received %s' % self.objective) - if not self.objective: + if not isinstance(self.language_code, basestring): + raise utils.ValidationError( + 'Expected language code to be a string, received %s' % + self.language_code) + + if not self.language_code: + raise utils.ValidationError( + 'A language must be specified (in the \'Settings\' tab).') + + if not any([self.language_code == lc['code'] + for lc in feconf.ALL_LANGUAGE_CODES]): raise utils.ValidationError( - 'An objective must be specified (in the \'Settings\' tab).') + 'Invalid language code: %s' % self.language_code) + + if not isinstance(self.tags, list): + raise utils.ValidationError( + 'Expected tags to be a list, received %s' % self.tags) + for tag in self.tags: + if not isinstance(tag, basestring): + raise utils.ValidationError( + 'Expected each tag to be a string, received \'%s\'' % tag) + + if not tag: + raise utils.ValidationError('Tags should be non-empty.') + + if not re.match(feconf.TAG_REGEX, tag): + raise utils.ValidationError( + 'Tags should only contain lowercase letters and spaces, ' + 'received \'%s\'' % tag) + + if (tag[0] not in string.ascii_lowercase or + tag[-1] not in string.ascii_lowercase): + raise utils.ValidationError( + 'Tags should not start or end with whitespace, received ' + ' \'%s\'' % tag) + + if re.search(r'\s\s+', tag): + raise utils.ValidationError( + 'Adjacent whitespace in tags should be collapsed, ' + 'received \'%s\'' % tag) + if len(set(self.tags)) != len(self.tags): + raise utils.ValidationError('Some tags duplicate each other') if not isinstance(self.schema_version, int): raise utils.ValidationError( @@ -497,6 +583,18 @@ def validate(self, strict=True): node.validate() if strict: + if not self.title: + raise utils.ValidationError( + 'A title must be specified for the collection.') + + if not self.objective: + raise utils.ValidationError( + 'An objective must be specified for the collection.') + + if not self.category: + raise utils.ValidationError( + 'A category must be specified for the collection.') + if not self.nodes: raise utils.ValidationError( 'Expected to have at least 1 exploration in the ' @@ -532,14 +630,17 @@ def validate(self, strict=True): class CollectionSummary(object): """Domain object for an Oppia collection summary.""" - def __init__(self, collection_id, title, category, objective, - status, community_owned, owner_ids, editor_ids, + def __init__(self, collection_id, title, category, objective, language_code, + tags, status, community_owned, owner_ids, editor_ids, viewer_ids, contributor_ids, contributors_summary, version, - collection_model_created_on, collection_model_last_updated): + node_count, collection_model_created_on, + collection_model_last_updated): self.id = collection_id self.title = title self.category = category self.objective = objective + self.language_code = language_code + self.tags = tags self.status = status self.community_owned = community_owned self.owner_ids = owner_ids @@ -548,6 +649,7 @@ def __init__(self, collection_id, title, category, objective, self.contributor_ids = contributor_ids self.contributors_summary = contributors_summary self.version = version + self.node_count = node_count self.collection_model_created_on = collection_model_created_on self.collection_model_last_updated = collection_model_last_updated @@ -557,6 +659,8 @@ def to_dict(self): 'title': self.title, 'category': self.category, 'objective': self.objective, + 'language_code': self.language_code, + 'tags': self.tags, 'status': self.status, 'community_owned': self.community_owned, 'owner_ids': self.owner_ids, diff --git a/core/domain/collection_domain_test.py b/core/domain/collection_domain_test.py index 28376ca2049d..7257e5ce87bf 100644 --- a/core/domain/collection_domain_test.py +++ b/core/domain/collection_domain_test.py @@ -30,6 +30,7 @@ # utils.dict_from_yaml can isolate differences quickly. SAMPLE_YAML_CONTENT = ("""category: A category +language_code: en nodes: - acquired_skills: - Skill0a @@ -38,6 +39,7 @@ prerequisite_skills: [] objective: An objective schema_version: %d +tags: [] title: A title """) % (feconf.CURRENT_COLLECTION_SCHEMA_VERSION) @@ -82,6 +84,41 @@ def test_objective_validation(self): self.collection.objective = 0 self._assert_validation_error('Expected objective to be a string') + def test_language_code_validation(self): + self.collection.language_code = '' + self._assert_validation_error('language must be specified') + + self.collection.language_code = 0 + self._assert_validation_error('Expected language code to be a string') + + self.collection.language_code = 'xz' + self._assert_validation_error('Invalid language code') + + def test_tags_validation(self): + self.collection.tags = 'abc' + self._assert_validation_error('Expected tags to be a list') + + self.collection.tags = [2, 3] + self._assert_validation_error('Expected each tag to be a string') + + self.collection.tags = ['', 'tag'] + self._assert_validation_error('Tags should be non-empty') + + self.collection.tags = ['234'] + self._assert_validation_error( + 'Tags should only contain lowercase letters and spaces') + + self.collection.tags = [' abc'] + self._assert_validation_error( + 'Tags should not start or end with whitespace') + + self.collection.tags = ['abc def'] + self._assert_validation_error( + 'Adjacent whitespace in tags should be collapsed') + + self.collection.tags = ['abc', 'abc'] + self._assert_validation_error('Some tags duplicate each other') + def test_schema_version_validation(self): self.collection.schema_version = 'some_schema_version' self._assert_validation_error('Expected schema version to be an int') @@ -132,6 +169,38 @@ def test_initial_explorations_validation(self): 'Expected to have at least 1 exploration with no prerequisite ' 'skills.') + def test_metadata_validation(self): + self.collection.title = '' + self.collection.objective = '' + self.collection.category = '' + self.collection.nodes = [] + self.collection.add_node('exp_id_1') + + # Having no title is fine for non-strict validation. + self.collection.validate(strict=False) + # But it's not okay for strict validation. + self._assert_validation_error( + 'A title must be specified for the collection.') + self.collection.title = 'A title' + + # Having no objective is fine for non-strict validation. + self.collection.validate(strict=False) + # But it's not okay for strict validation. + self._assert_validation_error( + 'An objective must be specified for the collection.') + self.collection.objective = 'An objective' + + # Having no category is fine for non-strict validation. + self.collection.validate(strict=False) + # But it's not okay for strict validation. + self._assert_validation_error( + 'A category must be specified for the collection.') + self.collection.category = 'A category' + + # Now the collection passes both strict and non-strict validation. + self.collection.validate(strict=False) + self.collection.validate(strict=True) + def test_collection_completability_validation(self): # Add another exploration, but make it impossible to reach exp_id_1. self.collection.add_node('exp_id_1') @@ -194,16 +263,14 @@ def test_collection_node_skills_validation(self): def test_is_demo_property(self): """Test the is_demo property.""" - demo = collection_domain.Collection.create_default_collection( - '0', 'title', 'category', 'objective') + demo = collection_domain.Collection.create_default_collection('0') self.assertEqual(demo.is_demo, True) - notdemo1 = collection_domain.Collection.create_default_collection( - 'a', 'title', 'category', 'objective') + notdemo1 = collection_domain.Collection.create_default_collection('a') self.assertEqual(notdemo1.is_demo, False) notdemo2 = collection_domain.Collection.create_default_collection( - 'abcd', 'title', 'category', 'objective') + 'abcd') self.assertEqual(notdemo2.is_demo, False) def test_collection_export_import(self): @@ -213,7 +280,7 @@ def test_collection_export_import(self): self.save_new_valid_exploration( '0', 'user@example.com', end_state_name='End') collection = collection_domain.Collection.create_default_collection( - '0', 'title', 'category', 'objective') + '0', title='title', category='category', objective='objective') collection_dict = collection.to_dict() collection_from_dict = collection_domain.Collection.from_dict( collection_dict) @@ -223,7 +290,7 @@ def test_add_delete_node(self): """Test that add_node and delete_node fail in the correct situations. """ collection = collection_domain.Collection.create_default_collection( - '0', 'title', 'category', 'objective') + '0') self.assertEqual(len(collection.nodes), 0) collection.add_node('test_exp') @@ -252,7 +319,7 @@ def test_add_delete_node(self): def test_skills_property(self): collection = collection_domain.Collection.create_default_collection( - '0', 'title', 'category', 'objective') + '0') self.assertEqual(collection.skills, []) @@ -283,7 +350,7 @@ def test_initial_explorations(self): exploration. """ collection = collection_domain.Collection.create_default_collection( - 'collection_id', 'A title', 'A category', 'An objective') + 'collection_id') # If there are no explorations in the collection, there can be no # initial explorations. @@ -312,7 +379,7 @@ def test_next_explorations(self): in the collection. """ collection = collection_domain.Collection.create_default_collection( - 'collection_id', 'A title', 'A category', 'An objective') + 'collection_id') # There should be no next explorations for an empty collection. self.assertEqual(collection.get_next_exploration_ids([]), []) @@ -394,7 +461,7 @@ def test_next_explorations(self): def test_next_explorations_with_invalid_exploration_ids(self): collection = collection_domain.Collection.create_default_collection( - 'collection_id', 'A title', 'A category', 'An objective') + 'collection_id') collection.add_node('exp_id_1') # There should be one suggested exploration to complete by default. @@ -419,7 +486,8 @@ def test_yaml_import_and_export(self): self.EXPLORATION_ID, 'user@example.com', end_state_name='End') collection = collection_domain.Collection.create_default_collection( - self.COLLECTION_ID, 'A title', 'A category', 'An objective') + self.COLLECTION_ID, title='A title', category='A category', + objective='An objective') collection.add_node(self.EXPLORATION_ID) self.assertEqual(len(collection.nodes), 1) @@ -455,9 +523,23 @@ class SchemaMigrationUnitTests(test_utils.GenericTestBase): objective: '' schema_version: 1 title: A title +""") + YAML_CONTENT_V2 = ("""category: A category +language_code: en +nodes: +- acquired_skills: + - Skill1 + - Skill2 + exploration_id: Exp1 + prerequisite_skills: [] +objective: '' +schema_version: 2 +tags: [] +title: A title """) _LATEST_YAML_CONTENT = YAML_CONTENT_V1 + _LATEST_YAML_CONTENT = YAML_CONTENT_V2 def test_load_from_v1(self): """Test direct loading from a v1 yaml file.""" @@ -466,3 +548,11 @@ def test_load_from_v1(self): collection = collection_domain.Collection.from_yaml( 'cid', self.YAML_CONTENT_V1) self.assertEqual(collection.to_yaml(), self._LATEST_YAML_CONTENT) + + def test_load_from_v2(self): + """Test direct loading from a v2 yaml file.""" + self.save_new_valid_exploration( + 'Exp1', 'user@example.com', end_state_name='End') + collection = collection_domain.Collection.from_yaml( + 'cid', self.YAML_CONTENT_V2) + self.assertEqual(collection.to_yaml(), self._LATEST_YAML_CONTENT) diff --git a/core/domain/collection_services.py b/core/domain/collection_services.py index 777fc13f6303..83dd0f224e18 100644 --- a/core/domain/collection_services.py +++ b/core/domain/collection_services.py @@ -31,7 +31,6 @@ from core.domain import collection_domain from core.domain import exp_services from core.domain import rights_manager -from core.domain import summary_services from core.domain import user_services from core.platform import models import feconf @@ -125,6 +124,7 @@ def get_collection_from_model(collection_model, run_conversion=True): return collection_domain.Collection( collection_model.id, collection_model.title, collection_model.category, collection_model.objective, + collection_model.language_code, collection_model.tags, versioned_collection['schema_version'], [ collection_domain.CollectionNode.from_dict(collection_node_dict) for collection_node_dict in versioned_collection['nodes'] @@ -137,6 +137,7 @@ def get_collection_summary_from_model(collection_summary_model): return collection_domain.CollectionSummary( collection_summary_model.id, collection_summary_model.title, collection_summary_model.category, collection_summary_model.objective, + collection_summary_model.language_code, collection_summary_model.tags, collection_summary_model.status, collection_summary_model.community_owned, collection_summary_model.owner_ids, @@ -145,6 +146,7 @@ def get_collection_summary_from_model(collection_summary_model): collection_summary_model.contributor_ids, collection_summary_model.contributors_summary, collection_summary_model.version, + collection_summary_model.node_count, collection_summary_model.collection_model_created_on, collection_summary_model.collection_model_last_updated ) @@ -247,85 +249,6 @@ def is_collection_summary_editable(collection_summary, user_id=None): or collection_summary.community_owned) -def get_learner_collection_dict_by_id( - collection_id, user_id, strict=True, allow_invalid_explorations=False, - version=None): - """Creates and returns a dictionary representation of a collection given by - the provided collection ID. This dictionary contains extra information - along with the dict returned by collection_domain.Collection.to_dict() - which includes useful data for the collection learner view. The information - includes progress in the collection, information about explorations - referenced within the collection, and a slightly nicer data structure for - frontend work. - - This raises a ValidationError if the collection retrieved using the given ID - references non-existent explorations. - """ - collection = get_collection_by_id( - collection_id, strict=strict, version=version) - - exp_ids = collection.exploration_ids - exp_summary_dicts = ( - summary_services.get_displayable_exp_summary_dicts_matching_ids( - exp_ids, editor_user_id=user_id)) - exp_summaries_dict_map = { - exp_summary_dict['id']: exp_summary_dict - for exp_summary_dict in exp_summary_dicts - } - - # TODO(bhenning): Users should not be recommended explorations they have - # completed outside the context of a collection (see #1461). - next_exploration_ids = None - completed_exploration_ids = None - if user_id: - completed_exploration_ids = _get_valid_completed_exploration_ids( - user_id, collection_id, collection) - next_exploration_ids = collection.get_next_exploration_ids( - completed_exploration_ids) - else: - # If the user is not logged in or they have not completed any of - # the explorations yet within the context of this collection, - # recommend the initial explorations. - next_exploration_ids = collection.init_exploration_ids - completed_exploration_ids = [] - - collection_dict = collection.to_dict() - collection_dict['skills'] = collection.skills - collection_dict['playthrough_dict'] = { - 'next_exploration_ids': next_exploration_ids, - 'completed_exploration_ids': completed_exploration_ids - } - collection_dict['version'] = collection.version - - collection_is_public = rights_manager.is_collection_public(collection_id) - - # Insert an 'exploration' dict into each collection node, where the - # dict includes meta information about the exploration (ID and title). - for collection_node in collection_dict['nodes']: - exploration_id = collection_node['exploration_id'] - summary_dict = exp_summaries_dict_map.get(exploration_id) - if not allow_invalid_explorations: - if not summary_dict: - raise utils.ValidationError( - 'Expected collection to only reference valid ' - 'explorations, but found an exploration with ID: %s (was ' - 'the exploration deleted or is it a private exploration ' - 'that you do not have edit access to?)' - % exploration_id) - if collection_is_public and rights_manager.is_exploration_private( - exploration_id): - raise utils.ValidationError( - 'Cannot reference a private exploration within a public ' - 'collection, exploration ID: %s' % exploration_id) - - if summary_dict: - collection_node['exploration_summary'] = summary_dict - else: - collection_node['exploration_summary'] = None - - return collection_dict - - # Query methods. def get_collection_titles_and_categories(collection_ids): """Returns collection titles and categories for the given ids. @@ -372,7 +295,7 @@ def get_completed_exploration_ids(user_id, collection_id): return progress_model.completed_explorations if progress_model else [] -def _get_valid_completed_exploration_ids(user_id, collection_id, collection): +def get_valid_completed_exploration_ids(user_id, collection_id, collection): """Returns a filtered version of the return value of get_completed_exploration_ids, where explorations not also found within the collection are removed from the returned list. @@ -444,21 +367,21 @@ def get_collection_summaries_matching_ids(collection_ids): # TODO(bhenning): Update this function to support also matching the query to # explorations contained within this collection. Introduce tests to verify this # behavior. -def get_collection_summaries_matching_query(query_string, cursor=None): - """Returns a list with all collection summary domain objects matching the - given search query string, as well as a search cursor for future fetches. +def get_collection_ids_matching_query(query_string, cursor=None): + """Returns a list with all collection ids matching the given search query + string, as well as a search cursor for future fetches. This method returns exactly feconf.SEARCH_RESULTS_PAGE_SIZE results if there are at least that many, otherwise it returns all remaining results. (If this behaviour does not occur, an error will be logged.) The method also returns a search cursor. """ - summary_models = [] + returned_collection_ids = [] search_cursor = cursor for _ in range(MAX_ITERATIONS): remaining_to_fetch = feconf.SEARCH_RESULTS_PAGE_SIZE - len( - summary_models) + returned_collection_ids) collection_ids, search_cursor = search_collections( query_string, remaining_to_fetch, cursor=search_cursor) @@ -468,11 +391,11 @@ def get_collection_summaries_matching_query(query_string, cursor=None): collection_models.CollectionSummaryModel.get_multi( collection_ids)): if model is not None: - summary_models.append(model) + returned_collection_ids.append(collection_ids[ind]) else: invalid_collection_ids.append(collection_ids[ind]) - if len(summary_models) == feconf.SEARCH_RESULTS_PAGE_SIZE or ( + if len(returned_collection_ids) == feconf.SEARCH_RESULTS_PAGE_SIZE or ( search_cursor is None): break else: @@ -480,17 +403,13 @@ def get_collection_summaries_matching_query(query_string, cursor=None): 'Search index contains stale collection ids: %s' % ', '.join(invalid_collection_ids)) - if (len(summary_models) < feconf.SEARCH_RESULTS_PAGE_SIZE + if (len(returned_collection_ids) < feconf.SEARCH_RESULTS_PAGE_SIZE and search_cursor is not None): logging.error( 'Could not fulfill search request for query string %s; at least ' '%s retries were needed.' % (query_string, MAX_ITERATIONS)) - return ([ - get_collection_summary_from_model(summary_model) - for summary_model in summary_models - ], search_cursor) - + return (returned_collection_ids, search_cursor) # Repository SAVE and DELETE methods. def apply_change_list(collection_id, change_list): @@ -533,6 +452,12 @@ def apply_change_list(collection_id, change_list): elif (change.property_name == collection_domain.COLLECTION_PROPERTY_OBJECTIVE): collection.update_objective(change.new_value) + elif (change.property_name == + collection_domain.COLLECTION_PROPERTY_LANGUAGE_CODE): + collection.update_language_code(change.new_value) + elif (change.property_name == + collection_domain.COLLECTION_PROPERTY_TAGS): + collection.update_tags(change.new_value) elif ( change.cmd == collection_domain.CMD_MIGRATE_SCHEMA_TO_LATEST_VERSION): @@ -618,11 +543,13 @@ def _save_collection(committer_id, collection, commit_message, change_list): collection_model.category = collection.category collection_model.title = collection.title collection_model.objective = collection.objective + collection_model.language_code = collection.language_code + collection_model.tags = collection.tags collection_model.schema_version = collection.schema_version collection_model.nodes = [ collection_node.to_dict() for collection_node in collection.nodes ] - + collection_model.node_count = len(collection_model.nodes) collection_model.commit(committer_id, commit_message, change_list) memcache_services.delete(_get_collection_memcache_key(collection.id)) index_collections_given_ids([collection.id]) @@ -645,6 +572,8 @@ def _create_collection(committer_id, collection, commit_message, commit_cmds): category=collection.category, title=collection.title, objective=collection.objective, + language_code=collection.language_code, + tags=collection.tags, schema_version=collection.schema_version, nodes=[ collection_node.to_dict() for collection_node in collection.nodes @@ -817,14 +746,16 @@ def compute_summary_of_collection(collection, contributor_id_to_add): collection_model_last_updated = collection.last_updated collection_model_created_on = collection.created_on + collection_model_node_count = len(collection.nodes) collection_summary = collection_domain.CollectionSummary( collection.id, collection.title, collection.category, - collection.objective, collection_rights.status, - collection_rights.community_owned, collection_rights.owner_ids, - collection_rights.editor_ids, collection_rights.viewer_ids, - contributor_ids, contributors_summary, - collection.version, collection_model_created_on, + collection.objective, collection.language_code, collection.tags, + collection_rights.status, collection_rights.community_owned, + collection_rights.owner_ids, collection_rights.editor_ids, + collection_rights.viewer_ids, contributor_ids, contributors_summary, + collection.version, collection_model_node_count, + collection_model_created_on, collection_model_last_updated ) @@ -866,6 +797,8 @@ def save_collection_summary(collection_summary): title=collection_summary.title, category=collection_summary.category, objective=collection_summary.objective, + language_code=collection_summary.language_code, + tags=collection_summary.tags, status=collection_summary.status, community_owned=collection_summary.community_owned, owner_ids=collection_summary.owner_ids, @@ -874,6 +807,7 @@ def save_collection_summary(collection_summary): contributor_ids=collection_summary.contributor_ids, contributors_summary=collection_summary.contributors_summary, version=collection_summary.version, + node_count=collection_summary.node_count, collection_model_last_updated=( collection_summary.collection_model_last_updated), collection_model_created_on=( @@ -1045,6 +979,8 @@ def _collection_to_search_dict(collection): 'title': collection.title, 'category': collection.category, 'objective': collection.objective, + 'language_code': collection.language_code, + 'tags': collection.tags, 'rank': _get_search_rank(collection.id), } doc.update(_collection_rights_to_search_dict(rights)) diff --git a/core/domain/collection_services_test.py b/core/domain/collection_services_test.py index 27afa8835275..96c06f529cae 100644 --- a/core/domain/collection_services_test.py +++ b/core/domain/collection_services_test.py @@ -18,7 +18,6 @@ from core.domain import collection_domain from core.domain import collection_services -from core.domain import exp_services from core.domain import rights_manager from core.domain import user_services from core.platform import models @@ -343,8 +342,6 @@ def setUp(self): self.COL_ID_0, self.COL_ID_1, self.COL_ID_2, self.COL_ID_3, self.COL_ID_4]) - def _summaries_to_ids(self, col_summaries): - return sorted([col_summary.id for col_summary in col_summaries]) def _create_search_query(self, terms, categories): query = ' '.join(terms) @@ -355,9 +352,9 @@ def _create_search_query(self, terms, categories): def test_get_collection_summaries_with_no_query(self): # An empty query should return all collections. - (col_summaries, search_cursor) = ( - collection_services.get_collection_summaries_matching_query('')) - self.assertEqual(self._summaries_to_ids(col_summaries), [ + (col_ids, search_cursor) = ( + collection_services.get_collection_ids_matching_query('')) + self.assertEqual(sorted(col_ids), [ self.COL_ID_0, self.COL_ID_1, self.COL_ID_2, self.COL_ID_3, self.COL_ID_4 ]) @@ -369,11 +366,9 @@ def test_get_collection_summaries_with_deleted_collections(self): collection_services.delete_collection(self.owner_id, self.COL_ID_2) collection_services.delete_collection(self.owner_id, self.COL_ID_4) - col_summaries = ( - collection_services.get_collection_summaries_matching_query(''))[0] - self.assertEqual( - self._summaries_to_ids(col_summaries), - [self.COL_ID_1, self.COL_ID_3]) + col_ids = ( + collection_services.get_collection_ids_matching_query(''))[0] + self.assertEqual(sorted(col_ids), [self.COL_ID_1, self.COL_ID_3]) collection_services.delete_collection(self.owner_id, self.COL_ID_1) collection_services.delete_collection(self.owner_id, self.COL_ID_3) @@ -381,57 +376,48 @@ def test_get_collection_summaries_with_deleted_collections(self): # If no collections are loaded, a blank query should not get any # collections. self.assertEqual( - collection_services.get_collection_summaries_matching_query(''), + collection_services.get_collection_ids_matching_query(''), ([], None)) def test_search_collection_summaries(self): # Search within the 'Architecture' category. - col_summaries = ( - collection_services.get_collection_summaries_matching_query( + col_ids = ( + collection_services.get_collection_ids_matching_query( self._create_search_query([], ['Architecture'])))[0] - self.assertEqual( - self._summaries_to_ids(col_summaries), [self.COL_ID_0]) + self.assertEqual(col_ids, [self.COL_ID_0]) # Search for collections containing 'Oppia'. - col_summaries = ( - collection_services.get_collection_summaries_matching_query( + col_ids = ( + collection_services.get_collection_ids_matching_query( self._create_search_query(['Oppia'], [])))[0] - self.assertEqual( - self._summaries_to_ids(col_summaries), - [self.COL_ID_1, self.COL_ID_2]) + self.assertEqual(sorted(col_ids), [self.COL_ID_1, self.COL_ID_2]) # Search for collections containing 'Oppia' and 'Introduce'. - col_summaries = ( - collection_services.get_collection_summaries_matching_query( + col_ids = ( + collection_services.get_collection_ids_matching_query( self._create_search_query(['Oppia', 'Introduce'], [])))[0] - self.assertEqual( - self._summaries_to_ids(col_summaries), - [self.COL_ID_1, self.COL_ID_2]) + self.assertEqual(sorted(col_ids), [self.COL_ID_1, self.COL_ID_2]) # Search for collections containing 'England'. - col_summaries = ( - collection_services.get_collection_summaries_matching_query( + col_ids = ( + collection_services.get_collection_ids_matching_query( self._create_search_query(['England'], [])))[0] - self.assertEqual( - self._summaries_to_ids(col_summaries), [self.COL_ID_0]) + self.assertEqual(col_ids, [self.COL_ID_0]) # Search for collections containing 'in'. - col_summaries = ( - collection_services.get_collection_summaries_matching_query( + col_ids = ( + collection_services.get_collection_ids_matching_query( self._create_search_query(['in'], [])))[0] self.assertEqual( - self._summaries_to_ids(col_summaries), - [self.COL_ID_0, self.COL_ID_2, self.COL_ID_4]) + sorted(col_ids), [self.COL_ID_0, self.COL_ID_2, self.COL_ID_4]) # Search for collections containing 'in' in the 'Architecture' and # 'Welcome' categories. - col_summaries = ( - collection_services.get_collection_summaries_matching_query( + col_ids = ( + collection_services.get_collection_ids_matching_query( self._create_search_query( ['in'], ['Architecture', 'Welcome'])))[0] - self.assertEqual( - self._summaries_to_ids(col_summaries), - [self.COL_ID_0, self.COL_ID_2]) + self.assertEqual(sorted(col_ids), [self.COL_ID_0, self.COL_ID_2]) def test_collection_summaries_pagination_in_filled_search_results(self): # Ensure the maximum number of collections that can fit on the search @@ -444,28 +430,28 @@ def test_collection_summaries_pagination_in_filled_search_results(self): found_col_ids = [] # Page 1: 2 initial collections. - (col_summaries, search_cursor) = ( - collection_services.get_collection_summaries_matching_query( + (col_ids, search_cursor) = ( + collection_services.get_collection_ids_matching_query( '', None)) - self.assertEqual(len(col_summaries), 2) + self.assertEqual(len(col_ids), 2) self.assertIsNotNone(search_cursor) - found_col_ids += self._summaries_to_ids(col_summaries) + found_col_ids += col_ids # Page 2: 2 more collections. - (col_summaries, search_cursor) = ( - collection_services.get_collection_summaries_matching_query( + (col_ids, search_cursor) = ( + collection_services.get_collection_ids_matching_query( '', search_cursor)) - self.assertEqual(len(col_summaries), 2) + self.assertEqual(len(col_ids), 2) self.assertIsNotNone(search_cursor) - found_col_ids += self._summaries_to_ids(col_summaries) + found_col_ids += col_ids # Page 3: 1 final collection. - (col_summaries, search_cursor) = ( - collection_services.get_collection_summaries_matching_query( + (col_ids, search_cursor) = ( + collection_services.get_collection_ids_matching_query( '', search_cursor)) - self.assertEqual(len(col_summaries), 1) + self.assertEqual(len(col_ids), 1) self.assertIsNone(search_cursor) - found_col_ids += self._summaries_to_ids(col_summaries) + found_col_ids += col_ids # Validate all collections were seen. self.assertEqual(sorted(found_col_ids), [ @@ -607,16 +593,11 @@ def mock_delete_docs(doc_ids, index): collection_services.delete_collection( self.owner_id, self.COLLECTION_ID) - def test_create_new_collection_error_cases(self): - collection = collection_domain.Collection.create_default_collection( - self.COLLECTION_ID, '', '', '') - with self.assertRaisesRegexp(Exception, 'between 1 and 50 characters'): - collection_services.save_new_collection(self.owner_id, collection) - - collection = collection_domain.Collection.create_default_collection( - self.COLLECTION_ID, 'title', '', '') - with self.assertRaisesRegexp(Exception, 'between 1 and 50 characters'): - collection_services.save_new_collection(self.owner_id, collection) + def test_create_new_collection(self): + # Test that creating a new collection (with an empty title, etc.) + # succeeds. + collection_domain.Collection.create_default_collection( + self.COLLECTION_ID) def test_save_and_retrieve_collection(self): collection = self.save_new_valid_collection( @@ -863,6 +844,44 @@ def test_update_collection_objective(self): self.COLLECTION_ID) self.assertEqual(collection.objective, 'Some new objective') + def test_update_collection_language_code(self): + # Verify initial language code. + collection = collection_services.get_collection_by_id( + self.COLLECTION_ID) + self.assertEqual(collection.language_code, 'en') + + # Update the language code. + collection_services.update_collection( + self.owner_id, self.COLLECTION_ID, [{ + 'cmd': collection_domain.CMD_EDIT_COLLECTION_PROPERTY, + 'property_name': 'language_code', + 'new_value': 'fi' + }], 'Changed the language to Finnish') + + # Verify the language is different. + collection = collection_services.get_collection_by_id( + self.COLLECTION_ID) + self.assertEqual(collection.language_code, 'fi') + + def test_update_collection_tags(self): + # Verify initial tags. + collection = collection_services.get_collection_by_id( + self.COLLECTION_ID) + self.assertEqual(collection.tags, []) + + # Update the tags. + collection_services.update_collection( + self.owner_id, self.COLLECTION_ID, [{ + 'cmd': collection_domain.CMD_EDIT_COLLECTION_PROPERTY, + 'property_name': 'tags', + 'new_value': ['test'] + }], 'Add a new tag') + + # Verify that the tags are different. + collection = collection_services.get_collection_by_id( + self.COLLECTION_ID) + self.assertEqual(collection.tags, ['test']) + def test_update_collection_node_prerequisite_skills(self): # Verify initial prerequisite skills are empty. collection = collection_services.get_collection_by_id( @@ -908,103 +927,6 @@ def test_update_collection_node_acquired_skills(self): collection_node = collection.get_node(self.EXPLORATION_ID) self.assertEqual(collection_node.acquired_skills, ['third', 'fourth']) - -class CollectionLearnerDictTests(CollectionServicesUnitTests): - """Test get_learner_collection_dict_by_id.""" - - EXP_ID = 'exploration_id' - EXP_ID_1 = 'exp_id1' - - def setUp(self): - super(CollectionLearnerDictTests, self).setUp() - - def test_get_learner_dict_with_deleted_exp_fails_validation(self): - self.save_new_valid_collection( - self.COLLECTION_ID, self.owner_id, exploration_id=self.EXP_ID) - collection_services.get_learner_collection_dict_by_id( - self.COLLECTION_ID, self.owner_id) - - exp_services.delete_exploration(self.owner_id, self.EXP_ID) - - with self.assertRaisesRegexp( - utils.ValidationError, - 'Expected collection to only reference valid explorations, but ' - 'found an exploration with ID: exploration_id'): - collection_services.get_learner_collection_dict_by_id( - self.COLLECTION_ID, self.owner_id) - - def test_get_learner_dict_when_referencing_inaccessible_explorations(self): - self.save_new_default_collection(self.COLLECTION_ID, self.owner_id) - self.save_new_valid_exploration(self.EXP_ID, self.editor_id) - collection_services.update_collection( - self.owner_id, self.COLLECTION_ID, [{ - 'cmd': collection_domain.CMD_ADD_COLLECTION_NODE, - 'exploration_id': self.EXP_ID - }], 'Added another creator\'s private exploration') - - # A collection cannot access someone else's private exploration. - rights_manager.publish_collection(self.owner_id, self.COLLECTION_ID) - with self.assertRaisesRegexp( - utils.ValidationError, - 'Expected collection to only reference valid explorations, but ' - 'found an exploration with ID: exploration_id'): - collection_services.get_learner_collection_dict_by_id( - self.COLLECTION_ID, self.owner_id) - - # After the exploration is published, the dict can now be created. - rights_manager.publish_exploration(self.editor_id, self.EXP_ID) - collection_services.get_learner_collection_dict_by_id( - self.COLLECTION_ID, self.owner_id) - - def test_get_learner_dict_with_private_exp_fails_validation(self): - self.save_new_valid_collection( - self.COLLECTION_ID, self.owner_id, exploration_id=self.EXP_ID) - - # Since both the collection and exploration are private, the learner - # dict can be created. - collection_services.get_learner_collection_dict_by_id( - self.COLLECTION_ID, self.owner_id) - - # A public collection referencing a private exploration is bad, however. - rights_manager.publish_collection(self.owner_id, self.COLLECTION_ID) - with self.assertRaisesRegexp( - utils.ValidationError, - 'Cannot reference a private exploration within a public ' - 'collection, exploration ID: exploration_id'): - collection_services.get_learner_collection_dict_by_id( - self.COLLECTION_ID, self.owner_id) - - # After the exploration is published, the learner dict can be crated - # again. - rights_manager.publish_exploration(self.owner_id, self.EXP_ID) - collection_services.get_learner_collection_dict_by_id( - self.COLLECTION_ID, self.owner_id) - - def test_get_learner_dict_with_allowed_private_exps(self): - self.save_new_valid_collection( - self.COLLECTION_ID, self.owner_id, exploration_id=self.EXP_ID) - self.save_new_valid_exploration(self.EXP_ID_1, self.editor_id) - collection_services.update_collection( - self.owner_id, self.COLLECTION_ID, [{ - 'cmd': collection_domain.CMD_ADD_COLLECTION_NODE, - 'exploration_id': self.EXP_ID_1 - }], 'Added another creator\'s private exploration') - - rights_manager.publish_collection(self.owner_id, self.COLLECTION_ID) - - collection_dict = collection_services.get_learner_collection_dict_by_id( - self.COLLECTION_ID, self.owner_id, allow_invalid_explorations=True) - - # The author's private exploration will be contained in the public - # collection since invalid explorations are being allowed, but the - # private exploration of another author will not. - collection_node_dicts = collection_dict['nodes'] - self.assertEqual( - collection_node_dicts[0]['exploration_summary']['id'], - self.EXP_ID) - self.assertIsNone(collection_node_dicts[1]['exploration_summary']) - - def _get_node_change_list(exploration_id, property_name, new_value): """Generates a change list for a single collection node change.""" return [{ diff --git a/core/domain/config_domain.py b/core/domain/config_domain.py index 4daa3ea6d113..9758ca8f1736 100644 --- a/core/domain/config_domain.py +++ b/core/domain/config_domain.py @@ -1,3 +1,5 @@ +# coding: utf-8 +# # Copyright 2014 The Oppia Authors. All Rights Reserved. # # Licensed under the Apache License, Version 2.0 (the "License"); diff --git a/core/domain/exp_domain.py b/core/domain/exp_domain.py index 64a33e8ace9a..7d6e0c308ff3 100644 --- a/core/domain/exp_domain.py +++ b/core/domain/exp_domain.py @@ -1316,7 +1316,9 @@ def __init__(self, exploration_id, title, category, objective, @classmethod def create_default_exploration( - cls, exploration_id, title, category, objective='', + cls, exploration_id, title=feconf.DEFAULT_EXPLORATION_TITLE, + category=feconf.DEFAULT_EXPLORATION_CATEGORY, + objective=feconf.DEFAULT_EXPLORATION_OBJECTIVE, language_code=feconf.DEFAULT_LANGUAGE_CODE): init_state_dict = State.create_default_state( feconf.DEFAULT_INIT_STATE_NAME, is_initial_state=True).to_dict() @@ -1339,8 +1341,8 @@ def from_dict( # from an ExplorationModel/dictionary MUST be exhaustive and complete. exploration = cls.create_default_exploration( exploration_dict['id'], - exploration_dict['title'], - exploration_dict['category'], + title=exploration_dict['title'], + category=exploration_dict['category'], objective=exploration_dict['objective'], language_code=exploration_dict['language_code']) exploration.tags = exploration_dict['tags'] @@ -1432,13 +1434,15 @@ def validate(self, strict=False): if not isinstance(self.title, basestring): raise utils.ValidationError( 'Expected title to be a string, received %s' % self.title) - utils.require_valid_name(self.title, 'the exploration title') + utils.require_valid_name( + self.title, 'the exploration title', allow_empty=True) if not isinstance(self.category, basestring): raise utils.ValidationError( 'Expected category to be a string, received %s' % self.category) - utils.require_valid_name(self.category, 'the exploration category') + utils.require_valid_name( + self.category, 'the exploration category', allow_empty=True) if not isinstance(self.objective, basestring): raise utils.ValidationError( @@ -1642,6 +1646,14 @@ def validate(self, strict=False): except utils.ValidationError as e: warnings_list.append(unicode(e)) + if not self.title: + warnings_list.append( + 'A title must be specified (in the \'Settings\' tab).') + + if not self.category: + warnings_list.append( + 'A category must be specified (in the \'Settings\' tab).') + if not self.objective: warnings_list.append( 'An objective must be specified (in the \'Settings\' tab).' @@ -2404,8 +2416,8 @@ def _migrate_to_latest_yaml_version( if not (1 <= exploration_schema_version <= cls.CURRENT_EXP_SCHEMA_VERSION): raise Exception( - 'Sorry, we can only process v1 to v%s YAML files at ' - 'present.' % cls.CURRENT_EXP_SCHEMA_VERSION) + 'Sorry, we can only process v1 to v%s exploration YAML files ' + 'at present.' % cls.CURRENT_EXP_SCHEMA_VERSION) if exploration_schema_version == 1: exploration_dict = cls._convert_v1_dict_to_v2_dict( exploration_dict) @@ -2538,6 +2550,7 @@ def to_player_dict(self): for (state_name, state) in self.states.iteritems() }, 'title': self.title, + 'language_code': self.language_code, } def get_gadget_types(self): @@ -2553,18 +2566,20 @@ def get_gadget_types(self): def get_interaction_ids(self): """Get all interaction ids used in this exploration.""" return list(set([ - state.interaction.id for state in self.states.itervalues()])) + state.interaction.id for state in self.states.itervalues() + if state.interaction.id is not None])) class ExplorationSummary(object): """Domain object for an Oppia exploration summary.""" def __init__(self, exploration_id, title, category, objective, - language_code, tags, ratings, status, + language_code, tags, ratings, scaled_average_rating, status, community_owned, owner_ids, editor_ids, viewer_ids, contributor_ids, contributors_summary, version, exploration_model_created_on, - exploration_model_last_updated): + exploration_model_last_updated, + first_published_msec): """'ratings' is a dict whose keys are '1', '2', '3', '4', '5' and whose values are nonnegative integers representing frequency counts. Note that the keys need to be strings in order for this dict to be @@ -2577,6 +2592,7 @@ def __init__(self, exploration_id, title, category, objective, self.language_code = language_code self.tags = tags self.ratings = ratings + self.scaled_average_rating = scaled_average_rating self.status = status self.community_owned = community_owned self.owner_ids = owner_ids @@ -2587,3 +2603,4 @@ def __init__(self, exploration_id, title, category, objective, self.version = version self.exploration_model_created_on = exploration_model_created_on self.exploration_model_last_updated = exploration_model_last_updated + self.first_published_msec = first_published_msec diff --git a/core/domain/exp_domain_test.py b/core/domain/exp_domain_test.py index 297c88bf616f..a6c7f971d13b 100644 --- a/core/domain/exp_domain_test.py +++ b/core/domain/exp_domain_test.py @@ -272,21 +272,14 @@ class ExplorationDomainUnitTests(test_utils.GenericTestBase): # unit tests. Also, all validation errors should be covered in the tests. def test_validation(self): """Test validation of explorations.""" - exploration = exp_domain.Exploration.create_default_exploration( - 'exp_id', '', '') + exploration = exp_domain.Exploration.create_default_exploration('eid') exploration.init_state_name = '' exploration.states = {} - self._assert_validation_error( - exploration, 'between 1 and 50 characters') - exploration.title = 'Hello #' self._assert_validation_error(exploration, 'Invalid character #') exploration.title = 'Title' - self._assert_validation_error( - exploration, 'between 1 and 50 characters') - exploration.category = 'Category' # Note: If '/' ever becomes a valid state name, ensure that the rule @@ -567,8 +560,7 @@ def test_validation(self): def test_fallbacks_validation(self): """Test validation of state fallbacks.""" - exploration = exp_domain.Exploration.create_default_exploration( - 'exp_id', 'Title', 'Category') + exploration = exp_domain.Exploration.create_default_exploration('eid') exploration.objective = 'Objective' init_state = exploration.states[exploration.init_state_name] init_state.update_interaction_id('TextInput') @@ -664,8 +656,7 @@ def test_fallbacks_validation(self): def test_tag_validation(self): """Test validation of exploration tags.""" - exploration = exp_domain.Exploration.create_default_exploration( - 'exp_id', 'Title', 'Category') + exploration = exp_domain.Exploration.create_default_exploration('eid') exploration.objective = 'Objective' init_state = exploration.states[exploration.init_state_name] init_state.update_interaction_id('EndExploration') @@ -851,14 +842,28 @@ def test_exploration_get_gadget_types(self): ['AnotherGadget', 'TestGadget'] ) - def test_objective_validation(self): - """Test that objectives are validated only in 'strict' mode.""" + def test_title_category_and_objective_validation(self): + """Test that titles, categories and objectives are validated only in + 'strict' mode. + """ self.save_new_valid_exploration( - 'exp_id', 'user@example.com', title='Title', category='Category', + 'exp_id', 'user@example.com', title='', category='', objective='', end_state_name='End') exploration = exp_services.get_exploration_by_id('exp_id') exploration.validate() + with self.assertRaisesRegexp( + utils.ValidationError, 'title must be specified' + ): + exploration.validate(strict=True) + exploration.title = 'A title' + + with self.assertRaisesRegexp( + utils.ValidationError, 'category must be specified' + ): + exploration.validate(strict=True) + exploration.category = 'A category' + with self.assertRaisesRegexp( utils.ValidationError, 'objective must be specified' ): @@ -870,24 +875,20 @@ def test_objective_validation(self): def test_is_demo_property(self): """Test the is_demo property.""" - demo = exp_domain.Exploration.create_default_exploration( - '0', 'title', 'category') + demo = exp_domain.Exploration.create_default_exploration('0') self.assertEqual(demo.is_demo, True) - notdemo1 = exp_domain.Exploration.create_default_exploration( - 'a', 'title', 'category') + notdemo1 = exp_domain.Exploration.create_default_exploration('a') self.assertEqual(notdemo1.is_demo, False) - notdemo2 = exp_domain.Exploration.create_default_exploration( - 'abcd', 'title', 'category') + notdemo2 = exp_domain.Exploration.create_default_exploration('abcd') self.assertEqual(notdemo2.is_demo, False) def test_exploration_export_import(self): """Test that to_dict and from_dict preserve all data within an exploration. """ - demo = exp_domain.Exploration.create_default_exploration( - '0', 'title', 'category') + demo = exp_domain.Exploration.create_default_exploration('0') demo_dict = demo.to_dict() exp_from_dict = exp_domain.Exploration.from_dict(demo_dict) self.assertEqual(exp_from_dict.to_dict(), demo_dict) @@ -897,8 +898,7 @@ def test_interaction_with_none_id_is_not_terminal(self): being false. """ # Default exploration has a default interaction with an ID of None. - demo = exp_domain.Exploration.create_default_exploration( - '0', 'title', 'category') + demo = exp_domain.Exploration.create_default_exploration('0') init_state = demo.states[feconf.DEFAULT_INIT_STATE_NAME] self.assertFalse(init_state.interaction.is_terminal) @@ -909,7 +909,7 @@ class StateExportUnitTests(test_utils.GenericTestBase): def test_export_state_to_dict(self): """Test exporting a state to a dict.""" exploration = exp_domain.Exploration.create_default_exploration( - 'A different exploration_id', 'Title', 'Category') + 'exp_id') exploration.add_states(['New state']) state_dict = exploration.states['New state'].to_dict() @@ -943,7 +943,7 @@ class YamlCreationUnitTests(test_utils.GenericTestBase): def test_yaml_import_and_export(self): """Test the from_yaml() and to_yaml() methods.""" exploration = exp_domain.Exploration.create_default_exploration( - self.EXP_ID, 'Title', 'Category') + self.EXP_ID, title='Title', category='Category') exploration.add_states(['New state']) self.assertEqual(len(exploration.states), 2) @@ -1808,7 +1808,7 @@ def test_convert_exploration_to_player_dict(self): second_state_name = 'first state' exploration = exp_domain.Exploration.create_default_exploration( - 'eid', exp_title, 'Category') + 'eid', title=exp_title, category='Category') exploration.add_states([second_state_name]) def _get_default_state_dict(content_str, dest_name): @@ -1847,6 +1847,7 @@ def _get_default_state_dict(content_str, dest_name): 'skin_customizations': ( exp_domain.SkinInstance._get_default_skin_customizations() # pylint: disable=protected-access ), + 'language_code': 'en', }) @@ -1855,8 +1856,7 @@ class StateOperationsUnitTests(test_utils.GenericTestBase): def test_delete_state(self): """Test deletion of states.""" - exploration = exp_domain.Exploration.create_default_exploration( - 'eid', 'Title', 'Category') + exploration = exp_domain.Exploration.create_default_exploration('eid') exploration.add_states(['first state']) with self.assertRaisesRegexp( @@ -1872,8 +1872,7 @@ def test_delete_state(self): def test_state_operations(self): """Test adding, updating and checking existence of states.""" - exploration = exp_domain.Exploration.create_default_exploration( - 'eid', 'Title', 'Category') + exploration = exp_domain.Exploration.create_default_exploration('eid') self.assertNotIn('invalid_state_name', exploration.states) self.assertEqual(len(exploration.states), 1) @@ -1928,6 +1927,8 @@ def test_state_operations(self): exploration.states['State 2'].update_interaction_id('TextInput') # Other miscellaneous requirements for validation + exploration.title = 'Title' + exploration.category = 'Category' exploration.objective = 'Objective' # The exploration should NOT be terminable even though it has a state @@ -1957,8 +1958,7 @@ class GadgetOperationsUnitTests(test_utils.GenericTestBase): def test_gadget_operations(self): """Test deletion of gadgets.""" - exploration = exp_domain.Exploration.create_default_exploration( - 'eid', 'A title', 'A category') + exploration = exp_domain.Exploration.create_default_exploration('eid') with self.swap(feconf, 'ALLOWED_GADGETS', TEST_GADGETS): exploration.add_gadget(TEST_GADGET_DICT, 'bottom') diff --git a/core/domain/exp_jobs_one_off_test.py b/core/domain/exp_jobs_one_off_test.py index 8dacddf4957d..8182e4df43d3 100644 --- a/core/domain/exp_jobs_one_off_test.py +++ b/core/domain/exp_jobs_one_off_test.py @@ -25,6 +25,7 @@ from core.tests import test_utils import feconf import utils + (job_models, exp_models,) = models.Registry.import_models([ models.NAMES.job, models.NAMES.exploration]) search_services = models.Registry.import_search_services() @@ -166,6 +167,8 @@ def _run_batch_job_once_and_verify_output( exploration = exp_services.get_exploration_by_id(exp_id) exploration_model_last_updated = exploration.last_updated exploration_model_created_on = exploration.created_on + first_published_msec = ( + exp_rights_model.first_published_msec) # Manually create the expected summary specifying title, # category, etc. @@ -177,6 +180,7 @@ def _run_batch_job_once_and_verify_output( exploration.language_code, exploration.tags, feconf.get_empty_ratings(), + feconf.EMPTY_SCALED_AVERAGE_RATING, spec['status'], exp_rights_model.community_owned, exp_rights_model.owner_ids, @@ -186,7 +190,8 @@ def _run_batch_job_once_and_verify_output( {admin_id: 1}, exploration.version, exploration_model_created_on, - exploration_model_last_updated) + exploration_model_last_updated, + first_published_msec) # Note: Calling constructor for fields that are not required # and have no default value does not work, because @@ -419,6 +424,174 @@ def test_nonhuman_committers_not_counted(self): feconf.MIGRATION_BOT_USERNAME, exploration_summary.contributor_ids) +class ExplorationContributorsSummaryOneOffJobTest(test_utils.GenericTestBase): + ONE_OFF_JOB_MANAGERS_FOR_TESTS = [ + exp_jobs_one_off.ExplorationContributorsSummaryOneOffJob] + + EXP_ID = 'exp_id' + + USERNAME_A = 'usernamea' + USERNAME_B = 'usernameb' + EMAIL_A = 'emaila@example.com' + EMAIL_B = 'emailb@example.com' + + def setUp(self): + super(ExplorationContributorsSummaryOneOffJobTest, self).setUp() + self.signup(self.EMAIL_A, self.USERNAME_A) + self.signup(self.EMAIL_B, self.USERNAME_B) + + def test_contributors_for_valid_nonrevert_contribution(self): + """Test that if only non-revert commits are made by + contributor then the contributions summary shows same + exact number of commits for that contributor's ID + """ + + user_a_id = self.get_user_id_from_email(self.EMAIL_A) + + # Let USER A make three commits. + exploration = self.save_new_valid_exploration( + self.EXP_ID, user_a_id, title='Exploration Title') + + exp_services.update_exploration( + user_a_id, self.EXP_ID, [{ + 'cmd': 'edit_exploration_property', + 'property_name': 'title', + 'new_value': 'New Exploration Title' + }], 'Changed title.') + + exp_services.update_exploration( + user_a_id, self.EXP_ID, [{ + 'cmd': 'edit_exploration_property', + 'property_name': 'objective', + 'new_value': 'New Objective' + }], 'Changed Objective.') + + # Run the job to compute contributors summary + job_id = exp_jobs_one_off.ExplorationContributorsSummaryOneOffJob.create_new() # pylint: disable=line-too-long + exp_jobs_one_off.ExplorationContributorsSummaryOneOffJob.enqueue(job_id) + self.process_and_flush_pending_tasks() + + exploration_summary = exp_services.get_exploration_summary_by_id( + exploration.id) + + self.assertEqual( + 3, exploration_summary.contributors_summary[user_a_id]) + + def test_contributors_with_only_reverts_not_included(self): + """Test that if only reverts are made by contributor then the + contributions summary shouldn’t contain that contributor’s ID + """ + + user_a_id = self.get_user_id_from_email(self.EMAIL_A) + user_b_id = self.get_user_id_from_email(self.EMAIL_B) + + # Let USER A make three commits. + exploration = self.save_new_valid_exploration( + self.EXP_ID, user_a_id, title='Exploration Title 1') + + exp_services.update_exploration( + user_a_id, self.EXP_ID, [{ + 'cmd': 'edit_exploration_property', + 'property_name': 'title', + 'new_value': 'New Exploration Title' + }], 'Changed title.') + exp_services.update_exploration( + user_a_id, self.EXP_ID, [{ + 'cmd': 'edit_exploration_property', + 'property_name': 'objective', + 'new_value': 'New Objective' + }], 'Changed Objective.') + + # Let the second user revert version 3 to version 2 + exp_services.revert_exploration(user_b_id, self.EXP_ID, 3, 2) + + # Run the job to compute the contributors summary. + job_id = exp_jobs_one_off.ExplorationContributorsSummaryOneOffJob.create_new() # pylint: disable=line-too-long + exp_jobs_one_off.ExplorationContributorsSummaryOneOffJob.enqueue(job_id) + self.process_and_flush_pending_tasks() + + exploration_summary = exp_services.get_exploration_summary_by_id( + exploration.id) + + # Check that the contributors_summary does not contains user_b_id + self.assertNotIn(user_b_id, exploration_summary.contributors_summary) + + # Check that the User A has only 2 commits after user b has reverted + # to version 2 + self.assertEquals(2, exploration_summary.contributors_summary[user_a_id]) # pylint: disable=line-too-long + + def test_reverts_not_counted(self): + """Test that if both non-revert commits and revert are + made by contributor then the contributions summary shows + only non-revert commits for that contributor. However, + the commits made after the version to which we have reverted + shouldn't be counted either. + """ + + user_a_id = self.get_user_id_from_email(self.EMAIL_A) + + # Let USER A make 3 non-revert commits + exploration = self.save_new_valid_exploration( + self.EXP_ID, user_a_id, title='Exploration Title') + exp_services.update_exploration( + user_a_id, self.EXP_ID, [{ + 'cmd': 'edit_exploration_property', + 'property_name': 'title', + 'new_value': 'New Exploration Title' + }], 'Changed title.') + exp_services.update_exploration( + user_a_id, self.EXP_ID, [{ + 'cmd': 'edit_exploration_property', + 'property_name': 'objective', + 'new_value': 'New Objective' + }], 'Changed Objective.') + + # Let USER A revert version 3 to version 2 + exp_services.revert_exploration(user_a_id, self.EXP_ID, 3, 2) + + # Run the job to compute the contributor summary. + job_id = exp_jobs_one_off.ExplorationContributorsSummaryOneOffJob.create_new() # pylint: disable=line-too-long + exp_jobs_one_off.ExplorationContributorsSummaryOneOffJob.enqueue(job_id) + self.process_and_flush_pending_tasks() + + # Check that USER A's number of contributions is equal to 2 + exploration_summary = exp_services.get_exploration_summary_by_id( + exploration.id) + self.assertEqual(2, exploration_summary.contributors_summary[user_a_id]) + + def test_nonhuman_committers_not_counted(self): + """Test that only human committers are counted as contributors. + """ + # Create a commit with the system user id. + exploration = self.save_new_valid_exploration( + self.EXP_ID, feconf.SYSTEM_COMMITTER_ID, title='Original Title') + + # Create commits with all the system user ids + for system_id in feconf.SYSTEM_USER_IDS: + exp_services.update_exploration( + system_id, self.EXP_ID, [{ + 'cmd': 'edit_exploration_property', + 'property_name': 'title', + 'new_value': 'Title changed by %s' % system_id + }], 'Changed title.') + + # Run the job to compute the contributor summary. + job_id = exp_jobs_one_off.ExplorationContributorsSummaryOneOffJob.create_new() # pylint: disable=line-too-long + exp_jobs_one_off.ExplorationContributorsSummaryOneOffJob.enqueue(job_id) + self.process_and_flush_pending_tasks() + + # Check that no system id was added to the exploration's + # contributor's summary + + exploration_summary = exp_services.get_exploration_summary_by_id( + exploration.id) + + for system_id in feconf.SYSTEM_USER_IDS: + self.assertNotIn( + system_id, + exploration_summary.contributors_summary) + + class OneOffReindexExplorationsJobTest(test_utils.GenericTestBase): EXP_ID = 'exp_id' @@ -430,7 +603,9 @@ def setUp(self): self.owner_id = self.get_user_id_from_email(self.OWNER_EMAIL) explorations = [exp_domain.Exploration.create_default_exploration( - '%s%s' % (self.EXP_ID, i), 'title %d' % i, 'category%d' % i + '%s%s' % (self.EXP_ID, i), + title='title %d' % i, + category='category%d' % i ) for i in xrange(5)] for exp in explorations: @@ -482,7 +657,6 @@ def setUp(self): # Setup user who will own the test explorations. self.albert_id = self.get_user_id_from_email(self.ALBERT_EMAIL) self.signup(self.ALBERT_EMAIL, self.ALBERT_NAME) - self.process_and_flush_pending_tasks() def test_migration_job_does_not_convert_up_to_date_exp(self): @@ -492,7 +666,7 @@ def test_migration_job_does_not_convert_up_to_date_exp(self): # Create a new, default exploration that should not be affected by the # job. exploration = exp_domain.Exploration.create_default_exploration( - self.VALID_EXP_ID, 'title', 'category') + self.VALID_EXP_ID, title='title', category='category') init_state = exploration.states[exploration.init_state_name] init_state.update_interaction_id('EndExploration') init_state.interaction.default_outcome = None diff --git a/core/domain/exp_services.py b/core/domain/exp_services.py index c2ab13be4415..c92c411ebe74 100644 --- a/core/domain/exp_services.py +++ b/core/domain/exp_services.py @@ -25,6 +25,7 @@ import copy import datetime import logging +import math import os import pprint import StringIO @@ -158,13 +159,15 @@ def get_exploration_summary_from_model(exp_summary_model): exp_summary_model.id, exp_summary_model.title, exp_summary_model.category, exp_summary_model.objective, exp_summary_model.language_code, exp_summary_model.tags, - exp_summary_model.ratings, exp_summary_model.status, - exp_summary_model.community_owned, exp_summary_model.owner_ids, - exp_summary_model.editor_ids, exp_summary_model.viewer_ids, + exp_summary_model.ratings, exp_summary_model.scaled_average_rating, + exp_summary_model.status, exp_summary_model.community_owned, + exp_summary_model.owner_ids, exp_summary_model.editor_ids, + exp_summary_model.viewer_ids, exp_summary_model.contributor_ids, exp_summary_model.contributors_summary, exp_summary_model.version, exp_summary_model.exploration_model_created_on, - exp_summary_model.exploration_model_last_updated + exp_summary_model.exploration_model_last_updated, + exp_summary_model.first_published_msec ) @@ -374,6 +377,22 @@ def get_featured_exploration_summaries(): exp_models.ExpSummaryModel.get_featured()) +def get_top_rated_exploration_summaries(): + """Returns a dict with top rated exploration summary domain objects, + keyed by their id. + """ + return _get_exploration_summaries_from_models( + exp_models.ExpSummaryModel.get_top_rated()) + + +def get_recently_published_exploration_summaries(): + """Returns a dict with all featured exploration summary domain objects, + keyed by their id. + """ + return _get_exploration_summaries_from_models( + exp_models.ExpSummaryModel.get_recently_published()) + + def get_all_exploration_summaries(): """Returns a dict with all exploration summary domain objects, keyed by their id. @@ -1013,15 +1032,7 @@ def update_exploration( exploration = apply_change_list(exploration_id, change_list) _save_exploration(committer_id, exploration, commit_message, change_list) - # If there is an existing exploration draft for this user, clear it. - exp_user_data = user_models.ExplorationUserDataModel.get( - committer_id, exploration_id) - if exp_user_data: - exp_user_data.draft_change_list = None - exp_user_data.draft_change_list_last_updated = None - exp_user_data.draft_change_list_exp_version = None - exp_user_data.put() - + discard_draft(exploration_id, committer_id) # Update summary of changed exploration. update_exploration_summary(exploration.id, committer_id) user_services.add_edited_exploration_id(committer_id, exploration.id) @@ -1058,10 +1069,13 @@ def compute_summary_of_exploration(exploration, contributor_id_to_add): if exp_summary_model: old_exp_summary = get_exploration_summary_from_model(exp_summary_model) ratings = old_exp_summary.ratings or feconf.get_empty_ratings() + scaled_average_rating = get_scaled_average_rating( + old_exp_summary.ratings) contributor_ids = old_exp_summary.contributor_ids or [] contributors_summary = old_exp_summary.contributors_summary or {} else: ratings = feconf.get_empty_ratings() + scaled_average_rating = feconf.EMPTY_SCALED_AVERAGE_RATING contributor_ids = [] contributors_summary = {} @@ -1087,15 +1101,16 @@ def compute_summary_of_exploration(exploration, contributor_id_to_add): exploration_model_last_updated = datetime.datetime.fromtimestamp( _get_last_updated_by_human_ms(exploration.id) / 1000.0) exploration_model_created_on = exploration.created_on - + first_published_msec = exp_rights.first_published_msec exp_summary = exp_domain.ExplorationSummary( exploration.id, exploration.title, exploration.category, exploration.objective, exploration.language_code, - exploration.tags, ratings, exp_rights.status, + exploration.tags, ratings, scaled_average_rating, exp_rights.status, exp_rights.community_owned, exp_rights.owner_ids, exp_rights.editor_ids, exp_rights.viewer_ids, contributor_ids, contributors_summary, exploration.version, - exploration_model_created_on, exploration_model_last_updated) + exploration_model_created_on, exploration_model_last_updated, + first_published_msec) return exp_summary @@ -1137,6 +1152,7 @@ def save_exploration_summary(exp_summary): language_code=exp_summary.language_code, tags=exp_summary.tags, ratings=exp_summary.ratings, + scaled_average_rating=exp_summary.scaled_average_rating, status=exp_summary.status, community_owned=exp_summary.community_owned, owner_ids=exp_summary.owner_ids, @@ -1148,7 +1164,9 @@ def save_exploration_summary(exp_summary): exploration_model_last_updated=( exp_summary.exploration_model_last_updated), exploration_model_created_on=( - exp_summary.exploration_model_created_on) + exp_summary.exploration_model_created_on), + first_published_msec=( + exp_summary.first_published_msec) ) exp_summary_model.put() @@ -1223,13 +1241,12 @@ def get_demo_exploration_components(demo_path): def save_new_exploration_from_yaml_and_assets( - committer_id, yaml_content, title, category, exploration_id, - assets_list): - """Note that the title and category will be ignored if the YAML - schema version is greater than + committer_id, yaml_content, exploration_id, assets_list): + """Note that the default title and category will be used if the YAML + schema version is less than exp_domain.Exploration.LAST_UNTITLED_SCHEMA_VERSION, - since in that case there will already be a title and category present in - the YAML schema. + since in that case the YAML schema will not have a title and category + present. """ if assets_list is None: assets_list = [] @@ -1244,7 +1261,8 @@ def save_new_exploration_from_yaml_and_assets( # The schema of the YAML file for older explorations did not include # a title and a category; these need to be manually specified. exploration = exp_domain.Exploration.from_untitled_yaml( - exploration_id, title, category, yaml_content) + exploration_id, feconf.DEFAULT_EXPLORATION_TITLE, + feconf.DEFAULT_EXPLORATION_CATEGORY, yaml_content) else: exploration = exp_domain.Exploration.from_yaml( exploration_id, yaml_content) @@ -1294,8 +1312,8 @@ def load_demo(exploration_id): yaml_content, assets_list = get_demo_exploration_components(exp_filename) save_new_exploration_from_yaml_and_assets( - feconf.SYSTEM_COMMITTER_ID, yaml_content, None, None, - exploration_id, assets_list) + feconf.SYSTEM_COMMITTER_ID, yaml_content, exploration_id, + assets_list) publish_exploration_and_update_user_profiles( feconf.SYSTEM_COMMITTER_ID, exploration_id) @@ -1366,6 +1384,42 @@ def _should_index(exp): return rights.status != rights_manager.ACTIVITY_STATUS_PRIVATE +def get_average_rating(ratings): + """Returns the average rating of the ratings as a float. If there are no + ratings, it will return 0. + """ + rating_weightings = {'1': 1, '2': 2, '3': 3, '4': 4, '5': 5} + if ratings: + rating_sum = 0.0 + number_of_ratings = 0.0 + for rating_value, rating_count in ratings.items(): + rating_sum += rating_weightings[rating_value] * rating_count + number_of_ratings += rating_count + if number_of_ratings > 0: + return rating_sum / number_of_ratings + return 0 + + +def get_scaled_average_rating(ratings): + """Returns the lower bound wilson score of the ratings as a float. If + there are no ratings, it will return 0. The confidence of this result is + 95%. + """ + # The following is the number of ratings. + n = sum(ratings.values()) + if n == 0: + return 0 + average_rating = get_average_rating(ratings) + z = 1.9599639715843482 + x = (average_rating - 1) / 4 + # The following calculates the lower bound Wilson Score as documented + # http://www.goproblems.com/test/wilson/wilson.php?v1=0&v2=0&v3=0&v4=&v5=1 + a = x + (z**2)/(2*n) + b = z * math.sqrt((x*(1-x))/n + (z**2)/(4*n**2)) + wilson_score_lower_bound = (a - b)/(1 + z**2/n) + return 1 + 4 * wilson_score_lower_bound + + def get_search_rank_from_exp_summary(exp_summary): """Returns an integer determining the document's rank in search. @@ -1566,14 +1620,11 @@ def reject_suggestion(editor_id, thread_id, exploration_id): thread.put() -def is_draft_version_valid(exp_id, user_id): +def is_version_of_draft_valid(exp_id, version): """Checks if the draft version is the same as the latest version of the exploration.""" - draft_version = user_models.ExplorationUserDataModel.get( - user_id, exp_id).draft_change_list_exp_version - exp_version = get_exploration_by_id(exp_id).version - return draft_version == exp_version + return get_exploration_by_id(exp_id).version == version def create_or_update_draft( @@ -1584,13 +1635,18 @@ def create_or_update_draft( timestamp of the draft. The method assumes that a ExplorationUserDataModel object exists for the given user and exploration.""" - exp_user_data = user_models.ExplorationUserDataModel.get(user_id, exp_id) - if (exp_user_data.draft_change_list and + if (exp_user_data and exp_user_data.draft_change_list and exp_user_data.draft_change_list_last_updated > current_datetime): return + updated_exploration = apply_change_list(exp_id, change_list) updated_exploration.validate(strict=False) + + if exp_user_data is None: + exp_user_data = user_models.ExplorationUserDataModel.create( + user_id, exp_id) + exp_user_data.draft_change_list = change_list exp_user_data.draft_change_list_last_updated = current_datetime exp_user_data.draft_change_list_exp_version = exp_version @@ -1602,7 +1658,22 @@ def get_exp_with_draft_applied(exp_id, user_id): apply it to the exploration.""" exp_user_data = user_models.ExplorationUserDataModel.get(user_id, exp_id) + exploration = get_exploration_by_id(exp_id) return ( apply_change_list(exp_id, exp_user_data.draft_change_list) - if exp_user_data.draft_change_list - else get_exploration_by_id(exp_id)) + if exp_user_data and exp_user_data.draft_change_list and + is_version_of_draft_valid( + exp_id, exp_user_data.draft_change_list_exp_version) + else exploration) + + +def discard_draft(exp_id, user_id): + """Discard the draft for the given user and exploration.""" + + exp_user_data = user_models.ExplorationUserDataModel.get( + user_id, exp_id) + if exp_user_data: + exp_user_data.draft_change_list = None + exp_user_data.draft_change_list_last_updated = None + exp_user_data.draft_change_list_exp_version = None + exp_user_data.put() diff --git a/core/domain/exp_services_test.py b/core/domain/exp_services_test.py index 44179141ad10..1f21e8e96dd7 100644 --- a/core/domain/exp_services_test.py +++ b/core/domain/exp_services_test.py @@ -434,7 +434,6 @@ def test_explorations_are_removed_from_index_when_deleted(self): """Tests that explorations are removed from the search index when deleted. """ - self.save_new_default_exploration(self.EXP_ID, self.owner_id) def mock_delete_docs(doc_ids, index): @@ -447,16 +446,19 @@ def mock_delete_docs(doc_ids, index): with delete_docs_swap: exp_services.delete_exploration(self.owner_id, self.EXP_ID) - def test_create_new_exploration_error_cases(self): + def test_no_errors_are_raised_when_creating_default_exploration(self): exploration = exp_domain.Exploration.create_default_exploration( - self.EXP_ID, '', '') - with self.assertRaisesRegexp(Exception, 'between 1 and 50 characters'): - exp_services.save_new_exploration(self.owner_id, exploration) + self.EXP_ID) + exp_services.save_new_exploration(self.owner_id, exploration) + def test_that_default_exploration_fails_strict_validation(self): exploration = exp_domain.Exploration.create_default_exploration( - self.EXP_ID, 'title', '') - with self.assertRaisesRegexp(Exception, 'between 1 and 50 characters'): - exp_services.save_new_exploration(self.owner_id, exploration) + self.EXP_ID) + with self.assertRaisesRegexp( + utils.ValidationError, + 'This state does not have any interaction specified.' + ): + exploration.validate(strict=True) def test_save_and_retrieve_exploration(self): self.save_new_valid_exploration(self.EXP_ID, self.owner_id) @@ -1806,6 +1808,8 @@ def test_paging_with_no_commits(self): class ExplorationSearchTests(ExplorationServicesUnitTests): """Test exploration search.""" + USER_ID_1 = 'user_1' + def test_demo_explorations_are_added_to_search_index(self): results, _ = exp_services.search_explorations('Welcome', 2) self.assertEqual(results, []) @@ -1971,6 +1975,47 @@ def mock_search(query_string, index, cursor=None, limit=20, sort='', self.assertEqual(cursor, expected_result_cursor) self.assertEqual(result, doc_ids) + def test_get_average_rating(self): + self.save_new_valid_exploration(self.EXP_ID, self.owner_id) + exp = exp_services.get_exploration_summary_by_id(self.EXP_ID) + + self.assertEqual( + exp_services.get_average_rating(exp.ratings), 0) + + rating_services.assign_rating_to_exploration( + self.owner_id, self.EXP_ID, 5) + self.assertEqual( + exp_services.get_average_rating(exp.ratings), 5) + + rating_services.assign_rating_to_exploration( + self.USER_ID_1, self.EXP_ID, 2) + + exp = exp_services.get_exploration_summary_by_id(self.EXP_ID) + self.assertEqual( + exp_services.get_average_rating(exp.ratings), 3.5) + + def test_get_lower_bound_wilson_rating_from_exp_summary(self): + self.save_new_valid_exploration(self.EXP_ID, self.owner_id) + exp = exp_services.get_exploration_summary_by_id(self.EXP_ID) + + self.assertEqual( + exp_services.get_scaled_average_rating(exp.ratings), 0) + + rating_services.assign_rating_to_exploration( + self.owner_id, self.EXP_ID, 5) + self.assertAlmostEqual( + exp_services.get_scaled_average_rating(exp.ratings), + 1.8261731658956, places=4) + + rating_services.assign_rating_to_exploration( + self.USER_ID_1, self.EXP_ID, 4) + + exp = exp_services.get_exploration_summary_by_id(self.EXP_ID) + self.assertAlmostEqual( + exp_services.get_scaled_average_rating(exp.ratings), + 2.056191454757, places=4) + + def test_get_search_rank(self): self.save_new_valid_exploration(self.EXP_ID, self.owner_id) @@ -2218,20 +2263,22 @@ def test_get_non_private_exploration_summaries(self): self.EXP_ID_2: exp_domain.ExplorationSummary( self.EXP_ID_2, 'Exploration 2 Albert title', 'A category', 'An objective', 'en', [], - feconf.get_empty_ratings(), + feconf.get_empty_ratings(), feconf.EMPTY_SCALED_AVERAGE_RATING, rights_manager.ACTIVITY_STATUS_PUBLIC, False, [self.albert_id], [], [], [self.albert_id], {self.albert_id: 1}, self.EXPECTED_VERSION_2, actual_summaries[self.EXP_ID_2].exploration_model_created_on, - actual_summaries[self.EXP_ID_2].exploration_model_last_updated + actual_summaries[self.EXP_ID_2].exploration_model_last_updated, + actual_summaries[self.EXP_ID_2].first_published_msec )} # check actual summaries equal expected summaries self.assertEqual(actual_summaries.keys(), expected_summaries.keys()) simple_props = ['id', 'title', 'category', 'objective', - 'language_code', 'tags', 'ratings', 'status', + 'language_code', 'tags', 'ratings', + 'scaled_average_rating', 'status', 'community_owned', 'owner_ids', 'editor_ids', 'viewer_ids', 'contributor_ids', 'version', @@ -2249,22 +2296,24 @@ def test_get_all_exploration_summaries(self): self.EXP_ID_1: exp_domain.ExplorationSummary( self.EXP_ID_1, 'Exploration 1 title', 'A category', 'An objective', 'en', [], - feconf.get_empty_ratings(), + feconf.get_empty_ratings(), feconf.EMPTY_SCALED_AVERAGE_RATING, rights_manager.ACTIVITY_STATUS_PRIVATE, False, [self.albert_id], [], [], [self.albert_id, self.bob_id], {self.albert_id: 1, self.bob_id: 1}, self.EXPECTED_VERSION_1, actual_summaries[self.EXP_ID_1].exploration_model_created_on, - actual_summaries[self.EXP_ID_1].exploration_model_last_updated + actual_summaries[self.EXP_ID_1].exploration_model_last_updated, + actual_summaries[self.EXP_ID_1].first_published_msec ), self.EXP_ID_2: exp_domain.ExplorationSummary( self.EXP_ID_2, 'Exploration 2 Albert title', 'A category', 'An objective', 'en', [], - feconf.get_empty_ratings(), + feconf.get_empty_ratings(), feconf.EMPTY_SCALED_AVERAGE_RATING, rights_manager.ACTIVITY_STATUS_PUBLIC, False, [self.albert_id], [], [], [self.albert_id], {self.albert_id: 1}, self.EXPECTED_VERSION_2, actual_summaries[self.EXP_ID_2].exploration_model_created_on, - actual_summaries[self.EXP_ID_2].exploration_model_last_updated + actual_summaries[self.EXP_ID_2].exploration_model_last_updated, + actual_summaries[self.EXP_ID_2].first_published_msec ) } @@ -3121,15 +3170,25 @@ def test_draft_cleared_after_change_list_applied(self): self.assertIsNone(exp_user_data.draft_change_list_last_updated) self.assertIsNone(exp_user_data.draft_change_list_exp_version) - def test_draft_version_valid_true(self): - self.assertTrue(exp_services.is_draft_version_valid( - self.EXP_ID1, self.USER_ID)) + def test_draft_version_valid_returns_true(self): + exp_user_data = user_models.ExplorationUserDataModel.get_by_id( + '%s.%s' % (self.USER_ID, self.EXP_ID1)) + self.assertTrue(exp_services.is_version_of_draft_valid( + self.EXP_ID1, exp_user_data.draft_change_list_exp_version)) - def test_draft_version_valid_false(self): - self.assertFalse(exp_services.is_draft_version_valid( - self.EXP_ID2, self.USER_ID)) + def test_draft_version_valid_returns_false(self): + exp_user_data = user_models.ExplorationUserDataModel.get_by_id( + '%s.%s' % (self.USER_ID, self.EXP_ID2)) + self.assertFalse(exp_services.is_version_of_draft_valid( + self.EXP_ID2, exp_user_data.draft_change_list_exp_version)) - def test_create_or_update_draft_older_draft_exists(self): + def test_draft_version_valid_when_no_draft_exists(self): + exp_user_data = user_models.ExplorationUserDataModel.get_by_id( + '%s.%s' % (self.USER_ID, self.EXP_ID3)) + self.assertFalse(exp_services.is_version_of_draft_valid( + self.EXP_ID3, exp_user_data.draft_change_list_exp_version)) + + def test_create_or_update_draft_when_older_draft_exists(self): exp_services.create_or_update_draft( self.EXP_ID1, self.USER_ID, self.NEW_CHANGELIST, 5, self.NEWER_DATETIME) @@ -3142,7 +3201,7 @@ def test_create_or_update_draft_older_draft_exists(self): self.NEWER_DATETIME) self.assertEqual(exp_user_data.draft_change_list_exp_version, 5) - def test_create_or_update_draft_newer_draft_exists(self): + def test_create_or_update_draft_when_newer_draft_exists(self): exp_services.create_or_update_draft( self.EXP_ID1, self.USER_ID, self.NEW_CHANGELIST, 5, self.OLDER_DATETIME) @@ -3155,7 +3214,7 @@ def test_create_or_update_draft_newer_draft_exists(self): exp_user_data.draft_change_list_last_updated, self.DATETIME) self.assertEqual(exp_user_data.draft_change_list_exp_version, 2) - def test_create_or_update_draft_draft_does_not_exist(self): + def test_create_or_update_draft_when_draft_does_not_exist(self): exp_services.create_or_update_draft( self.EXP_ID3, self.USER_ID, self.NEW_CHANGELIST, 5, self.NEWER_DATETIME) @@ -3168,7 +3227,7 @@ def test_create_or_update_draft_draft_does_not_exist(self): self.NEWER_DATETIME) self.assertEqual(exp_user_data.draft_change_list_exp_version, 5) - def test_get_exp_with_draft_applied_draft_exists(self): + def test_get_exp_with_draft_applied_when_draft_exists(self): exploration = exp_services.get_exploration_by_id(self.EXP_ID1) self.assertEqual(exploration.init_state.param_changes, []) updated_exp = exp_services.get_exp_with_draft_applied( @@ -3180,9 +3239,24 @@ def test_get_exp_with_draft_applied_draft_exists(self): param_changes._customization_args, {'list_of_values': ['1', '2'], 'parse_with_jinja': False}) - def test_get_exp_with_draft_applied_draft_does_not_exist(self): + def test_get_exp_with_draft_applied_when_draft_does_not_exist(self): exploration = exp_services.get_exploration_by_id(self.EXP_ID3) self.assertEqual(exploration.init_state.param_changes, []) updated_exp = exp_services.get_exp_with_draft_applied( self.EXP_ID3, self.USER_ID) self.assertEqual(updated_exp.init_state.param_changes, []) + + def test_get_exp_with_draft_applied_when_draft_version_is_invalid(self): + exploration = exp_services.get_exploration_by_id(self.EXP_ID2) + self.assertEqual(exploration.init_state.param_changes, []) + updated_exp = exp_services.get_exp_with_draft_applied( + self.EXP_ID2, self.USER_ID) + self.assertEqual(updated_exp.init_state.param_changes, []) + + def test_draft_discarded(self): + exp_services.discard_draft(self.EXP_ID1, self.USER_ID,) + exp_user_data = user_models.ExplorationUserDataModel.get_by_id( + '%s.%s' % (self.USER_ID, self.EXP_ID1)) + self.assertIsNone(exp_user_data.draft_change_list) + self.assertIsNone(exp_user_data.draft_change_list_last_updated) + self.assertIsNone(exp_user_data.draft_change_list_exp_version) diff --git a/core/domain/feedback_services.py b/core/domain/feedback_services.py index 32261f881418..c689cc5656b5 100644 --- a/core/domain/feedback_services.py +++ b/core/domain/feedback_services.py @@ -23,6 +23,7 @@ import feconf (feedback_models,) = models.Registry.import_models([models.NAMES.feedback]) +taskqueue_services = models.Registry.import_taskqueue_services() DEFAULT_SUGGESTION_THREAD_SUBJECT = 'Suggestion from a learner' DEFAULT_SUGGESTION_THREAD_INITIAL_MESSAGE = '' @@ -245,3 +246,11 @@ def get_all_threads(exploration_id, has_suggestion): if thread.has_suggestion == has_suggestion: all_threads.append(thread) return all_threads + + +def enqueue_feedback_message_email_task(user_id): + """Adds a 'send feedback email' task into the taskqueue.""" + + taskqueue_services.enqueue_task( + feconf.FEEDBACK_MESSAGE_EMAIL_HANDLER_URL, {'user_id': user_id}, + feconf.DEFAULT_FEEDBACK_MESSAGE_EMAIL_COUNTDOWN_SECS) diff --git a/core/domain/feedback_services_test.py b/core/domain/feedback_services_test.py index ad9c5d0ae137..5250dad4066f 100644 --- a/core/domain/feedback_services_test.py +++ b/core/domain/feedback_services_test.py @@ -18,10 +18,10 @@ from core.domain import user_services from core.platform import models from core.tests import test_utils +import feconf (feedback_models,) = models.Registry.import_models([models.NAMES.feedback]) - class FeedbackServicesUnitTests(test_utils.GenericTestBase): """Test functions in feedback_services.""" @@ -244,3 +244,18 @@ def test_get_all_threads(self): self.assertEqual(2, len(threads)) self.assertDictContainsSubset(expected_thread_dict, threads[1].to_dict()) + + +class EmailsTaskqueueTests(test_utils.GenericTestBase): + """Tests for tasks in emails taskqueue.""" + + def test_create_new_task(self): + user_id = 'user' + feedback_services.enqueue_feedback_message_email_task(user_id) + self.assertEqual( + self.count_jobs_in_taskqueue(), 1) + + tasks = self.get_pending_tasks() + self.assertEqual( + tasks[0].url, feconf.FEEDBACK_MESSAGE_EMAIL_HANDLER_URL) + self.process_and_flush_pending_tasks() diff --git a/core/domain/rating_services.py b/core/domain/rating_services.py index 7a6b4703eb9a..f9fb597faead 100644 --- a/core/domain/rating_services.py +++ b/core/domain/rating_services.py @@ -71,6 +71,11 @@ def _update_user_rating(): exploration_summary.ratings[str(new_rating)] += 1 if old_rating: exploration_summary.ratings[str(old_rating)] -= 1 + + exploration_summary.scaled_average_rating = ( + exp_services.get_scaled_average_rating( + exploration_summary.ratings)) + exp_services.save_exploration_summary(exploration_summary) diff --git a/core/domain/rating_services_test.py b/core/domain/rating_services_test.py index 0ee66f1ae33c..26fe4e6bd080 100644 --- a/core/domain/rating_services_test.py +++ b/core/domain/rating_services_test.py @@ -36,13 +36,16 @@ def test_rating_assignation(self): exp_services.save_new_exploration( self.EXP_ID, - exp_domain.Exploration.create_default_exploration( - self.EXP_ID, 'A title', 'A category')) + exp_domain.Exploration.create_default_exploration(self.EXP_ID)) self.assertEqual( rating_services.get_overall_ratings_for_exploration(self.EXP_ID), {'1': 0, '2': 0, '3': 0, '4': 0, '5': 0}) + exp_summary = exp_services.get_exploration_summary_by_id(self.EXP_ID) + self.assertEqual( + exp_summary.scaled_average_rating, 0) + self.assertEqual( rating_services.get_user_specific_rating_for_exploration( self.USER_ID_1, self.EXP_ID), None) @@ -54,6 +57,10 @@ def test_rating_assignation(self): rating_services.assign_rating_to_exploration( self.USER_ID_1, self.EXP_ID, 3) + exp_summary = exp_services.get_exploration_summary_by_id(self.EXP_ID) + self.assertAlmostEqual( + exp_summary.scaled_average_rating, 1.5667471839848, places=4) + self.assertEqual( rating_services.get_user_specific_rating_for_exploration( self.USER_ID_1, self.EXP_ID), 3) @@ -78,8 +85,7 @@ def test_time_of_ratings_recorded(self): exp_services.save_new_exploration( self.EXP_ID, - exp_domain.Exploration.create_default_exploration( - self.EXP_ID, 'A title', 'A category')) + exp_domain.Exploration.create_default_exploration(self.EXP_ID)) rating_services.assign_rating_to_exploration( self.USER_ID_1, self.EXP_ID, 1) @@ -104,12 +110,10 @@ def test_rating_assignations_do_not_conflict(self): exp_services.save_new_exploration( exp_id_a, - exp_domain.Exploration.create_default_exploration( - exp_id_a, 'A title', 'A category')) + exp_domain.Exploration.create_default_exploration(exp_id_a)) exp_services.save_new_exploration( exp_id_b, - exp_domain.Exploration.create_default_exploration( - exp_id_b, 'A title', 'A category')) + exp_domain.Exploration.create_default_exploration(exp_id_b)) rating_services.assign_rating_to_exploration( self.USER_ID_1, exp_id_a, 1) diff --git a/core/domain/rights_manager.py b/core/domain/rights_manager.py index 9cfe071efa70..4c70e7a85ca9 100644 --- a/core/domain/rights_manager.py +++ b/core/domain/rights_manager.py @@ -625,18 +625,15 @@ def _change_activity_status( if new_status != ACTIVITY_STATUS_PRIVATE: activity_rights.viewer_ids = [] + if activity_rights.first_published_msec is None: + activity_rights.first_published_msec = ( + utils.get_current_time_in_millisecs()) _save_activity_rights( committer_id, activity_rights, activity_type, commit_message, commit_cmds) _update_activity_summary(activity_type, activity_rights) - if new_status != ACTIVITY_STATUS_PRIVATE: - # Change first_published_msec in activity rights if necessary. - if activity_rights.first_published_msec is None: - activity_rights.first_published_msec = ( - utils.get_current_time_in_millisecs()) - def _publish_activity(committer_id, activity_id, activity_type): if not Actor(committer_id).can_publish(activity_type, activity_id): diff --git a/core/domain/rights_manager_test.py b/core/domain/rights_manager_test.py index 91a2d1d9cab1..47d0180be436 100644 --- a/core/domain/rights_manager_test.py +++ b/core/domain/rights_manager_test.py @@ -123,8 +123,7 @@ def test_non_splash_page_demo_exploration(self): rights_manager.ACTIVITY_TYPE_EXPLORATION, '3')) def test_ownership_of_exploration(self): - exp = exp_domain.Exploration.create_default_exploration( - self.EXP_ID, 'A title', 'A category') + exp = exp_domain.Exploration.create_default_exploration(self.EXP_ID) exp_services.save_new_exploration(self.user_id_a, exp) rights_manager.assign_role_for_exploration( @@ -143,8 +142,7 @@ def test_ownership_of_exploration(self): rights_manager.ACTIVITY_TYPE_EXPLORATION, self.EXP_ID)) def test_newly_created_exploration(self): - exp = exp_domain.Exploration.create_default_exploration( - self.EXP_ID, 'A title', 'A category') + exp = exp_domain.Exploration.create_default_exploration(self.EXP_ID) exp_services.save_new_exploration(self.user_id_a, exp) self.assertTrue( @@ -187,8 +185,7 @@ def test_newly_created_exploration(self): rights_manager.ACTIVITY_TYPE_EXPLORATION, self.EXP_ID)) def test_inviting_collaborator_to_exploration(self): - exp = exp_domain.Exploration.create_default_exploration( - self.EXP_ID, 'A title', 'A category') + exp = exp_domain.Exploration.create_default_exploration(self.EXP_ID) exp_services.save_new_exploration(self.user_id_a, exp) self.assertFalse( @@ -222,8 +219,7 @@ def test_inviting_collaborator_to_exploration(self): rights_manager.ACTIVITY_TYPE_EXPLORATION, self.EXP_ID)) def test_inviting_playtester_to_exploration(self): - exp = exp_domain.Exploration.create_default_exploration( - self.EXP_ID, 'A title', 'A category') + exp = exp_domain.Exploration.create_default_exploration(self.EXP_ID) exp_services.save_new_exploration(self.user_id_a, exp) self.assertFalse( @@ -257,8 +253,7 @@ def test_inviting_playtester_to_exploration(self): rights_manager.ACTIVITY_TYPE_EXPLORATION, self.EXP_ID)) def test_setting_rights_of_exploration(self): - exp = exp_domain.Exploration.create_default_exploration( - self.EXP_ID, 'A title', 'A category') + exp = exp_domain.Exploration.create_default_exploration(self.EXP_ID) exp_services.save_new_exploration(self.user_id_a, exp) rights_manager.assign_role_for_exploration( @@ -295,7 +290,7 @@ def test_setting_rights_of_exploration(self): def test_publishing_and_unpublishing_exploration(self): exp = exp_domain.Exploration.create_default_exploration( - self.EXP_ID, 'A title', 'A category') + self.EXP_ID, title='A title', category='A category') exp_services.save_new_exploration(self.user_id_a, exp) self.assertFalse( @@ -334,7 +329,7 @@ def test_publishing_and_unpublishing_exploration(self): def test_can_only_delete_unpublished_explorations(self): exp = exp_domain.Exploration.create_default_exploration( - self.EXP_ID, 'A title', 'A category') + self.EXP_ID, title='A title', category='A category') exp_services.save_new_exploration(self.user_id_a, exp) self.assertTrue( @@ -355,7 +350,7 @@ def test_can_only_delete_unpublished_explorations(self): def test_can_publicize_exploration(self): exp = exp_domain.Exploration.create_default_exploration( - self.EXP_ID, 'A title', 'A category') + self.EXP_ID, title='A title', category='A category') exp_services.save_new_exploration(self.user_id_a, exp) rights_manager.publish_exploration(self.user_id_a, self.EXP_ID) @@ -369,7 +364,7 @@ def test_can_publicize_exploration(self): def test_changing_viewability_of_exploration(self): exp = exp_domain.Exploration.create_default_exploration( - self.EXP_ID, 'A title', 'A category') + self.EXP_ID, title='A title', category='A category') exp_services.save_new_exploration(self.user_id_a, exp) self.assertFalse( diff --git a/core/domain/stats_domain_test.py b/core/domain/stats_domain_test.py index ceb3243897fd..3d7d7697e85e 100644 --- a/core/domain/stats_domain_test.py +++ b/core/domain/stats_domain_test.py @@ -27,8 +27,7 @@ class StateRuleAnswerLogUnitTests(test_utils.GenericTestBase): DEFAULT_RULESPEC_STR = exp_domain.DEFAULT_RULESPEC_STR def test_state_rule_answer_logs(self): - exp = exp_domain.Exploration.create_default_exploration( - 'eid', 'title', 'category') + exp = exp_domain.Exploration.create_default_exploration('eid') exp_services.save_new_exploration('user_id', exp) state_name = exp.init_state_name @@ -78,8 +77,7 @@ def test_state_rule_answer_logs(self): answer_log.get_top_answers(2), [('answer2', 3), ('answer1', 2)]) def test_recording_answer_for_different_rules(self): - exp = exp_domain.Exploration.create_default_exploration( - 'eid', 'title', 'category') + exp = exp_domain.Exploration.create_default_exploration('eid') exp_services.save_new_exploration('user_id', exp) rule = exp_domain.RuleSpec.from_dict({ @@ -106,8 +104,7 @@ def test_recording_answer_for_different_rules(self): self.assertEquals(other_rule_answer_log.total_answer_count, 1) def test_resolving_answers(self): - exp = exp_domain.Exploration.create_default_exploration( - 'eid', 'title', 'category') + exp = exp_domain.Exploration.create_default_exploration('eid') exp_services.save_new_exploration('user_id', exp) state_name = exp.init_state_name diff --git a/core/domain/stats_services_test.py b/core/domain/stats_services_test.py index 6ec24166063b..8f7ee321ddff 100644 --- a/core/domain/stats_services_test.py +++ b/core/domain/stats_services_test.py @@ -127,8 +127,7 @@ def _get_swap_context(self): ModifiedStatisticsAggregator.get_statistics) def test_get_state_improvements(self): - exp = exp_domain.Exploration.create_default_exploration( - 'eid', 'A title', 'A category') + exp = exp_domain.Exploration.create_default_exploration('eid') exp_services.save_new_exploration('fake@user.com', exp) for ind in range(5): @@ -154,8 +153,7 @@ def test_get_state_improvements(self): }]) def test_single_default_rule_hit(self): - exp = exp_domain.Exploration.create_default_exploration( - 'eid', 'A title', 'A category') + exp = exp_domain.Exploration.create_default_exploration('eid') exp_services.save_new_exploration('fake@user.com', exp) state_name = exp.init_state_name @@ -199,8 +197,7 @@ def test_no_improvement_flag_hit(self): self.assertEquals(stats_services.get_state_improvements('eid', 1), []) def test_incomplete_and_default_flags(self): - exp = exp_domain.Exploration.create_default_exploration( - 'eid', 'A title', 'A category') + exp = exp_domain.Exploration.create_default_exploration('eid') exp_services.save_new_exploration('fake@user.com', exp) state_name = exp.init_state_name diff --git a/core/domain/subscription_services_test.py b/core/domain/subscription_services_test.py index fe75e7471e1f..7ef7055d7b30 100644 --- a/core/domain/subscription_services_test.py +++ b/core/domain/subscription_services_test.py @@ -171,15 +171,12 @@ def test_creating_exploration_results_in_subscription(self): self.assertEqual( self._get_exploration_ids_subscribed_to(USER_ID), []) exp_services.save_new_exploration( - USER_ID, - exp_domain.Exploration.create_default_exploration( - EXP_ID, 'Title', 'Category')) + USER_ID, exp_domain.Exploration.create_default_exploration(EXP_ID)) self.assertEqual( self._get_exploration_ids_subscribed_to(USER_ID), [EXP_ID]) def test_adding_new_exploration_owner_or_editor_role_results_in_subscription(self): # pylint: disable=line-too-long - exploration = exp_domain.Exploration.create_default_exploration( - EXP_ID, 'Title', 'Category') + exploration = exp_domain.Exploration.create_default_exploration(EXP_ID) exp_services.save_new_exploration(self.owner_id, exploration) self.assertEqual( @@ -197,8 +194,7 @@ def test_adding_new_exploration_owner_or_editor_role_results_in_subscription(sel self._get_exploration_ids_subscribed_to(self.editor_id), [EXP_ID]) def test_adding_new_exploration_viewer_role_does_not_result_in_subscription(self): # pylint: disable=line-too-long - exploration = exp_domain.Exploration.create_default_exploration( - EXP_ID, 'Title', 'Category') + exploration = exp_domain.Exploration.create_default_exploration(EXP_ID) exp_services.save_new_exploration(self.owner_id, exploration) self.assertEqual( @@ -209,8 +205,7 @@ def test_adding_new_exploration_viewer_role_does_not_result_in_subscription(self self._get_exploration_ids_subscribed_to(self.viewer_id), []) def test_deleting_exploration_does_not_delete_subscription(self): - exploration = exp_domain.Exploration.create_default_exploration( - EXP_ID, 'Title', 'Category') + exploration = exp_domain.Exploration.create_default_exploration(EXP_ID) exp_services.save_new_exploration(self.owner_id, exploration) self.assertEqual( self._get_exploration_ids_subscribed_to(self.owner_id), [EXP_ID]) diff --git a/core/domain/summary_services.py b/core/domain/summary_services.py index bdcf86b18e9a..201cab69d413 100644 --- a/core/domain/summary_services.py +++ b/core/domain/summary_services.py @@ -16,10 +16,12 @@ """Commands that can be used to operate on exploration summaries.""" +from core.domain import collection_services from core.domain import exp_services from core.domain import rights_manager from core.domain import stats_jobs_continuous from core.domain import user_services +import feconf import utils _LIBRARY_INDEX_GROUPS = [{ @@ -45,11 +47,14 @@ ], }, { 'header': 'Languages', - 'search_categories': ['Languages', 'Reading', 'English', 'Latin'], + 'search_categories': [ + 'Languages', 'Reading', 'English', 'Latin', 'Spanish', 'Gaulish' + ], }, { 'header': 'Social Science', 'search_categories': [ - 'Business', 'Economics', 'Geography', 'Government', 'History', 'Law'], + 'Business', 'Economics', 'Geography', 'Government', 'History', 'Law' + ], }] @@ -69,6 +74,93 @@ def get_human_readable_contributors_summary(contributors_summary): } +def get_learner_collection_dict_by_id( + collection_id, user_id, strict=True, allow_invalid_explorations=False, + version=None): + """Creates and returns a dictionary representation of a collection given by + the provided collection ID. This dictionary contains extra information + along with the dict returned by collection_domain.Collection.to_dict() + which includes useful data for the collection learner view. The information + includes progress in the collection, information about explorations + referenced within the collection, and a slightly nicer data structure for + frontend work. + This raises a ValidationError if the collection retrieved using the given ID + references non-existent explorations. + which includes useful data for the collection learner view. + """ + collection = collection_services.get_collection_by_id( + collection_id, strict=strict, version=version) + + exp_ids = collection.exploration_ids + exp_summary_dicts = get_displayable_exp_summary_dicts_matching_ids( + exp_ids, editor_user_id=user_id) + exp_summaries_dict_map = { + exp_summary_dict['id']: exp_summary_dict + for exp_summary_dict in exp_summary_dicts + } + + # TODO(bhenning): Users should not be recommended explorations they have + # completed outside the context of a collection (see #1461). + next_exploration_ids = None + completed_exp_ids = None + if user_id: + completed_exp_ids = collection_services.get_valid_completed_exploration_ids( # pylint: disable=line-too-long + user_id, collection_id, collection) + next_exploration_ids = collection.get_next_exploration_ids( + completed_exp_ids) + else: + # If the user is not logged in or they have not completed any of + # the explorations yet within the context of this collection, + # recommend the initial explorations. + next_exploration_ids = collection.init_exploration_ids + completed_exp_ids = [] + + collection_dict = collection.to_dict() + collection_dict['skills'] = collection.skills + collection_dict['playthrough_dict'] = { + 'next_exploration_ids': next_exploration_ids, + 'completed_exploration_ids': completed_exp_ids + } + collection_dict['version'] = collection.version + collection_is_public = rights_manager.is_collection_public(collection_id) + + # Insert an 'exploration' dict into each collection node, where the + # dict includes meta information about the exploration (ID and title). + for collection_node in collection_dict['nodes']: + exploration_id = collection_node['exploration_id'] + summary_dict = exp_summaries_dict_map.get(exploration_id) + if not allow_invalid_explorations: + if not summary_dict: + raise utils.ValidationError( + 'Expected collection to only reference valid ' + 'explorations, but found an exploration with ID: %s (was ' + 'the exploration deleted or is it a private exploration ' + 'that you do not have edit access to?)' + % exploration_id) + if collection_is_public and rights_manager.is_exploration_private( + exploration_id): + raise utils.ValidationError( + 'Cannot reference a private exploration within a public ' + 'collection, exploration ID: %s' % exploration_id) + + if summary_dict: + collection_node['exploration_summary'] = summary_dict + else: + collection_node['exploration_summary'] = None + + return collection_dict + + +def get_displayable_collection_summary_dicts_matching_ids(collection_ids): + """Returns a list with all collection summary objects that can be + displayed on the library page as collection summary tiles. + """ + collection_summaries = ( + collection_services.get_collection_summaries_matching_ids( + collection_ids)) + return _get_displayable_collection_summary_dicts(collection_summaries) + + def get_displayable_exp_summary_dicts_matching_ids( exploration_ids, editor_user_id=None): """Given a list of exploration ids, optionally filters the list for @@ -97,13 +189,16 @@ def get_displayable_exp_summary_dicts_matching_ids( filtered_exploration_summaries.append(exploration_summary) - return _get_displayable_exp_summary_dicts(filtered_exploration_summaries) + return _get_displayable_exp_summary_dicts( + filtered_exploration_summaries, include_contributors=False) -def _get_displayable_exp_summary_dicts(exploration_summaries): +def _get_displayable_exp_summary_dicts( + exploration_summaries, include_contributors=True): """Given a list of exploration summary domain objects, returns a list, with the same number of elements, of the corresponding human-readable - exploration summary dicts. + exploration summary dicts. If include_contributors is False, the resulting + dicts will not have a 'human_readable_contributors_summary' attribute. This assumes that all the exploration summary domain objects passed in are valid (i.e., none of them are None). @@ -121,9 +216,10 @@ def _get_displayable_exp_summary_dicts(exploration_summaries): if not exploration_summary: continue - displayable_exp_summaries.append({ + summary_dict = { 'id': exploration_summary.id, 'title': exploration_summary.title, + 'activity_type': rights_manager.ACTIVITY_TYPE_EXPLORATION, 'category': exploration_summary.category, 'objective': exploration_summary.objective, 'language_code': exploration_summary.language_code, @@ -133,20 +229,47 @@ def _get_displayable_exp_summary_dicts(exploration_summaries): 'status': exploration_summary.status, 'ratings': exploration_summary.ratings, 'community_owned': exploration_summary.community_owned, - 'human_readable_contributors_summary': - get_human_readable_contributors_summary( - exploration_summary.contributors_summary), 'tags': exploration_summary.tags, 'thumbnail_icon_url': utils.get_thumbnail_icon_url_for_category( exploration_summary.category), 'thumbnail_bg_color': utils.get_hex_color_for_category( exploration_summary.category), 'num_views': view_counts[ind], - }) + } + + if include_contributors: + summary_dict['human_readable_contributors_summary'] = ( + get_human_readable_contributors_summary( + exploration_summary.contributors_summary)) + + displayable_exp_summaries.append(summary_dict) return displayable_exp_summaries +def _get_displayable_collection_summary_dicts(collection_summaries): + displayable_collection_summaries = [] + for collection_summary in collection_summaries: + if collection_summary and collection_summary.status != ( + rights_manager.ACTIVITY_STATUS_PRIVATE): + displayable_collection_summaries.append({ + 'id': collection_summary.id, + 'title': collection_summary.title, + 'category': collection_summary.category, + 'activity_type': rights_manager.ACTIVITY_TYPE_COLLECTION, + 'objective': collection_summary.objective, + 'language_code': collection_summary.language_code, + 'tags': collection_summary.tags, + 'node_count': collection_summary.node_count, + 'last_updated_msec': utils.get_time_in_millisecs( + collection_summary.collection_model_last_updated), + 'thumbnail_icon_url': utils.get_thumbnail_icon_url_for_category( + collection_summary.category), + 'thumbnail_bg_color': utils.get_hex_color_for_category( + collection_summary.category)}) + return displayable_collection_summaries + + def get_library_groups(language_codes): """Returns a list of groups for the library index page. Each group has a header and a list of dicts representing activity summaries. @@ -161,6 +284,28 @@ def _generate_query(categories): return 'category=("%s")%s' % ( '" OR "'.join(categories), language_codes_suffix) + # Collect all collection ids so that the summary details can be retrieved + # with a single get_multi() call. + all_collection_ids = [] + header_to_collection_ids = {} + for group in _LIBRARY_INDEX_GROUPS: + collection_ids = collection_services.search_collections( + _generate_query(group['search_categories']), 8)[0] + header_to_collection_ids[group['header']] = collection_ids + all_collection_ids += collection_ids + + collection_summaries = [ + summary for summary in + collection_services.get_collection_summaries_matching_ids( + all_collection_ids) + if summary is not None] + collection_summary_dicts = { + summary_dict['id']: summary_dict + for summary_dict in _get_displayable_collection_summary_dicts( + collection_summaries) + } + + # Collect all exp ids so that the summary details can be retrieved with a # single get_multi() call. all_exp_ids = [] @@ -171,20 +316,29 @@ def _generate_query(categories): header_to_exp_ids[group['header']] = exp_ids all_exp_ids += exp_ids - all_exploration_summaries = ( - exp_services.get_exploration_summaries_matching_ids(all_exp_ids)) - all_summary_dicts = { + exp_summaries = [ + summary for summary in + exp_services.get_exploration_summaries_matching_ids(all_exp_ids) + if summary is not None] + + exp_summary_dicts = { summary_dict['id']: summary_dict for summary_dict in _get_displayable_exp_summary_dicts( - all_exploration_summaries) + exp_summaries, include_contributors=False) } results = [] for group in _LIBRARY_INDEX_GROUPS: - exp_ids_to_display = header_to_exp_ids[group['header']] + summary_dicts = [] + collection_ids_to_display = header_to_collection_ids[group['header']] summary_dicts = [ - all_summary_dicts[exp_id] for exp_id in exp_ids_to_display - if exp_id in all_summary_dicts] + collection_summary_dicts[collection_id] for collection_id in collection_ids_to_display # pylint: disable=line-too-long + if collection_id in collection_summary_dicts] + + exp_ids_to_display = header_to_exp_ids[group['header']] + summary_dicts += [ + exp_summary_dicts[exp_id] for exp_id in exp_ids_to_display + if exp_id in exp_summary_dicts] if not summary_dicts: continue @@ -219,4 +373,43 @@ def get_featured_exploration_summary_dicts(language_codes): key=lambda exp_summary: search_ranks[exp_summary.id], reverse=True) - return _get_displayable_exp_summary_dicts(sorted_exp_summaries) + return _get_displayable_exp_summary_dicts( + sorted_exp_summaries, include_contributors=False) + + +def get_top_rated_exploration_summary_dicts(language_codes): + """Returns a list of top rated explorations with the given language code. + + The return value is sorted in decreasing order of average rating. + """ + filtered_exp_summaries = [ + exp_summary for exp_summary in + exp_services.get_top_rated_exploration_summaries().values() + if exp_summary.language_code in language_codes and + sum(exp_summary.ratings.values()) > 0] + + sorted_exp_summaries = sorted( + filtered_exp_summaries, + key=lambda exp_summary: exp_summary.scaled_average_rating, + reverse=True)[:feconf.NUMBER_OF_TOP_RATED_EXPLORATIONS] + + return _get_displayable_exp_summary_dicts( + sorted_exp_summaries, include_contributors=False) + + +def get_recently_published_exploration_summary_dicts(): + """Returns a list of recently published explorations + with the given language code. + """ + recently_published_exploration_summaries = [ + exp_summary for exp_summary in + exp_services.get_recently_published_exploration_summaries().values()] + + # Arranging recently published exploration summaries with respect to time. + # sorted() is used to sort the random list of recently published summaries. + summaries = sorted( + recently_published_exploration_summaries, + key=lambda exp_summary: exp_summary.first_published_msec, + reverse=True) + + return _get_displayable_exp_summary_dicts(summaries) diff --git a/core/domain/summary_services_test.py b/core/domain/summary_services_test.py index 957d4602ec3e..6f6b68439d9d 100644 --- a/core/domain/summary_services_test.py +++ b/core/domain/summary_services_test.py @@ -14,13 +14,17 @@ # See the License for the specific language governing permissions and # limitations under the License. +from core.domain import collection_domain +from core.domain import collection_services from core.domain import exp_services from core.domain import exp_services_test +from core.domain import rating_services from core.domain import rights_manager from core.domain import summary_services from core.domain import user_services from core.tests import test_utils import feconf +import utils class ExplorationDisplayableSummariesTest( @@ -182,13 +186,6 @@ def test_get_displayable_exp_summary_dicts_matching_ids(self): expected_summary = { 'category': u'A category', 'community_owned': False, - 'human_readable_contributors_summary': { - self.ALBERT_NAME: { - 'num_commits': 2, - 'profile_picture_data_url': ( - user_services.DEFAULT_IDENTICON_DATA_URL) - } - }, 'id': self.EXP_ID_2, 'language_code': feconf.DEFAULT_LANGUAGE_CODE, 'num_views': 0, @@ -270,7 +267,6 @@ def test_get_library_groups(self): expected_exploration_summary_dict = { 'category': u'Algorithms', 'community_owned': True, - 'human_readable_contributors_summary': {}, 'id': '2', 'language_code': feconf.DEFAULT_LANGUAGE_CODE, 'num_views': 0, @@ -362,3 +358,409 @@ def test_for_featured_explorations(self): } self.assertDictContainsSubset( expected_summary, featured_exploration_summaries[0]) + + +class CollectionLearnerDictTests(test_utils.GenericTestBase): + """Test get_learner_collection_dict_by_id.""" + + EXP_ID = 'exploration_id' + EXP_ID_1 = 'exp_id1' + COLLECTION_ID = 'A_collection_id' + + def setUp(self): + super(CollectionLearnerDictTests, self).setUp() + + self.owner_id = self.get_user_id_from_email(self.OWNER_EMAIL) + self.editor_id = self.get_user_id_from_email(self.EDITOR_EMAIL) + + user_services.get_or_create_user(self.owner_id, self.OWNER_EMAIL) + user_services.get_or_create_user(self.editor_id, self.EDITOR_EMAIL) + + self.signup(self.OWNER_EMAIL, self.OWNER_USERNAME) + self.signup(self.EDITOR_EMAIL, self.EDITOR_USERNAME) + + def test_get_learner_dict_with_deleted_exp_fails_validation(self): + self.save_new_valid_collection( + self.COLLECTION_ID, self.owner_id, exploration_id=self.EXP_ID) + summary_services.get_learner_collection_dict_by_id( + self.COLLECTION_ID, self.owner_id) + + exp_services.delete_exploration(self.owner_id, self.EXP_ID) + + with self.assertRaisesRegexp( + utils.ValidationError, + 'Expected collection to only reference valid explorations, but ' + 'found an exploration with ID: exploration_id'): + summary_services.get_learner_collection_dict_by_id( + self.COLLECTION_ID, self.owner_id) + + def test_get_learner_dict_when_referencing_inaccessible_explorations(self): + self.save_new_default_collection(self.COLLECTION_ID, self.owner_id) + self.save_new_valid_exploration(self.EXP_ID, self.editor_id) + collection_services.update_collection( + self.owner_id, self.COLLECTION_ID, [{ + 'cmd': collection_domain.CMD_ADD_COLLECTION_NODE, + 'exploration_id': self.EXP_ID + }], 'Added another creator\'s private exploration') + + # A collection cannot access someone else's private exploration. + rights_manager.publish_collection(self.owner_id, self.COLLECTION_ID) + with self.assertRaisesRegexp( + utils.ValidationError, + 'Expected collection to only reference valid explorations, but ' + 'found an exploration with ID: exploration_id'): + summary_services.get_learner_collection_dict_by_id( + self.COLLECTION_ID, self.owner_id) + + # After the exploration is published, the dict can now be created. + rights_manager.publish_exploration(self.editor_id, self.EXP_ID) + summary_services.get_learner_collection_dict_by_id( + self.COLLECTION_ID, self.owner_id) + + def test_get_learner_dict_with_private_exp_fails_validation(self): + self.save_new_valid_collection( + self.COLLECTION_ID, self.owner_id, exploration_id=self.EXP_ID) + + # Since both the collection and exploration are private, the learner + # dict can be created. + summary_services.get_learner_collection_dict_by_id( + self.COLLECTION_ID, self.owner_id) + + # A public collection referencing a private exploration is bad, however. + rights_manager.publish_collection(self.owner_id, self.COLLECTION_ID) + with self.assertRaisesRegexp( + utils.ValidationError, + 'Cannot reference a private exploration within a public ' + 'collection, exploration ID: exploration_id'): + summary_services.get_learner_collection_dict_by_id( + self.COLLECTION_ID, self.owner_id) + + # After the exploration is published, the learner dict can be crated + # again. + rights_manager.publish_exploration(self.owner_id, self.EXP_ID) + summary_services.get_learner_collection_dict_by_id( + self.COLLECTION_ID, self.owner_id) + + def test_get_learner_dict_with_allowed_private_exps(self): + self.save_new_valid_collection( + self.COLLECTION_ID, self.owner_id, exploration_id=self.EXP_ID) + self.save_new_valid_exploration(self.EXP_ID_1, self.editor_id) + collection_services.update_collection( + self.owner_id, self.COLLECTION_ID, [{ + 'cmd': collection_domain.CMD_ADD_COLLECTION_NODE, + 'exploration_id': self.EXP_ID_1 + }], 'Added another creator\'s private exploration') + + rights_manager.publish_collection(self.owner_id, self.COLLECTION_ID) + + collection_dict = summary_services.get_learner_collection_dict_by_id( + self.COLLECTION_ID, self.owner_id, allow_invalid_explorations=True) + + # The author's private exploration will be contained in the public + # collection since invalid explorations are being allowed, but the + # private exploration of another author will not. + collection_node_dicts = collection_dict['nodes'] + self.assertEqual( + collection_node_dicts[0]['exploration_summary']['id'], + self.EXP_ID) + self.assertIsNone(collection_node_dicts[1]['exploration_summary']) + + +class TopRatedExplorationDisplayableSummariesTest( + test_utils.GenericTestBase): + """Test functions for getting displayable top rated exploration + summary dicts. + """ + + ALBERT_EMAIL = 'albert@example.com' + ALICE_EMAIL = 'alice@example.com' + BOB_EMAIL = 'bob@example.com' + ALBERT_NAME = 'albert' + ALICE_NAME = 'alice' + BOB_NAME = 'bob' + + EXP_ID_1 = 'eid1' + EXP_ID_2 = 'eid2' + EXP_ID_3 = 'eid3' + EXP_ID_4 = 'eid4' + EXP_ID_5 = 'eid5' + EXP_ID_6 = 'eid6' + EXP_ID_7 = 'eid7' + EXP_ID_8 = 'eid8' + EXP_ID_9 = 'eid9' + + def setUp(self): + """Populate the database of explorations and their summaries. + + The sequence of events is: + - (1) Albert creates EXP_ID_1. + - (2) Albert creates EXP_ID_2. + - (3) Albert creates EXP_ID_3. + - (4) Albert creates EXP_ID_4. + - (5) Albert creates EXP_ID_5. + - (6) Albert creates EXP_ID_6. + - (7) Albert creates EXP_ID_7. + - (8) Albert creates EXP_ID_8. + - (9) Albert creates EXP_ID_9. + - (10) Albert publishes EXP_ID_1. + - (11) Albert publishes EXP_ID_2. + - (12) Albert publishes EXP_ID_3. + - (13) Albert publishes EXP_ID_4. + - (14) Albert publishes EXP_ID_5. + - (15) Albert publishes EXP_ID_6. + - (16) Albert publishes EXP_ID_7. + - (17) Albert publishes EXP_ID_8. + - (18) Albert publishes EXP_ID_9. + - (19) Admin user is set up. + """ + + super(TopRatedExplorationDisplayableSummariesTest, self).setUp() + + self.admin_id = self.get_user_id_from_email(self.ADMIN_EMAIL) + self.albert_id = self.get_user_id_from_email(self.ALBERT_EMAIL) + self.alice_id = self.get_user_id_from_email(self.ALICE_EMAIL) + self.bob_id = self.get_user_id_from_email(self.BOB_EMAIL) + + self.signup(self.ADMIN_EMAIL, self.ADMIN_USERNAME) + self.signup(self.ALBERT_EMAIL, self.ALBERT_NAME) + self.signup(self.ALICE_EMAIL, self.ALICE_NAME) + self.signup(self.BOB_EMAIL, self.BOB_NAME) + + self.save_new_valid_exploration(self.EXP_ID_1, self.albert_id) + self.save_new_valid_exploration(self.EXP_ID_2, self.albert_id) + self.save_new_valid_exploration(self.EXP_ID_3, self.albert_id) + self.save_new_valid_exploration(self.EXP_ID_4, self.albert_id) + self.save_new_valid_exploration(self.EXP_ID_5, self.albert_id) + self.save_new_valid_exploration(self.EXP_ID_6, self.albert_id) + self.save_new_valid_exploration(self.EXP_ID_7, self.albert_id) + self.save_new_valid_exploration(self.EXP_ID_8, self.albert_id) + self.save_new_valid_exploration(self.EXP_ID_9, self.albert_id) + + rights_manager.publish_exploration(self.albert_id, self.EXP_ID_1) + rights_manager.publish_exploration(self.albert_id, self.EXP_ID_2) + rights_manager.publish_exploration(self.albert_id, self.EXP_ID_3) + rights_manager.publish_exploration(self.albert_id, self.EXP_ID_4) + rights_manager.publish_exploration(self.albert_id, self.EXP_ID_5) + rights_manager.publish_exploration(self.albert_id, self.EXP_ID_6) + rights_manager.publish_exploration(self.albert_id, self.EXP_ID_7) + rights_manager.publish_exploration(self.albert_id, self.EXP_ID_8) + rights_manager.publish_exploration(self.albert_id, self.EXP_ID_9) + + self.set_admins([self.ADMIN_USERNAME]) + + def test_at_most_eight_top_rated_explorations(self): + """Note that at most 8 explorations should be returned. + """ + rating_services.assign_rating_to_exploration( + self.bob_id, self.EXP_ID_2, 5) + rating_services.assign_rating_to_exploration( + self.alice_id, self.EXP_ID_3, 5) + rating_services.assign_rating_to_exploration( + self.bob_id, self.EXP_ID_3, 4) + rating_services.assign_rating_to_exploration( + self.bob_id, self.EXP_ID_4, 4) + rating_services.assign_rating_to_exploration( + self.alice_id, self.EXP_ID_5, 4) + rating_services.assign_rating_to_exploration( + self.bob_id, self.EXP_ID_5, 3) + rating_services.assign_rating_to_exploration( + self.bob_id, self.EXP_ID_6, 3) + rating_services.assign_rating_to_exploration( + self.alice_id, self.EXP_ID_6, 2) + rating_services.assign_rating_to_exploration( + self.bob_id, self.EXP_ID_8, 2) + rating_services.assign_rating_to_exploration( + self.alice_id, self.EXP_ID_8, 2) + rating_services.assign_rating_to_exploration( + self.bob_id, self.EXP_ID_7, 2) + rating_services.assign_rating_to_exploration( + self.bob_id, self.EXP_ID_9, 2) + rating_services.assign_rating_to_exploration( + self.bob_id, self.EXP_ID_1, 1) + + top_rated_exploration_summaries = ( + summary_services.get_top_rated_exploration_summary_dicts([ + feconf.DEFAULT_LANGUAGE_CODE])) + expected_summary = { + 'status': u'public', + 'thumbnail_bg_color': '#a33f40', + 'community_owned': False, + 'tags': [], + 'thumbnail_icon_url': '/images/subjects/Lightbulb.svg', + 'language_code': feconf.DEFAULT_LANGUAGE_CODE, + 'id': self.EXP_ID_3, + 'category': u'A category', + 'ratings': {u'1': 0, u'3': 0, u'2': 0, u'5': 1, u'4': 1}, + 'title': u'A title', + 'num_views': 0, + 'objective': u'An objective' + } + + self.assertDictContainsSubset( + expected_summary, top_rated_exploration_summaries[0]) + + expected_ordering = [ + self.EXP_ID_3, self.EXP_ID_2, self.EXP_ID_5, self.EXP_ID_4, + self.EXP_ID_6, self.EXP_ID_8, self.EXP_ID_7, self.EXP_ID_9] + + actual_ordering = [exploration['id'] for exploration in + top_rated_exploration_summaries] + + self.assertEqual(expected_ordering, actual_ordering) + + def test_only_explorations_with_ratings_are_returned(self): + """Note that only explorations with ratings will be included + """ + rating_services.assign_rating_to_exploration( + self.bob_id, self.EXP_ID_2, 5) + + top_rated_exploration_summaries = ( + summary_services.get_top_rated_exploration_summary_dicts([ + feconf.DEFAULT_LANGUAGE_CODE])) + + expected_summary = { + 'status': u'public', + 'thumbnail_bg_color': '#a33f40', + 'community_owned': False, + 'tags': [], + 'thumbnail_icon_url': '/images/subjects/Lightbulb.svg', + 'language_code': feconf.DEFAULT_LANGUAGE_CODE, + 'id': self.EXP_ID_2, + 'category': u'A category', + 'ratings': {u'1': 0, u'3': 0, u'2': 0, u'5': 1, u'4': 0}, + 'title': u'A title', + 'num_views': 0, + 'objective': u'An objective' + } + self.assertDictContainsSubset( + expected_summary, top_rated_exploration_summaries[0]) + + expected_ordering = [self.EXP_ID_2] + + actual_ordering = [exploration['id'] for exploration in + top_rated_exploration_summaries] + + self.assertEqual(expected_ordering, actual_ordering) + + +class RecentlyPublishedExplorationDisplayableSummariesTest( + test_utils.GenericTestBase): + """Test functions for getting displayable recently published exploration + summary dicts. + """ + + ALBERT_NAME = 'albert' + ALBERT_EMAIL = 'albert@example.com' + + EXP_ID_1 = 'eid1' + EXP_ID_2 = 'eid2' + EXP_ID_3 = 'eid3' + + def setUp(self): + """Populate the database of explorations and their summaries. + + The sequence of events is: + - (1) Albert creates EXP_ID_1. + - (2) Albert creates EXP_ID_2. + - (3) Albert creates EXP_ID_3. + - (4) Albert publishes EXP_ID_1. + - (5) Albert publishes EXP_ID_2. + - (6) Albert publishes EXP_ID_3. + - (7) Admin user is set up. + """ + + super(RecentlyPublishedExplorationDisplayableSummariesTest, + self).setUp() + + self.admin_id = self.get_user_id_from_email(self.ADMIN_EMAIL) + self.albert_id = self.get_user_id_from_email(self.ALBERT_EMAIL) + self.signup(self.ADMIN_EMAIL, self.ADMIN_USERNAME) + self.signup(self.ALBERT_EMAIL, self.ALBERT_NAME) + + self.save_new_valid_exploration( + self.EXP_ID_1, self.albert_id, + end_state_name='End') + self.save_new_valid_exploration( + self.EXP_ID_2, self.albert_id, + end_state_name='End') + self.save_new_valid_exploration( + self.EXP_ID_3, self.albert_id, + end_state_name='End') + + rights_manager.publish_exploration(self.albert_id, self.EXP_ID_2) + rights_manager.publish_exploration(self.albert_id, self.EXP_ID_1) + rights_manager.publish_exploration(self.albert_id, self.EXP_ID_3) + + self.set_admins([self.ADMIN_USERNAME]) + + def test_for_recently_published_explorations(self): + """ Tests for recently published explorations. + """ + + recently_published_exploration_summaries = ( + summary_services.get_recently_published_exploration_summary_dicts()) + test_summary_1 = { + 'status': 'public', + 'thumbnail_bg_color': '#a33f40', + 'community_owned': False, + 'tags': [], + 'thumbnail_icon_url': '/images/subjects/Lightbulb.svg', + 'language_code': feconf.DEFAULT_LANGUAGE_CODE, + 'id': self.EXP_ID_1, + 'category': u'A category', + 'ratings': feconf.get_empty_ratings(), + 'title': u'A title', + 'num_views': 0, + 'objective': u'An objective' + } + test_summary_2 = { + 'status': 'public', + 'thumbnail_bg_color': '#a33f40', + 'community_owned': False, + 'tags': [], + 'thumbnail_icon_url': '/images/subjects/Lightbulb.svg', + 'language_code': feconf.DEFAULT_LANGUAGE_CODE, + 'id': self.EXP_ID_2, + 'category': u'A category', + 'ratings': feconf.get_empty_ratings(), + 'title': u'A title', + 'num_views': 0, + 'objective': u'An objective' + } + test_summary_3 = { + 'status': 'public', + 'thumbnail_bg_color': '#a33f40', + 'community_owned': False, + 'tags': [], + 'thumbnail_icon_url': '/images/subjects/Lightbulb.svg', + 'language_code': feconf.DEFAULT_LANGUAGE_CODE, + 'id': self.EXP_ID_3, + 'category': u'A category', + 'ratings': feconf.get_empty_ratings(), + 'title': u'A title', + 'num_views': 0, + 'objective': u'An objective' + } + + self.assertDictContainsSubset( + test_summary_3, recently_published_exploration_summaries[0]) + self.assertDictContainsSubset( + test_summary_1, recently_published_exploration_summaries[1]) + self.assertDictContainsSubset( + test_summary_2, recently_published_exploration_summaries[2]) + + # Test that editing an exploration does not change its + # 'recently-published' status. + exp_services.update_exploration( + self.albert_id, self.EXP_ID_1, [{ + 'cmd': 'edit_exploration_property', + 'property_name': 'title', + 'new_value': 'New title' + }], 'Changed title.') + + recently_published_exploration_summaries = ( + summary_services.get_recently_published_exploration_summary_dicts()) + self.assertEqual( + recently_published_exploration_summaries[1]['title'], 'New title') + self.assertDictContainsSubset( + test_summary_3, recently_published_exploration_summaries[0]) diff --git a/core/domain/user_jobs_continuous.py b/core/domain/user_jobs_continuous.py index 92d93f4b314a..0a0a6ca9713c 100644 --- a/core/domain/user_jobs_continuous.py +++ b/core/domain/user_jobs_continuous.py @@ -258,18 +258,18 @@ def reduce(key, stringified_values): ).put() -class UserImpactRealtimeModel( +class UserStatsRealtimeModel( jobs.BaseRealtimeDatastoreClassForContinuousComputations): pass -class UserImpactAggregator(jobs.BaseContinuousComputationManager): - """A continuous-computation job that computes the impact score - for every user. +class UserStatsAggregator(jobs.BaseContinuousComputationManager): + """A continuous-computation job that computes user stats for creator + dashboard. This job does not have a working realtime component: the - UserImpactRealtimeModel does nothing. There will be a delay in - propagating new updates to the profile page; the length of the + UserStatsRealtimeModel does nothing. There will be a delay in + propagating new updates to the view; the length of the delay will be approximately the time it takes a batch job to run. """ @classmethod @@ -278,29 +278,34 @@ def get_event_types_listened_to(cls): @classmethod def _get_realtime_datastore_class(cls): - return UserImpactRealtimeModel + return UserStatsRealtimeModel @classmethod def _get_batch_job_manager_class(cls): - return UserImpactMRJobManager + return UserStatsMRJobManager @classmethod def _handle_incoming_event(cls, active_realtime_layer, event_type, *args): pass -class UserImpactMRJobManager( +class UserStatsMRJobManager( jobs.BaseMapReduceJobManagerForContinuousComputations): - # Impact of user is defined as S^(2/3) where S is the sum - # over all explorations this user has contributed to of - # value (per_user) * reach * fractional contribution - # Value per user: average rating - 2 - # Reach: sum over all cards of count of answers given ^ (2/3) - # Fractional contribution: percent of commits by this user - # The final number will be rounded to the nearest integer. + """Job that calculates stats models for every user. + Includes: * average of average ratings of all explorations owned by the user + * sum of total plays of all explorations owned by the user + * impact score for all explorations contributed to by the user: + Impact of user is defined as S^(2/3) where S is the sum + over all explorations this user has contributed to of + value (per_user) * reach * fractional contribution + Value per user: average rating - 2 + Reach: sum over all cards of count of answers given ^ (2/3) + Fractional contribution: percent of commits by this user + The final number will be rounded to the nearest integer. + """ @classmethod def _get_continuous_computation_class(cls): - return UserImpactAggregator + return UserStatsAggregator @classmethod def entity_classes_to_map_over(cls): @@ -313,16 +318,25 @@ def map(item): exponent = 2.0/3 + # This is set to False only when the exploration impact score is not + # valid to be calculated. + calculate_exploration_impact_score = True + # Get average rating and value per user total_rating = 0 for ratings_value in item.ratings: total_rating += item.ratings[ratings_value] * int(ratings_value) - if not sum(item.ratings.itervalues()): - return - average_rating = total_rating / sum(item.ratings.itervalues()) - value_per_user = average_rating - 2 - if value_per_user <= 0: - return + sum_of_ratings = sum(item.ratings.itervalues()) + + average_rating = ((total_rating / sum_of_ratings) if sum_of_ratings + else None) + + if average_rating is not None: + value_per_user = average_rating - 2 + if value_per_user <= 0: + calculate_exploration_impact_score = False + else: + calculate_exploration_impact_score = False statistics = ( stats_jobs_continuous.StatisticsAggregator.get_statistics( @@ -347,18 +361,82 @@ def map(item): contributors = exploration_summary.contributors_summary total_commits = sum(contributors.itervalues()) if total_commits == 0: - return + calculate_exploration_impact_score = False + + mapped_owner_ids = [] for contrib_id in contributors: - # Find fractional contribution for each contributor - contribution = contributors[contrib_id] / float(total_commits) - # Find score for this specific exploration - exploration_impact_score = value_per_user * reach * contribution - yield (contrib_id, exploration_impact_score) + exploration_data = {} + + # Set the value of exploration impact score only if it needs to be + # calculated. + if calculate_exploration_impact_score: + # Find fractional contribution for each contributor + contribution = ( + contributors[contrib_id] / float(total_commits)) + + # Find score for this specific exploration + exploration_data.update({ + 'exploration_impact_score': ( + value_per_user * reach * contribution) + }) + + # if the user is an owner for the exploration, then update dict with + # 'average ratings' and 'total plays' as well. + if contrib_id in exploration_summary.owner_ids: + mapped_owner_ids.append(contrib_id) + # Get num starts (total plays) for the exploration + exploration_data.update({ + 'total_plays_for_owned_exp': ( + statistics['start_exploration_count']), + }) + # Update data with average rating only if it is not None. + if average_rating is not None: + exploration_data.update({ + 'average_rating_for_owned_exp': average_rating + }) + yield (contrib_id, exploration_data) + + for owner_id in exploration_summary.owner_ids: + if owner_id not in mapped_owner_ids: + mapped_owner_ids.append(owner_id) + # Get num starts (total plays) for the exploration + exploration_data = { + 'total_plays_for_owned_exp': ( + statistics['start_exploration_count']), + } + # Update data with average rating only if it is not None. + if average_rating is not None: + exploration_data.update({ + 'average_rating_for_owned_exp': average_rating + }) + yield (owner_id, exploration_data) @staticmethod def reduce(key, stringified_values): values = [ast.literal_eval(v) for v in stringified_values] exponent = 2.0/3 # Find the final score and round to a whole number - user_impact_score = int(round(sum(values) ** exponent)) - user_models.UserStatsModel(id=key, impact_score=user_impact_score).put() + user_impact_score = int(round(sum( + value['exploration_impact_score'] for value in values + if value.get('exploration_impact_score')) ** exponent)) + + # Sum up the total plays for all explorations + total_plays = sum( + value['total_plays_for_owned_exp'] for value in values + if value.get('total_plays_for_owned_exp')) + + # Find the average of all average ratings + ratings = [value['average_rating_for_owned_exp'] for value in values + if value.get('average_rating_for_owned_exp')] + if len(ratings) != 0: + average_ratings = (sum(ratings) / float(len(ratings))) + user_models.UserStatsModel( + id=key, + impact_score=user_impact_score, + total_plays=total_plays, + average_ratings=average_ratings).put() + else: + user_models.UserStatsModel( + id=key, + impact_score=user_impact_score, + total_plays=total_plays).put() diff --git a/core/domain/user_jobs_continuous_test.py b/core/domain/user_jobs_continuous_test.py index 6f6124fb74f8..eafb72374b3b 100644 --- a/core/domain/user_jobs_continuous_test.py +++ b/core/domain/user_jobs_continuous_test.py @@ -557,31 +557,32 @@ def test_basic_computation_works_if_collection_is_deleted(self): recent_notifications[0]['last_updated_ms']) -class ModifiedUserImpactAggregator( - user_jobs_continuous.UserImpactAggregator): - """A modified UserImpactAggregator that does not start a new +class ModifiedUserStatsAggregator( + user_jobs_continuous.UserStatsAggregator): + """A modified UserStatsAggregator that does not start a new batch job when the previous one has finished. """ @classmethod def _get_batch_job_manager_class(cls): - return ModifiedUserImpactMRJobManager + return ModifiedUserStatsMRJobManager @classmethod def _kickoff_batch_job_after_previous_one_ends(cls): pass -class ModifiedUserImpactMRJobManager( - user_jobs_continuous.UserImpactMRJobManager): +class ModifiedUserStatsMRJobManager( + user_jobs_continuous.UserStatsMRJobManager): @classmethod def _get_continuous_computation_class(cls): - return ModifiedUserImpactAggregator + return ModifiedUserStatsAggregator -class UserImpactAggregatorTest(test_utils.GenericTestBase): - """ Tests the calculation of a user's impact score from the - continuous computation of UserImpactAggregator. +class UserStatsAggregatorTest(test_utils.GenericTestBase): + """ Tests the calculation of a user's statistics - + impact score, average ratings, total plays + from the continuous computation of UserStatsAggregator. """ EXP_ID_1 = 'exp_id_1' @@ -595,14 +596,17 @@ class UserImpactAggregatorTest(test_utils.GenericTestBase): EXPONENT = 2.0/3 def setUp(self): - super(UserImpactAggregatorTest, self).setUp() + super(UserStatsAggregatorTest, self).setUp() self.num_completions = defaultdict(int) + self.num_starts = defaultdict(int) def _mock_get_statistics(self, exp_id, unused_version): current_completions = { self.EXP_ID_1: { 'complete_exploration_count': ( self.num_completions[self.EXP_ID_1]), + 'start_exploration_count': ( + self.num_starts[self.EXP_ID_1]), 'state_hit_counts': { 'state1': { 'first_entry_count': 3, @@ -617,6 +621,8 @@ def _mock_get_statistics(self, exp_id, unused_version): self.EXP_ID_2: { 'complete_exploration_count': ( self.num_completions[self.EXP_ID_2]), + 'start_exploration_count': ( + self.num_starts[self.EXP_ID_2]), 'state_hit_counts': { 'state1': { 'first_entry_count': 3, @@ -629,6 +635,8 @@ def _mock_get_statistics(self, exp_id, unused_version): } }, self.EXP_ID_3: { + 'start_exploration_count': ( + self.num_starts[self.EXP_ID_3]), 'state_hit_counts': {} } } @@ -652,7 +660,7 @@ def _run_computation(self): completion events.""" with self.swap(stats_jobs_continuous.StatisticsAggregator, 'get_statistics', self._mock_get_statistics): - ModifiedUserImpactAggregator.start_computation() + ModifiedUserStatsAggregator.start_computation() self.process_and_flush_pending_tasks() @@ -662,8 +670,7 @@ def _sign_up_user(self, user_email, username): return self.get_user_id_from_email(user_email) def _create_exploration(self, exp_id, user_id): - exploration = exp_domain.Exploration.create_default_exploration( - exp_id, 'A title', 'A category') + exploration = exp_domain.Exploration.create_default_exploration(exp_id) exp_services.save_new_exploration(user_id, exploration) return exploration @@ -678,9 +685,9 @@ def _rate_exploration(self, exp_id, num_ratings, rating): rating_services.assign_rating_to_exploration( user_id, exp_id, rating) - def test_user_with_no_explorations_has_no_impact(self): + def test_stats_for_user_with_no_explorations(self): """Test that a user who is not a contributor on any exploration - is not assigned an impact score by the UserImpactMRJobManager. + is not assigned value of impact score, total plays and average ratings. """ user_a_id = self._sign_up_user( self.USER_A_EMAIL, self.USER_A_USERNAME) @@ -689,7 +696,7 @@ def test_user_with_no_explorations_has_no_impact(self): user_a_id, strict=False) self.assertIsNone(user_stats_model) - def test_standard_user_impact_calculation_one_exploration(self): + def test_standard_user_stats_calculation_one_exploration(self): # Sign up a user and have them create an exploration. user_a_id = self._sign_up_user( self.USER_A_EMAIL, self.USER_A_USERNAME) @@ -738,7 +745,7 @@ def test_exploration_multiple_contributors(self): self.assertEqual( user_stats_model.impact_score, expected_user_impact_score) - def test_standard_user_impact_calculation_multiple_explorations(self): + def test_standard_user_stats_calculation_multiple_explorations(self): # Sign up a user and have them create two explorations. user_a_id = self._sign_up_user( self.USER_A_EMAIL, self.USER_A_USERNAME) @@ -774,16 +781,16 @@ def test_only_yield_when_rating_greater_than_two(self): self._run_computation() user_stats_model = user_models.UserStatsModel.get( user_a_id, strict=False) - self.assertIsNone(user_stats_model) - ModifiedUserImpactAggregator.stop_computation(user_a_id) + self.assertEqual(user_stats_model.impact_score, 0) + ModifiedUserStatsAggregator.stop_computation(user_a_id) # Give two ratings of 2. self._rate_exploration(self.EXP_ID_1, 2, 2) self._run_computation() user_stats_model = user_models.UserStatsModel.get( user_a_id, strict=False) - self.assertIsNone(user_stats_model) - ModifiedUserImpactAggregator.stop_computation(user_a_id) + self.assertEqual(user_stats_model.impact_score, 0) + ModifiedUserStatsAggregator.stop_computation(user_a_id) # Give two ratings of 3. The impact score should now be nonzero. self._rate_exploration(self.EXP_ID_1, 2, 3) diff --git a/core/domain/user_services.py b/core/domain/user_services.py index 5a265ee6b520..7c1644de91e8 100644 --- a/core/domain/user_services.py +++ b/core/domain/user_services.py @@ -47,7 +47,8 @@ def __init__( last_started_state_editor_tutorial=None, profile_picture_data_url=None, user_bio='', subject_interests=None, first_contribution_msec=None, - preferred_language_codes=None): + preferred_language_codes=None, + preferred_site_language_code=None): self.user_id = user_id self.email = email self.username = username @@ -61,6 +62,7 @@ def __init__( self.first_contribution_msec = first_contribution_msec self.preferred_language_codes = ( preferred_language_codes if preferred_language_codes else []) + self.preferred_site_language_code = preferred_site_language_code def validate(self): if not isinstance(self.user_id, basestring): @@ -204,7 +206,9 @@ def get_users_settings(user_ids): user_bio=model.user_bio, subject_interests=model.subject_interests, first_contribution_msec=model.first_contribution_msec, - preferred_language_codes=model.preferred_language_codes + preferred_language_codes=model.preferred_language_codes, + preferred_site_language_code=( + model.preferred_site_language_code) )) else: result.append(None) @@ -294,7 +298,8 @@ def _save_user_settings(user_settings): subject_interests=user_settings.subject_interests, first_contribution_msec=user_settings.first_contribution_msec, preferred_language_codes=user_settings.preferred_language_codes, - + preferred_site_language_code=( + user_settings.preferred_site_language_code) ).put() @@ -427,6 +432,13 @@ def update_preferred_language_codes(user_id, preferred_language_codes): _save_user_settings(user_settings) +def update_preferred_site_language_code(user_id, preferred_site_language_code): + user_settings = get_user_settings(user_id, strict=True) + user_settings.preferred_site_language_code = ( + preferred_site_language_code) + _save_user_settings(user_settings) + + def get_human_readable_user_ids(user_ids): """Converts the given ids to usernames, or truncated email addresses. diff --git a/core/jobs_registry.py b/core/jobs_registry.py index 993478bf0dca..87d833c8a88e 100644 --- a/core/jobs_registry.py +++ b/core/jobs_registry.py @@ -51,7 +51,7 @@ exp_jobs_continuous.SearchRanker, stats_jobs_continuous.StatisticsAggregator, user_jobs_continuous.DashboardRecentUpdatesAggregator, - user_jobs_continuous.UserImpactAggregator, + user_jobs_continuous.UserStatsAggregator, feedback_jobs_continuous.FeedbackAnalyticsAggregator, recommendations_jobs_continuous.ExplorationRecommendationsAggregator] diff --git a/core/jobs_test.py b/core/jobs_test.py index e5016a5a45e9..79becf1edb84 100644 --- a/core/jobs_test.py +++ b/core/jobs_test.py @@ -503,7 +503,7 @@ def setUp(self): """Create an exploration so that there is something to count.""" super(MapReduceJobIntegrationTests, self).setUp() exploration = exp_domain.Exploration.create_default_exploration( - 'exp_id', 'title', 'A category') + 'exp_id') exp_services.save_new_exploration('owner_id', exploration) self.process_and_flush_pending_tasks() @@ -603,7 +603,7 @@ def setUp(self): """Create an exploration so that there is something to count.""" super(TwoClassesMapReduceJobIntegrationTests, self).setUp() exploration = exp_domain.Exploration.create_default_exploration( - 'exp_id', 'title', 'A category') + 'exp_id') # Note that this ends up creating an entry in the # ExplorationRightsModel as well. exp_services.save_new_exploration('owner_id', exploration) @@ -741,7 +741,7 @@ def setUp(self): super(ContinuousComputationTests, self).setUp() exploration = exp_domain.Exploration.create_default_exploration( - self.EXP_ID, 'title', 'A category') + self.EXP_ID) exp_services.save_new_exploration('owner_id', exploration) self.process_and_flush_pending_tasks() diff --git a/core/platform/taskqueue/gae_taskqueue_services.py b/core/platform/taskqueue/gae_taskqueue_services.py index 45ae8afdb825..136c57a9f96d 100644 --- a/core/platform/taskqueue/gae_taskqueue_services.py +++ b/core/platform/taskqueue/gae_taskqueue_services.py @@ -16,6 +16,7 @@ """Provides a seam for taskqueue-related operations.""" +from google.appengine.api import taskqueue from google.appengine.ext import deferred # NOTE: The following constants should match the queue names in queue.yaml. @@ -23,7 +24,8 @@ QUEUE_NAME_DEFAULT = 'default' # Deferred queue for processing events outside the request/response cycle. QUEUE_NAME_EVENTS = 'events' - +# Taskqueue for sending email. +QUEUE_NAME_EMAILS = 'emails' def defer(fn, *args, **kwargs): """Adds a new task to the default deferred queue.""" @@ -36,6 +38,20 @@ def defer_to_events_queue(fn, *args, **kwargs): # details on the _queue kwarg. deferred.defer(fn, *args, _queue=QUEUE_NAME_EVENTS, **kwargs) +def enqueue_task(url, params, countdown): + """Adds a new task for sending email. + + Args: + - url: url of the handler function. + - params: parameters that will be passed as payload to handler + function. + - countdown: amount of time, in seconds, to wait before executing task. + """ + # See https://cloud.google.com/appengine/docs/python/taskqueue for + # details of various parameters set when adding a new task. + taskqueue.add( + queue_name=QUEUE_NAME_EMAILS, url=url, params=params, + countdown=countdown, target=taskqueue.DEFAULT_APP_VERSION) # A special exception that ensures that the task is not tried again, if it # fails. diff --git a/core/storage/collection/gae_models.py b/core/storage/collection/gae_models.py index ea68b041b1d3..b6a7bdc284e0 100644 --- a/core/storage/collection/gae_models.py +++ b/core/storage/collection/gae_models.py @@ -51,6 +51,11 @@ class CollectionModel(base_models.VersionedModel): category = ndb.StringProperty(required=True, indexed=True) # The objective of this collection. objective = ndb.TextProperty(default='', indexed=False) + # The language code of this collection. + language_code = ndb.StringProperty( + default=feconf.DEFAULT_LANGUAGE_CODE, indexed=True) + # Tags associated with this collection. + tags = ndb.StringProperty(repeated=True, indexed=True) # The version of all property blob schemas. schema_version = ndb.IntegerProperty( @@ -278,6 +283,10 @@ class CollectionSummaryModel(base_models.BaseModel): category = ndb.StringProperty(required=True, indexed=True) # The objective of this collection. objective = ndb.TextProperty(required=True, indexed=False) + # The ISO 639-1 code for the language this collection is written in. + language_code = ndb.StringProperty(required=True, indexed=True) + # Tags associated with this collection. + tags = ndb.StringProperty(repeated=True, indexed=True) # Aggregate user-assigned ratings of the collection ratings = ndb.JsonProperty(default=None, indexed=False) @@ -320,6 +329,8 @@ class CollectionSummaryModel(base_models.BaseModel): # The version number of the collection after this commit. Only populated # for commits to an collection (as opposed to its rights, etc.) version = ndb.IntegerProperty() + # The number of nodes(explorations) that are within this collection. + node_count = ndb.IntegerProperty() @classmethod def get_non_private(cls): diff --git a/core/storage/exploration/gae_models.py b/core/storage/exploration/gae_models.py index 1e0b2069f913..942755bf5097 100644 --- a/core/storage/exploration/gae_models.py +++ b/core/storage/exploration/gae_models.py @@ -310,14 +310,16 @@ class ExpSummaryModel(base_models.BaseModel): # The objective of this exploration. objective = ndb.TextProperty(required=True, indexed=False) # The ISO 639-1 code for the language this exploration is written in. - language_code = ndb.StringProperty( - required=True, indexed=True) + language_code = ndb.StringProperty(required=True, indexed=True) # Tags associated with this exploration. tags = ndb.StringProperty(repeated=True, indexed=True) # Aggregate user-assigned ratings of the exploration ratings = ndb.JsonProperty(default=None, indexed=False) + # Scaled average rating for the exploration. + scaled_average_rating = ndb.FloatProperty(indexed=True) + # Time when the exploration model was last updated (not to be # confused with last_updated, which is the time when the # exploration *summary* model was last updated) @@ -326,6 +328,8 @@ class ExpSummaryModel(base_models.BaseModel): # with created_on, which is the time when the exploration *summary* # model was created) exploration_model_created_on = ndb.DateTimeProperty(indexed=True) + # Time when the exploration was first published. + first_published_msec = ndb.FloatProperty(indexed=True) # The publication status of this exploration. status = ndb.StringProperty( @@ -375,6 +379,20 @@ def get_featured(cls): ExpSummaryModel.deleted == False # pylint: disable=singleton-comparison ).fetch(feconf.DEFAULT_QUERY_LIMIT) + @classmethod + def get_top_rated(cls): + """Returns an iterable with the top rated exp summaries that are + public in descending order. + """ + return ExpSummaryModel.query().filter( + ndb.OR(ExpSummaryModel.status == feconf.ACTIVITY_STATUS_PUBLIC, + ExpSummaryModel.status == feconf.ACTIVITY_STATUS_PUBLICIZED) + ).filter( + ExpSummaryModel.deleted == False # pylint: disable=singleton-comparison + ).order( + -ExpSummaryModel.scaled_average_rating + ).fetch(feconf.NUMBER_OF_TOP_RATED_EXPLORATIONS) + @classmethod def get_private_at_least_viewable(cls, user_id): """Returns an iterable with private exp summaries that are at least @@ -401,3 +419,17 @@ def get_at_least_editable(cls, user_id): ).filter( ExpSummaryModel.deleted == False # pylint: disable=singleton-comparison ).fetch(feconf.DEFAULT_QUERY_LIMIT) + + @classmethod + def get_recently_published(cls): + """Returns an iterable with exp summaries that are recently + published. + """ + return ExpSummaryModel.query().filter( + ndb.OR(ExpSummaryModel.status == feconf.ACTIVITY_STATUS_PUBLIC, + ExpSummaryModel.status == feconf.ACTIVITY_STATUS_PUBLICIZED) + ).filter( + ExpSummaryModel.deleted == False # pylint: disable=singleton-comparison + ).order( + -ExpSummaryModel.first_published_msec + ).fetch(feconf.RECENTLY_PUBLISHED_QUERY_LIMIT) diff --git a/core/storage/feedback/gae_models.py b/core/storage/feedback/gae_models.py index 18077472aeae..a59312d20155 100644 --- a/core/storage/feedback/gae_models.py +++ b/core/storage/feedback/gae_models.py @@ -287,3 +287,24 @@ def get_by_exploration_and_thread_id(cls, exploration_id, thread_id): Returns None if it doesn't match anything.""" return cls.get_by_id(cls._get_instance_id(exploration_id, thread_id)) + + +class UnsentFeedbackEmailModel(base_models.BaseModel): + """Model for storing feedback messages that need to be sent to creators. + + Instances of this model contain information about feedback messages that + have been received by the site, but have not yet been sent to creators. + The model instances will be deleted once the corresponding email has been + sent. + + The id of each model instance is the user_id of the user who should receive + the messages.""" + + # The list of feedback messages that need to be sent to this user. + # Each element in this list is a dict with keys 'exploration_id', + # 'thread_id' and 'message_id'; this information is used to retrieve + # corresponding FeedbackMessageModel instance. + feedback_messages = ndb.JsonProperty(repeated=True) + # The number of failed attempts that have been made (so far) to + # send an email to this user. + retries = ndb.IntegerProperty(default=0, required=True, indexed=True) diff --git a/core/storage/feedback/gae_models_test.py b/core/storage/feedback/gae_models_test.py index 3eebf349efe5..d2fa713b2cb3 100644 --- a/core/storage/feedback/gae_models_test.py +++ b/core/storage/feedback/gae_models_test.py @@ -107,3 +107,25 @@ def test_get_by_exploration_and_thread_id_no_suggestion(self): 'invalid_exp_id', 'thread_id1')) self.assertIsNone(actual_suggestion) + + +class UnsentFeedbackEmailModelTest(test_utils.GenericTestBase): + """Tests for FeedbackMessageEmailDataModel class""" + + def test_new_instances_stores_correct_data(self): + user_id = 'A' + feedback_messages_dict = { + 'exploration_id': 'ABC123', + 'thread_id': 'thread_id1', + 'message_id': 'message_id1' + } + email_instance = feedback_models.UnsentFeedbackEmailModel( + id=user_id, feedback_messages=[feedback_messages_dict]) + email_instance.put() + + retrieved_instance = ( + feedback_models.UnsentFeedbackEmailModel.get_by_id(id=user_id)) + + self.assertEqual( + retrieved_instance.feedback_messages, [feedback_messages_dict]) + self.assertEqual(retrieved_instance.retries, 0) diff --git a/core/storage/user/gae_models.py b/core/storage/user/gae_models.py index d57f8a13c2fb..b05cf985971f 100644 --- a/core/storage/user/gae_models.py +++ b/core/storage/user/gae_models.py @@ -49,7 +49,7 @@ class UserSettingsModel(base_models.BaseModel): # The time, in milliseconds, when the user first contributed to Oppia. # May be None. first_contribution_msec = ndb.FloatProperty(default=None) - # Language preferences specified by the user. + # Exploration language preferences specified by the user. # TODO(sll): Add another field for the language that the user wants the # site to display in. These language preferences are mainly for the purpose # of figuring out what to show by default in the library index page. @@ -57,6 +57,10 @@ class UserSettingsModel(base_models.BaseModel): repeated=True, indexed=True, choices=[lc['code'] for lc in feconf.ALL_LANGUAGE_CODES]) + # System language preference (for I18N). + preferred_site_language_code = ndb.StringProperty( + default=None, + choices=feconf.SUPPORTED_SITE_LANGUAGES.keys()) @classmethod def is_normalized_username_taken(cls, normalized_username): @@ -132,7 +136,13 @@ class UserRecentChangesBatchModel(base_models.BaseMapReduceBatchResultsModel): class UserStatsModel(base_models.BaseMapReduceBatchResultsModel): - """The impact score for a particular user, where impact is defined as: + """User-specific statistics keyed by user id. + Values for total plays and average ratings are recorded by aggregating over + all explorations owned by a user. + Impact scores are calculated over explorations for which a user + is listed as a contributor + + The impact score for a particular user is defined as: Sum of ( ln(playthroughs) * (ratings_scaler) * (average(ratings) - 2.5)) *(multiplier), @@ -141,13 +151,16 @@ class UserStatsModel(base_models.BaseMapReduceBatchResultsModel): The impact score is 0 for an exploration with 0 playthroughs or with an average rating of less than 2.5. - - Impact scores are calculated over explorations for which a user - is listed as a contributor. Keys for this model are user_ids. """ # The impact score. impact_score = ndb.FloatProperty(indexed=True) + # The total plays of all the explorations. + total_plays = ndb.IntegerProperty(indexed=True) + + # The average of average ratings of all explorations. + average_ratings = ndb.FloatProperty(indexed=True) + class ExplorationUserDataModel(base_models.BaseModel): """User-specific data pertaining to a specific exploration. diff --git a/core/templates/dev/head/admin/Admin.js b/core/templates/dev/head/admin/Admin.js index 665cc3f689d6..a1c93c62610d 100644 --- a/core/templates/dev/head/admin/Admin.js +++ b/core/templates/dev/head/admin/Admin.js @@ -115,7 +115,7 @@ oppia.controller('Admin', ['$scope', '$http', function($scope, $http) { }; $scope.saveConfigProperties = function() { - if ($scope.message == 'Saving...') { + if ($scope.message === 'Saving...') { return; } diff --git a/core/templates/dev/head/admin/admin.html b/core/templates/dev/head/admin/admin.html index f4ba6a0a9dd0..e1703c08255e 100644 --- a/core/templates/dev/head/admin/admin.html +++ b/core/templates/dev/head/admin/admin.html @@ -65,8 +65,8 @@
  • - - My Explorations + + Creator Dashboard
  • @@ -118,7 +118,7 @@ Jobs
    • -
  • +
  • Activities diff --git a/core/templates/dev/head/app.js b/core/templates/dev/head/app.js index c4754f92bf1d..d34d1cd56de1 100644 --- a/core/templates/dev/head/app.js +++ b/core/templates/dev/head/app.js @@ -24,7 +24,8 @@ var oppia = angular.module( 'oppia', [ 'ngMaterial', 'ngAnimate', 'ngSanitize', 'ngTouch', 'ngResource', 'ui.bootstrap', 'ui.sortable', 'infinite-scroll', 'ngJoyRide', 'ngImgCrop', - 'ui.validate', 'textAngular', 'toastr' + 'ui.validate', 'textAngular', 'pascalprecht.translate', 'ngCookies', + 'toastr' ].concat( window.GLOBALS ? (window.GLOBALS.ADDITIONAL_ANGULAR_MODULES || []) : [])); @@ -46,7 +47,7 @@ oppia.config([ // Prevent the search page from reloading if the search query is changed. $locationProvider.html5Mode(false); - if (window.location.pathname == '/search/find') { + if (window.location.pathname === '/search/find') { $locationProvider.html5Mode(true); } @@ -66,9 +67,16 @@ oppia.config([ request: function(config) { // If this request carries data (in the form of a JS object), // JSON-stringify it and store it under 'payload'. + var csrfToken = ''; if (config.data) { + var csrfToken = ( + config.requestIsForCreateExploration ? + GLOBALS.csrf_token_create_exploration : + config.requestIsForI18n ? GLOBALS.csrf_token_i18n : + GLOBALS.csrf_token); + config.data = $.param({ - csrf_token: GLOBALS.csrf_token, + csrf_token: csrfToken, payload: JSON.stringify(config.data), source: document.URL }, true); @@ -232,10 +240,10 @@ oppia.factory('oppiaDatetimeFormatter', ['$filter', function($filter) { // Otherwise, returns the full date (with the year abbreviated). getLocaleAbbreviatedDatetimeString: function(millisSinceEpoch) { var date = new Date(millisSinceEpoch); - if (date.toLocaleDateString() == new Date().toLocaleDateString()) { + if (date.toLocaleDateString() === new Date().toLocaleDateString()) { // The replace function removes 'seconds' from the time returned. return date.toLocaleTimeString().replace(/:\d\d /, ' '); - } else if (date.getFullYear() == new Date().getFullYear()) { + } else if (date.getFullYear() === new Date().getFullYear()) { return $filter('date')(date, 'MMM d'); } else { return $filter('date')(date, 'shortDate'); @@ -267,9 +275,9 @@ oppia.factory('validatorsService', [ * butterbar. * @return {boolean} True if the entity name is valid, false otherwise. */ - isValidEntityName: function(input, showWarnings) { + isValidEntityName: function(input, showWarnings, allowEmpty) { input = $filter('normalizeWhitespace')(input); - if (!input) { + if (!input && !allowEmpty) { if (showWarnings) { alertsService.addWarning('Please enter a non-empty name.'); } @@ -289,6 +297,21 @@ oppia.factory('validatorsService', [ } return true; }, + isValidExplorationTitle: function(input, showWarnings) { + if (!this.isValidEntityName(input, showWarnings)) { + return false; + } + + if (input.length > 40) { + if (showWarnings) { + alertsService.addWarning( + 'Exploration titles should be at most 40 characters long.'); + } + return false; + } + + return true; + }, // NB: this does not check whether the card name already exists in the // states dict. isValidStateName: function(input, showWarnings) { @@ -443,13 +466,15 @@ oppia.factory('siteAnalyticsService', ['$window', function($window) { _sendEventToGoogleAnalytics( 'CreateExplorationButton', 'click', $window.location.pathname); }, - registerOpenExplorationCreationModalEvent: function() { - _sendEventToGoogleAnalytics( - 'CreateExplorationModal', 'open', $window.location.pathname); - }, registerCreateNewExplorationEvent: function(explorationId) { + _sendEventToGoogleAnalytics('NewExploration', 'create', explorationId); + }, + registerCreateNewExplorationInCollectionEvent: function(explorationId) { _sendEventToGoogleAnalytics( - 'NewExploration', 'create', explorationId); + 'NewExplorationFromCollection', 'create', explorationId); + }, + registerCreateNewCollectionEvent: function(collectionId) { + _sendEventToGoogleAnalytics('NewCollection', 'create', collectionId); }, registerCommitChangesToPrivateExplorationEvent: function(explorationId) { _sendEventToGoogleAnalytics( diff --git a/core/templates/dev/head/appSpec.js b/core/templates/dev/head/appSpec.js index ed9ab04bb0d0..eb0169f09115 100644 --- a/core/templates/dev/head/appSpec.js +++ b/core/templates/dev/head/appSpec.js @@ -43,6 +43,23 @@ describe('Validators service', function() { expect(vs.isValidEntityName('bx')).toBe(false); }); + it('should correctly validate exploration titles', function() { + GLOBALS = { + INVALID_NAME_CHARS: '#' + }; + + expect(vs.isValidExplorationTitle('b')).toBe(true); + expect(vs.isValidExplorationTitle('abc def')).toBe(true); + + expect(vs.isValidExplorationTitle('')).toBe(false); + expect(vs.isValidExplorationTitle(null)).toBe(false); + expect(vs.isValidExplorationTitle(undefined)).toBe(false); + expect(vs.isValidExplorationTitle( + 'A title with invalid characters #')).toBe(false); + expect(vs.isValidExplorationTitle( + 'A title that is way way way way way way way too long.')).toBe(false); + }); + it('should correctly validate non-emptiness', function() { expect(vs.isNonempty('b')).toBe(true); expect(vs.isNonempty('abc def')).toBe(true); diff --git a/core/templates/dev/head/base.html b/core/templates/dev/head/base.html index 77106a4afa0c..5985470399ec 100644 --- a/core/templates/dev/head/base.html +++ b/core/templates/dev/head/base.html @@ -17,7 +17,7 @@
    - <[loadingMessage]> + . . . @@ -56,8 +56,16 @@ - {% block subtitle %}{% endblock %} - {% block maintitle %}Oppia{% endblock %} + + + {% block base_url %}{% endblock base_url %} + {% block header_css %} {% include 'header_css_libs.html' %} {% endblock header_css %} @@ -66,6 +74,8 @@ var GLOBALS = { ADDITIONAL_ANGULAR_MODULES: [], csrf_token: JSON.parse('{{csrf_token|js_string}}'), + csrf_token_i18n: JSON.parse('{{csrf_token_i18n|js_string}}'), + csrf_token_create_exploration: JSON.parse('{{csrf_token_create_exploration|js_string}}'), DEV_MODE: JSON.parse('{{DEV_MODE|js_string}}'), INVALID_NAME_CHARS: JSON.parse('{{INVALID_NAME_CHARS|js_string}}'), ACTIVITY_STATUS_PRIVATE: JSON.parse( @@ -84,7 +94,9 @@ /* A list of functions to be called when an exploration is completed. */ POST_COMPLETION_HOOKS: [], SYSTEM_USERNAMES: JSON.parse('{{SYSTEM_USERNAMES|js_string}}'), - userIsLoggedIn: JSON.parse('{{user_is_logged_in|js_string}}') + userIsLoggedIn: JSON.parse('{{user_is_logged_in|js_string}}'), + preferredSiteLanguageCode: JSON.parse('{{preferred_site_language_code|js_string}}'), + SUPPORTED_SITE_LANGUAGES: JSON.parse('{{SUPPORTED_SITE_LANGUAGES|js_string}}') }; {% if additional_angular_modules %} @@ -124,6 +136,7 @@
    + {% if nav_mode != 'signup' %}
      @@ -161,6 +174,7 @@ {% endif %}
    +
    • {% else %} -
  • +
  • @@ -223,40 +241,39 @@
    • {% endif %} + +