Skip to content
This repository has been archived by the owner on Dec 4, 2018. It is now read-only.

Cleanup main view and add tests #67

Merged
merged 1 commit into from
Sep 5, 2017
Merged

Cleanup main view and add tests #67

merged 1 commit into from
Sep 5, 2017

Conversation

chosak
Copy link
Member

@chosak chosak commented Sep 5, 2017

This commit converts the landing page (main view) into a Django class-based view and moves it into its own module. It also adds various unit tests to cover this view.

This is a partial fix to the issue in #44 where non-GET (e.g. POST) requests to this view would raise a 500 error.

This commit adds a dummy data/header-only.xml RegML file (which validates against the schema) that can be used for simple testing of this view only. The new unit tests for this view validate that it works with no data or with data loaded.

This commit also adds a simple unittest against eregs_core.urls to ensure that the URLs module can be imported (to protect against syntax errors which would otherwise not be discovered via the test suite).

Additions

  • New test cases against the main view and the urls.

Changes

  • Moves eregs_core.views.main into a Django class-based view at eregs_core.views.main.MainView.

Testing

  • Run unit tests with e.g. tox -e dj18.
  • Run a local server with ./manage.py runserver and navigate to http://localhost:8000.

Checklist

  • Changes are limited to a single goal (no scope creep)
  • Code can be automatically merged (no conflicts)
  • Code follows the standards laid out in the front end playbook
  • Passes all existing automated tests
  • New functions include new tests
  • Visually tested in supported browsers and devices
  • Project documentation has been updated (including the "Unreleased" section of the CHANGELOG)

This commit converts the landing page (main view) into a Django
class-based view and moves it into its own module. It also adds various
unit tests to cover this view.

This is a partial fix to the issue in #44 where non-GET (e.g. POST)
requests to this view would raise a 500 error.

This commit adds a dummy `data/header-only.xml` RegML file (which
validates against the schema) that can be used for simple testing of
this view only. The new unit tests for this view validate that it works
with no data or with data loaded.

This commit also adds a simple unittest against `eregs_core.urls` to
ensure that the URLs module can be imported (to protect against syntax
errors which would otherwise not be discovered via the test suite).
Copy link
Contributor

@grapesmoker grapesmoker left a comment

Choose a reason for hiding this comment

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

Just curious, what's the reason for this? I get that posting to main raises a 500, but... you shouldn't be allowed to post there. We can add a case to handle that, too. Does the class-based view provide us with some additional flexibility?

@chosak
Copy link
Member Author

chosak commented Sep 5, 2017

CBVs make views easier to test and reduce code duplication. Using CBVs automatically provides proper handling of non-GET requests (returning 405), meaning we don't have to reimplement it for every view. We could implement "you shouldn't be allowed to POST there" by putting logic like

if request.method != 'GET':
    return HttpResponseNotAllowed()

everywhere it's needed but then we'd have to reproduce this on every single view (see #44).

Similarly CBVs let us use standard Django patterns like defining template_name on the class instead of having to manually call render_to_response.

@grapesmoker
Copy link
Contributor

Cool, that makes sense.

@chosak chosak merged commit c068461 into master Sep 5, 2017
@chosak chosak deleted the cleanup-main-view branch September 5, 2017 16:26
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants