Fix #5466: Added check to enforce that all name of controllers end with "Handler" or "Page" #5878

lilithxxx · 2018-11-16T18:27:42Z

Fixes #5466
This PR adds a function to check all controller's name end with 'Handler' or 'Page'.
The controllers that are sub-classes of the base class which did not end with the words are changed accordingly.

This PR is a duplicate of #5641

Explanation

Checklist

The PR title starts with "Fix #bugnum: ", followed by a short, clear summary of the changes. (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)
The PR explanation includes the words "Fixes #bugnum: ..." (or "Fixes part of #bugnum" if the PR only partially fixes an issue).
The linter/Karma presubmit checks have passed.
- These should run automatically, but if not, you can manually trigger them locally using python scripts/pre_commit_linter.py and bash scripts/run_frontend_tests.sh.
The PR is made from a branch that's not called "develop".
The PR follows the style guide.
The PR is assigned to an appropriate reviewer.
- If you're a new contributor, please ask on Gitter for someone to assign a reviewer.
- If you're not sure who the appropriate reviewer is, please assign to the issue's "owner" -- see the "talk-to" label on the issue.

seanlip · 2018-11-16T20:17:40Z

Hi @lilithxxx, FYI the travis build is still failing. Could you please investigate?

Also please include a reference in this PR to the other PR (and close one of them so that there's just a single point of focus ).

Thanks!

codecov-io · 2018-11-16T21:06:13Z

Codecov Report

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

@@           Coverage Diff            @@
##           develop    #5878   +/-   ##
========================================
  Coverage    45.53%   45.53%           
========================================
  Files          517      517           
  Lines        30335    30335           
  Branches      4562     4562           
========================================
  Hits         13812    13812           
  Misses       16523    16523

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cf4939c...af4b4e1. Read the comment docs.

lilithxxx · 2018-11-16T23:36:34Z

@seanlip made the changes, PTAL

seanlip · 2018-11-17T00:38:12Z

core/controllers/base_test.py

+        """
+        def get(self):
+            """Handles GET requests."""
+            text = 'example'


text is vague -- consider file_contents instead.

seanlip · 2018-11-17T00:39:15Z

core/controllers/collection_editor_test.py

@@ -107,8 +107,8 @@ def test_editable_collection_handler_get(self):
        response = self.testapp.get(
            '%s/%s' % (
                feconf.COLLECTION_EDITOR_DATA_URL_PREFIX,
-                self.COLLECTION_ID))
-        self.assertEqual(response.status_int, 302)
+                self.COLLECTION_ID), expect_errors=True)


I don't understand why your PR is causing the underlying behaviour here to change. Conceptually this PR is supposed to be just a renaming, so this looks like an unintended consequence.

Can you dig into this and find out why it changed? Tests are supposed to protect against this sort of thing; when a test fails, it may be due to an error in the newly-introduced code.

@seanlip Isn't this an expected behaviour? This test checks that non-editors cannot access the editor data handler due to them not being whitelisted. Hence if we dont add expect_errors there's an app error of not being authorised.

But why did your PR in particular cause this behaviour to change?

(I think 302 typically represents a redirect to the login page. So it's not necessarily wrong. But the more pertinent question is the one above.)

It seems the error occurs when modifying EditableCollectionDataHandler in collection_editor by adding GET_HANDLER_ERROR_RETURN_TYPE. There seems to be some kind of authorisation problem.

OK, that's a good start. Can you continue digging into this and tracing what exactly happens, then explaining that in this comment? It's important that we understand what exactly we are modifying, especially with something so crucial as rights management -- we need to get that right.

So, perhaps explain what the flow was like before, and then explain what the flow is now after your modification.

Thanks!

Hi @seanlip. The error is occurring in this line. For NotLoggedInException its matching if the error_return_type is 'json' or not as there is no payload for GET requests. This error does not occur in other classes as there are either tests for logged in users only or for controllers rendering HTML

Ah, ok, thanks for checking! That's a good find.

Looking at that part of the code, I think the logic is not quite correct. Could we modify it so that we return the HTML error page response only if the request is GET and the handler type is HTML, otherwise we return the JSON response?

@seanlip Upon changing that piece of code other error pops up like here and here.
I think the above ^^ is expected and the error that now shows should be changed. If the user is not logged in the error that should be displayed is error 401 and not 302.

I agree that 401 errors make sense in both these cases. But they should be JSON responses, right? I'm not sure how this conceptually conflicts with the last comment I made.... could you please explain?

(Note that my last comment talks about the form of the response -- HTML vs JSON -- and does not suggest any changes to the response status codes.)

oppiabot · 2018-11-17T10:06:05Z

Hi @lilithxxx. Due to recent changes in the "develop" branch, this PR now has a merge conflict. Please follow this link if you need help resolving the conflict, so that the PR can be merged. Thanks!

oppiabot · 2018-11-25T19:08:34Z

Hi @lilithxxx. Due to recent changes in the "develop" branch, this PR now has a merge conflict. Please follow this link if you need help resolving the conflict, so that the PR can be merged. Thanks!

lilithxxx · 2018-11-26T18:22:24Z

@seanlip made changes. PTAL

seanlip · 2018-11-26T20:50:39Z

core/controllers/base.py

-        elif return_type == feconf.HANDLER_TYPE_HTML:
+        method = self.request.environ['REQUEST_METHOD']
+
+        if(return_type == feconf.HANDLER_TYPE_HTML and


Missing space before '('

@apb7 -- shouldn't the linter catch this?

seanlip · 2018-11-26T20:51:07Z

core/controllers/base.py

            self.values.update(values)
            if 'iframed' in self.values and self.values['iframed']:
                self.render_template(
                    'pages/error/error_iframed.html', iframe_restriction=None)
            else:
                self.render_template('pages/error/error.html')
        else:
-            logging.warning('Not a recognized return type: '
-                            'defaulting to render JSON.')
+            if return_type != feconf.HANDLER_TYPE_JSON:


What if the return type is DOWNLOADABLE (say)?

seanlip · 2018-11-26T20:52:18Z

core/controllers/base_test.py

+            file_contents = 'example'
+            self.render_downloadable_file(
+                file_contents, 'example.pdf',
+                'text/plain')


Move to end of previous line.

lilithxxx · 2018-11-27T10:46:04Z

@seanlip made changes. PTAL

seanlip · 2018-11-27T10:51:56Z

@lilithxxx, why are you force pushing? Please don't do that. It messes up the history and makes it harder to review things.

Otherwise, the PR LGTM, and I have no further comments. Thanks!

lilithxxx · 2018-11-27T10:56:53Z

Actually there were conflicts so I had to force push. I won't do that again

seanlip · 2018-11-27T10:59:42Z

Thanks for explaining! For future reference, see notes in step 5 here for how to merge code -- you should address the conflicts directly when you're doing the merge, by searching for "<<< HEAD" say. See https://help.github.com/articles/resolving-a-merge-conflict-using-the-command-line/ for more details.

(And yep, never force-push again, please. Thanks!)

Rishav Chakraborty added 7 commits November 16, 2018 21:05

Added line breaks

bff1fca

Moved test to base_test

dc28ba3

Modified code

c2cb23e

Modified code

843d180

Changed condition

e630d23

Modified render_downloadable test

67634f9

Check for class name test

41c3ff5

lilithxxx mentioned this pull request Nov 16, 2018

Fix #5466: Added check to enforce that all name of controllers end with "Handler" or "Page" #5641

Closed

6 tasks

lilithxxx requested review from seanlip and apb7 November 16, 2018 18:37

Fixed test case

737ee27

seanlip requested changes Nov 17, 2018

View reviewed changes

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

Remove conflicts

d570f1d

oppiabot bot added PR: don't merge - HAS MERGE CONFLICTS and removed PR: don't merge - HAS MERGE CONFLICTS labels Nov 23, 2018

Change condition of html response

84d54cc

lilithxxx force-pushed the new_base branch from bff0c62 to 84d54cc Compare November 26, 2018 18:11

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

seanlip reviewed Nov 26, 2018

View reviewed changes

Made lint changes

0f7b1ff

lilithxxx force-pushed the new_base branch from 9180b55 to 0f7b1ff Compare November 27, 2018 10:35

Merge branch 'develop' into new_base

af4b4e1

seanlip approved these changes Nov 27, 2018

View reviewed changes

seanlip added the PR: LGTM label Nov 27, 2018

seanlip merged commit d7c44b2 into oppia:develop Nov 28, 2018

lilithxxx deleted the new_base branch December 1, 2018 20:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #5466: Added check to enforce that all name of controllers end with "Handler" or "Page" #5878

Fix #5466: Added check to enforce that all name of controllers end with "Handler" or "Page" #5878

lilithxxx commented Nov 16, 2018 •

edited

Loading

seanlip commented Nov 16, 2018

codecov-io commented Nov 16, 2018 •

edited

Loading

lilithxxx commented Nov 16, 2018

seanlip Nov 17, 2018

seanlip Nov 17, 2018

lilithxxx Nov 17, 2018

seanlip Nov 17, 2018 •

edited

Loading

lilithxxx Nov 17, 2018

seanlip Nov 17, 2018

lilithxxx Nov 22, 2018

seanlip Nov 23, 2018 •

edited

Loading

lilithxxx Nov 23, 2018

seanlip Nov 24, 2018 •

edited

Loading

lilithxxx Nov 26, 2018

oppiabot bot commented Nov 17, 2018

oppiabot bot commented Nov 25, 2018

lilithxxx commented Nov 26, 2018

seanlip Nov 26, 2018

lilithxxx Nov 27, 2018

seanlip Nov 26, 2018

lilithxxx Nov 27, 2018

seanlip Nov 26, 2018

lilithxxx Nov 27, 2018

lilithxxx commented Nov 27, 2018

seanlip commented Nov 27, 2018

lilithxxx commented Nov 27, 2018

seanlip commented Nov 27, 2018 •

edited

Loading

Fix #5466: Added check to enforce that all name of controllers end with "Handler" or "Page" #5878

Fix #5466: Added check to enforce that all name of controllers end with "Handler" or "Page" #5878

Conversation

lilithxxx commented Nov 16, 2018 • edited Loading

Explanation

Checklist

seanlip commented Nov 16, 2018

codecov-io commented Nov 16, 2018 • edited Loading

Codecov Report

lilithxxx commented Nov 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanlip Nov 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanlip Nov 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanlip Nov 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oppiabot bot commented Nov 17, 2018

oppiabot bot commented Nov 25, 2018

lilithxxx commented Nov 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lilithxxx commented Nov 27, 2018

seanlip commented Nov 27, 2018

lilithxxx commented Nov 27, 2018

seanlip commented Nov 27, 2018 • edited Loading

lilithxxx commented Nov 16, 2018 •

edited

Loading

codecov-io commented Nov 16, 2018 •

edited

Loading

seanlip Nov 17, 2018 •

edited

Loading

seanlip Nov 23, 2018 •

edited

Loading

seanlip Nov 24, 2018 •

edited

Loading

seanlip commented Nov 27, 2018 •

edited

Loading