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

Split logic: public admin and dashboard #238

Merged
merged 4 commits into from
Oct 24, 2017

Conversation

cuducos
Copy link
Collaborator

@cuducos cuducos commented Aug 19, 2017

This PR refactor the architecture underneath the dashboard. Basically what we had so far was two files (jarbas/dashboard/sites.py and jarbas/dashboard/admin.py) that handled two different logics:

  • They had the logic to transform the Django Admin in a read-only login-less dashboard
  • And they also had the logic of the dashboard views (which columns to show, how to parse details, list filters etc.)

This PR splits this logic in two apps:

  • jarbas/public_admin app: logic to make Django Admin public and login-less
  • jarbas/dashboard app: logic for the views of Jarbas Dashboard

The intention is to extract as much logic as possible related public_admin app in order to have it as a general purpose, external, plugable app (maybe even installable via pip).

Public Admin app: logic to make Django Adminpublic and loginless

Dashboard app: logic for the views of Jarbas Admin
@cuducos cuducos changed the title Separate logic: public admin and dashboard Split logic: public admin and dashboard Aug 19, 2017
@jtemporal
Copy link
Collaborator

jtemporal commented Aug 28, 2017

Hey @cuducos could you help me out a little here? I'm hoping for some idea on how to test this functionality-wise. Any ideas?

( I already ran locally and everything seems okay with the usual testing)

Copy link
Collaborator

@jtemporal jtemporal left a comment

Choose a reason for hiding this comment

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

Mostly 🎉 that isort pointed out and we may want to change

@@ -5,15 +5,15 @@
from django.contrib.contenttypes.models import ContentType
from django.test import TestCase

from jarbas.dashboard.sites import DashboardSite, DummyUser, dashboard
from jarbas.public_admin.sites import PublicAdminSite, DummyUser, public_admin
Copy link
Collaborator

Choose a reason for hiding this comment

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

🎉 shouldn't this import line be from jarbas.public_admin.sites import DummyUser, PublicAdminSite, public_admin?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed in 94d8f94

@@ -1,6 +1,6 @@
from django.conf.urls import url
from jarbas.dashboard.sites import dashboard
from jarbas.public_admin.sites import public_admin
Copy link
Collaborator

Choose a reason for hiding this comment

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

🎉 shouldn't this and line 1 be separated by one blank line?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed in 94d8f94

@cuducos
Copy link
Collaborator Author

cuducos commented Aug 29, 2017

I'm hoping for some idea on how to test this functionality-wise. Any ideas?

Actually this part of Jarbas is not tested in depth within Jarbas (mostly because it's mostly Django configuration). However in terms of the authentication there some tests in the dashboard and in the new public_admin — these suites makes sure, for exemple, no POST request is allowed and some URLs are dropped (URL with edit, delete, login etc.).

In terms of UI/UX there are actually no tests (this is the part that the code is pretty much declarative, merely Django Admin configuration). Maybe integration tests with someting like PhantomJS could be written in the future.

That said this PR adds zero new functionality (it's an architectural change): therefore if running the dashboard still working fine it works.

Is there anything else you had in mind when you mentioned functionality-wise?

@cuducos
Copy link
Collaborator Author

cuducos commented Sep 28, 2017

@jtemporal after 94d8f94 is there anything left from code review? LMK because I'm afraid this branch will soon end up in conflicts regarding #252

@cuducos cuducos dismissed jtemporal’s stale review October 23, 2017 23:39

As we talked in PVT I'll free you from this code review… I'm passing it to Ana ; )

@cuducos cuducos requested a review from anaschwendler October 23, 2017 23:39
@anaschwendler
Copy link
Collaborator

@jtemporal is free, I can test this PR, let me just understand what is being made:

  1. Clone the repository:
$ git clone [email protected]:datasciencebr/jarbas.git
  1. Open the repo folder:
$ cd jarbas
  1. Checkout to @cuducos branch:
$ git checkout -b cuducos-public-admin-architecture-refactor origin/cuducos-public-admin-architecture-refactor
  1. Update the branch:
$ git merge master
  1. Copy the .env file:
$ cp contrib/.env.sample env
  1. Build and start services:
$ docker-compose up -d
  1. Create the database and apply the migrations:
$ docker-compose run --rm django python manage.py migrate
$ docker-compose run --rm django python manage.py searchvector
  1. Seeding it with sample data:
$ docker-compose run --rm django python manage.py reimbursements /mnt/data/reimbursements_sample.xz
$ docker-compose run --rm django python manage.py companies /mnt/data/companies_sample.xz
$ docker-compose run --rm django python manage.py suspicions /mnt/data/suspicions_sample.xz
$ docker-compose run --rm django python manage.py tweets
  1. That PR split things, as @cuducos said, so besides accessing localhost:8000/dashboard, and checking if everything is fine, don't think anything else to test, for me it looks good :)

@anaschwendler anaschwendler merged commit dc0f5bc into master Oct 24, 2017
@anaschwendler anaschwendler deleted the cuducos-public-admin-architecture-refactor branch October 24, 2017 17:40
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants