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

Login success #4

Closed
wants to merge 10 commits into from
Closed

Login success #4

wants to merge 10 commits into from

Conversation

dbalatero
Copy link
Collaborator

  • Got the development db up and running
  • Login/signup/forgot password point back at the Rails app
  • Fixed all the errors I was getting in my JS console
  • Markers correctly update
  • Location types preload

# Development
auth_host = "http://localhost:3000"
api_host = "http://localhost:3100/api/0.2"
if document.URL.indexOf("http://localhost:9001/") > -1
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

maybe prefer indexOf("http://localhost") and don't worry about the port? the less hardcoded the port the better

@dbalatero
Copy link
Collaborator Author

dbalatero commented Feb 8, 2017

@ezwelty Thanks for catching those location fields. It's hard to see from the code how the API might have changed between Node and Rails, so my strategy so far has been to just use the site and watch how things have changed. I'll maybe start making the API a bit clearer? Reading this method for example is very entangled and hard to parse as an outside dev… even adding paragraph breaks between relevant sections would help readability which might be nice. Also giving the req.query variables clearer names would be good - at first glance I have no idea what req.query.c might do… it appears to filter categories though? Anyways this is just feedback as someone coming into it with less context, and might help future developers that want to assist on the project.

Are we good with merging these changes to master? It will start the move away from the Rails API, which means master will be broken until the API retrofit is done. Alternatively we can maintain a new-api branch to do all this dev work on and merge that to master when it's done. It just depends if you need to cut a new version of the mobile app any time soon.

@dbalatero
Copy link
Collaborator Author

@ezwelty also btw if we're both pushing to the same branch, you can avoid merges usually with git pull --rebase!

@ezwelty
Copy link
Collaborator

ezwelty commented Feb 8, 2017

I'd suggest we keep this in a separate branch, as I might be making design and locale changes while the API transition is ironed out.

I pointed basic location and list functionality to the new locations.json data structure. That should make it easier to detect things that are broken. Here are few things I notice:

types.json

  • "name" is now jus the (en) common name, not the full name "Common name [Scientific name]" (as needed by the type selectors). Such a field can be added client-side to the data once downloaded.

locations.json

  • returns distance (when lat,lng specified) but paginated result is not sorted by distance (used by list). Force sort from server so we don't have to get every page and then sort to find nearest locations?

locations/{id}.json (#3383 at Kalmia & 26th in Boulder is good for testing)

  • missing season_start and season_stop (used by view_location)
  • missing created_at and updated_at (used by view_location)

locations*

  • return both type_ids and type_names. type_names are just common names (in the specified locale), and lack scientific name. Can either discard type_names and use type_ids to build all needed strings from types.json, or if this is too slow for the list view, generate a locale-specific location "title" server-side, as was previously available from the Rails API.

Map vs. List

  • Both should now be switched to using the same API call and data so that locations.json isn't called twice. Should include the type filter as a parameter (as currently done by map) and lat,lng (as currently done by list).

p.s. I had left a commented block for Production endpoints in localhost because it's sometimes useful to point to the Production API.

@dbalatero
Copy link
Collaborator Author

@ezwelty Cool. I think a good strategy for me will be to capture a valid request from the Rails server, switch to the Node server, and then diff the two responses. This will show me what fields might differ. I can make Trello cards for the things you pointed out above ^

@ezwelty
Copy link
Collaborator

ezwelty commented Feb 8, 2017

@dbalatero Clever. And I agree with the need for better documentation of the NodeJS API. I'll let you rename this branch to new-api and keep hacking?

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.

None yet

2 participants