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

[WIP] Feature/facebook login #6

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

larribas
Copy link
Contributor

No description provided.

Copy link

@groyoh groyoh left a comment

Choose a reason for hiding this comment

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

Looks good even though I don't get everything at the moment.


// A trip represents a one-time visit to a particular country
type Trip struct {
Id string `json:"id"`
User string `json:"user"`
Place string `json:"place"`
Country string `json:"country"`
Copy link

Choose a reason for hiding this comment

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

This should probably be a Country struct at some point.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree. Everything you see here is very WIP at the moment. Our idea was to use an external API for countries, but I'm not sure yet which options we would have

)

const userConfFilename string = "conf.json"

// TODO: Move this to API
func Ping(w http.ResponseWriter, r *http.Request, ps httprouter.Params) {
fmt.Fprint(w, "pong")
Copy link

Choose a reason for hiding this comment

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

Nice 😁

@@ -0,0 +1,8 @@
package models
Copy link

Choose a reason for hiding this comment

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

I'd rather move this into an input/api validator rather than a models validator. I'd even put it into a parser module and return the actual Time rather than using a string within the model.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good :)

"io/ioutil"
)

func Login(w http.ResponseWriter, r *http.Request, ps httprouter.Params, a *config.Config) {
Copy link

Choose a reason for hiding this comment

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

I'm not sure I understand this endpoint. Could you detail it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Our idea is to allow 3rd-party login via Facebook, right? This endpoint is used by the client to request the user to log in. It redirects the user to a Facebook login page with our app's information. When the user clicks on "accept" or "reject", Facebook calls the LoginWithFacebook callback with an access token corresponding to the user, and then we can get their data (email, name, ...)

One of the main ideas of this is that in the future maybe we can parse the metadata of pictures to create an interactive map of where they've been and with whom.

@@ -0,0 +1,50 @@
package trip_repo
Copy link

Choose a reason for hiding this comment

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

AFAIK it's not recommended to use _ within package name, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True. It's not very Go-y. What package structure and names would you use?

Copy link

Choose a reason for hiding this comment

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

Following go coding style I'd say triprepo


repo.trips[trip.Id] = trip
return nil
}
Copy link

Choose a reason for hiding this comment

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

There's a new line missing at the end of some of your files.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could be I didn't even go fmt the whole thing

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.

2 participants