Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Define interface for selecting Census regions #222

Merged
merged 5 commits into from
Oct 31, 2019

Conversation

jeancochrane
Copy link
Contributor

@jeancochrane jeancochrane commented Oct 28, 2019

Overview

This PR sketches out the UI for selecting Census regions when creating new CensusAreas. Also, fix a bug where View.get_queryset_for_agency() improperly caches the queryset.

Connects #221, closes #223.

Notes

  • I wanted to get this PR in before actually adding new CensusRegions so that we could figure out what the interface should look like. Because of this, the new view won't really show any new regions -- it should simply illustrate what the user flow will look like, with Philadelphia as the only currently-supported region.

  • Allow user to create CensusAreas for new cities #221 suggests that we might try to create a "shortcut" workflow that will skip the region selection step for power users who only create CensusAreas in one region. I didn't add that as part of this PR yet but I'd be curious to hear if you have thoughts about how we might achieve that -- there doesn't seem to me to be anywhere obvious where it would make sense.

Testing Instructions

  • Run migrations and start dev server
  • Log in and navigate to http://localhost:8000/census-areas/
  • Hit the Create new census area button
  • On the new form view, add a name and agency for your area, and choose Philadelphia from the Region selector
  • Confirm you get redirected to an updated form including the map with census tracts in Philadelphia
  • Go back to http://localhost:8000/census-areas/ and edit an existing CensusArea
  • Confirm that the Region input is disabled

Copy link
Contributor

@beamalsky beamalsky left a comment

Choose a reason for hiding this comment

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

This is looking great. I'm not actually able to create new Philadelphia census areas—did you intend that temporary breakage? In terms of user flow, though, I think this works!

I have two thoughts on storing default region values: I think we could either add a default_region field to the [django-pldp Agency model](https://github.com/datamade/django-pldp/blob/master/pldp/models/agency.py or to the custom User model in this repo. The advantage of attaching it to user is that it would accommodate theoretical multi-regional agencies; the advantage of attaching it to Agency is that region wouldn't have to be set for each individual user. What do you think?

model = survey_models.CensusArea
fields = ['name', 'agency', 'region']

def add_inputs(self, helper):
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah nice!

@jeancochrane
Copy link
Contributor Author

I did a little bit of debugging and was able to figure out the root problem with creating new CensusAreas, which I've captured in #223. Tomorrow I'll add in a fix for it, add Agency.default_region, and also add a little spinner to the CensusRegion select form so the user knows to hang tight.

@jeancochrane
Copy link
Contributor Author

@beamalsky Pushed up a couple of changes -- one commit fixes #223, and the other adds a loading spinner via a modal to give more of a hint that a new page is loading (not a comment you left, just something I noticed as I was testing it out).

I decided not to implement Agency.default_region just yet. After getting sidetracked by #223 I want to make sure we have enough time show Tyler something and get his feedback before we move forward on a feature he didn't approve. Does that sound OK to you?

@beamalsky
Copy link
Contributor

I pulled down and I'm not seeing an Agency checkbox!

Screen Shot 2019-10-30 at 2 58 41 PM

@jeancochrane
Copy link
Contributor Author

@beamalsky It should be below the map! Do you see it there?

Copy link
Contributor

@beamalsky beamalsky left a comment

Choose a reason for hiding this comment

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

Your handling of the agency checkbox makes a lot of sense. LGTM! Holding off on a default_region attribute until we know if it'll be useful sounds like the right move.

@jeancochrane jeancochrane merged commit f92ae88 into master Oct 31, 2019
@jeancochrane jeancochrane deleted the feature/jfc/census-region-selector branch October 31, 2019 20:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AgencyRestrictQuerysetMixin causes object create to fail
2 participants