Backend Api mocks #16

dpordomingo · 2018-01-18T15:27:53Z

partially solves #3

This PR will create a backend api serving mocks, using the requirements defined by #3

If approved, the things that will be done in the next PRs will be:

using DB stuff,
tests,
improve error handling and error messages,
using user from Auth

carlosms · 2018-01-18T16:19:41Z

server/cmd/code-annotation/serializer/serializers.go

+	return nil
+}
+
+type Identifable interface {


typo? identifiable

ouch! many thanks

bzz · 2018-01-19T10:02:14Z

Nice!

A bit confused though - what is the reason to have all the code under /cmd/ package?

Usually, it is reserved for executable things, like server of CLI, etc. (see borges/cli). Everything else could belong to a better-named package under /server/.

smacker · 2018-01-19T10:52:18Z

Could you please not merge it yet. I want to take a look. I'll do it a bit later today. Thanks.

dpordomingo · 2018-01-19T11:41:28Z

Sure!
I think it's a good idea to avoid merging things before they're fully reviewed by all of us.

smacker

Good job. But I have comments. Some of them are actually applicable to many similar lines, but I didn't copy-past them to avoid bloating of PR.

smacker · 2018-01-19T11:17:40Z

server/cmd/code-annotation/handler/assignments.go

+	"github.com/go-chi/chi"
+	"github.com/go-chi/render"
+
+	"github.com/src-d/code-annotation/server/cmd/code-annotation/serializer"


do you think it's better to move it out of cmd? It's a common pattern in go to keep only entry point in cmd folder.
Please let me know if you need more information about it.

Sure!!!
That aligns quite well with Alex comment
I tried to follow the initial skeleton and I might misunderstand it.
I'd propose the following #16 (comment)

smacker · 2018-01-19T11:21:30Z

server/cmd/code-annotation/handler/assignments.go

+	"github.com/src-d/code-annotation/server/repository"
+)
+
+func HandleGetAssignmentsForUserExperiment(w http.ResponseWriter, r *http.Request) {


do you think we could follow https://github.com/golang/go/wiki/CodeReviewComments#package-names here?
The package has name handler. So I would suggest handler.GetAssignmentsByUser. I think we don't really need experiment in the name because looks like assignments always belong to experiment. Feel free to suggest better name!

I'd sugest handler.HandleInHandlerPackageThatGetAssignmentsByUserExperiment

You're totally right.

I only disagree in the Experiment suffix, because there will be many experiments; that's because handled URL is /experiments/{experimentId}/assignments.

smacker · 2018-01-19T11:23:52Z

server/cmd/code-annotation/handler/assignments.go

+func HandleGetAssignmentsForUserExperiment(w http.ResponseWriter, r *http.Request) {
+	experimentID, err := strconv.Atoi(chi.URLParam(r, "experimentId"))
+	if err != nil {
+		render.Render(w, r, serializer.NewErrors(err))


do you think we can provide better error message here? Currently, it's strconv.Atoi: parsing "qwe": invalid syntax. I would suggest add information that we can't parse experimentId not a random string.

sure thing!
That's because I left as "pending" in the PR desc:

improve error handling and error messages

I just tried to postpone the error messages for the next iteration, so it wouldn't block the API.
If you agree, we'd merge without this "feature".

smacker · 2018-01-19T11:26:02Z

server/cmd/code-annotation/handler/assignments.go

+		return
+	}
+
+	assignments, err := repository.GetAssignmentsFor(userID, experimentID)


As I remember here we create all assignments. How do you think is it better to put creation logic here or in a service? And keep repository as a slim interface to DB?

I was thinking in:

assignments, err := repository.GetAssignmentsFor(userID, experimentID) if err.is(NO_ASSIGNMENTS_INITIALIZED) { assignments, err = repository.CreateAssignmentsFor(userID, experimentID) }

I agree on putting business logic in service, instead of in repository... but should we create a new service package for only these 3 lines that could be in the handler.GetAssignmentsForUserExperiment ?

I would put it in handler as you posted snippet above for now. Later if we have more cases we can refactor it to service.

smacker · 2018-01-19T11:28:43Z

server/cmd/code-annotation/handler/assignments.go

+		return
+	}
+
+	var assignment serializer.Assignment


I usually try to keep structures close to the place where they are used (in the same package and as a private struct) if they are used only once. What is your opinion about it?

I think that handlers and serializers should be separated.

could you please explain why? How are you going to use serializers beside in handlers? Because in Go usually, you export functions/types if you consider them as your API.
And it's a good idea to keep your API as small as possible.

Packages also help us to choose good names.

If we now care about the exposed API, it's because we're taking care of our external users, but this repo does not contain a library to be used for third parties but a repo containing a bunch of things like a web app, tools for DB handling, an API...

If we still think that a curated external API is needed here, we could move that internal stuff to internal packages and keep the benefits of both sides.

I still don't like to have a package for only 1 particular consumer. But I can live with it.

smacker · 2018-01-19T11:38:39Z

server/cmd/code-annotation/handler/assignments.go

+		return
+	}
+
+	//TODO: use chi logger


can we use logrus?

I'd use chi default through chi/middleware/Logger, whose default seems to be STDOUT and STDERR, but also lets us use Logrus or others.

I'd prefer to consider the Logging lib to use in a separate issue since we're using three big logging libraries in different of our biggest projects

log → go-git...

github.com/inconshreveable/log15 → borges, rovers, framework...

github.com/sirupsen/logrus → go-compose-installer

chi/middleware/Logger the name of the package says you it's not supposed to be used outside of middleware. There is no problem to use middleware with logrus: https://github.com/go-chi/chi/blob/master/_examples/logging/main.go

log15 is also fine for me, can we votelogrus vslog15 then?
for go-git using standard log makes sense. it's library, not application.

I didn't test it yet, but if I understood it the Logger can be retrieved (and used) from the inside of the handlers;

type printfer interface{ func Printf(format string, v ...interface{}) }

and then

logger := r.Context().Value(middleware.LogEntryCtxKey).(printfer) logger.Printf("whatever")

About the log library to use, I think we should consider it as a team decision, considering the company first rule; I mean, imho we should use the same logger in all our projects, and it should be the one that is "the most wanted" by engineering. What do you think about that decision @bzz ?

Let's use Logrus for consistency.

That's what I was trying to find: Choose one log library for Go and implement on all the projects, many thanks for pointing it out !!

smacker · 2018-01-19T11:47:11Z

server/cmd/code-annotation/serializer/serializers.go

+import (
+	"net/http"
+
+	"github.com/go-chi/render"


render package is quite complex. The main advantage of it, you can handle different types of input/output. Like json/xml for example. But I'm not sure we really need it in this project.
Are you open to considering simpler solution?

It was quite easy to implement in our case, but I'd reconsider it for sure, and ping you when done 🗡️ Thanks for being nitpicker !!!

smacker · 2018-01-19T11:50:16Z

server/cmd/code-annotation/serializer/serializers.go

+}
+
+type ResponseMany struct {
+	Data []Identifiable `json:"data"`


do you think it's possible we will render something that doesn't have id?
I personally prefer to add constraints only when they are really necessary.
please consider this comment only as an open discussion, not action request.

You may be right. I'll reconsider it and I'll ping you with the answer. Thanks for suggesting it!

I don't advocate for this particular code, just another way how to do it for you to consider: #22

I also find empty return approach error prone. In my previous project, I myself fixed like 5 bugs about it before we moved to non-empty return. And reviewed about 5 more fixed by other developers. I mean bugs that we found in production, not during CR.

I agree with the 1st paragraph.

I'm lost in "empty return approach". I could not find any occurrence of empty word in this PR. Could you give me a clue?

func Handler(w, r) { if err != nil { write(w) return // empty return } write(w) }

I think what Maxim means is that it's a good and less error-prone to always return something like i.e https://github.com/src-d/code-annotation/pull/22/files#diff-3e10846b2816b527e6724efedecbb964R15

As a global assertion, I agree with it... but in the context of chi handlers, the expected ones must have the signature http.HandlerFunc, that is "empty return"
In the code you linked, the empty return handler is deeper, but it is

If you're talking about status, errors and so on, I agree, but it's not related to the "empty return" funcs

it's not chi handlers. It's net/http handlers. Of course there is empty return on top level. But I can't imagine how you can make mistake there. Everything inside won't compile without proper return.

chi.Router.Get(pattern string, h http.HandlerFunc) requires a http.HandlerFunc (an empty return func). It does not matter what you put as h, that it'll be an empty return func.

Again: I agree on the status, errors and so on, but it's a different thing that I already confirmed it will be added to the server.

smacker · 2018-01-19T11:53:39Z

server/model/models.go

@@ -0,0 +1,54 @@
+package model
+
+type User struct {


we defined this model in schema task, but it doesn't have all necessary field. For example avatar url.
I would also like to abstract from github in model. login and username fields are better in my opinion. Then we can add other providers later. Google for example. Or allow registering user. If you use abstract model consumers of model don't need to know how user was created.
Please share your thought!

I just tried to follow our specs because I was told that other log-in providers are not expected in the middle term. (We even considered using the gh-user-name as the User.ID) Anyhow, I'd consider https://github.com/src-d/code-annotation/pull/22/files#diff-c318b873cebb799af6f409abe8089672 for sure.

About storing the avatar... I'd like to confirm it with @src-d/product, because in our former web apps they requested us to do not store it, but loading it from gh (so it would be always updated) htps://avatars2.githubusercontent.com/u/{gh-user-id}

To do so, let us know if you can retrieve the gh-user-id from GH, or if we should store it in the DB.

Github return https://avatars0.githubusercontent.com/u/406916?v=4 as avatar url. I suggest to store it.

It's a business decision.

@bzz should we keep the user avatar in sync with the real one?

in sync: showing whatever avatar GH shows under htps://avatars2.githubusercontent.com/u/{gh-user-id}

outdated: showing a stored version when the user first logged in

It's totally fine, either not to have avatar at all, or to show an outdated on.

On the side-note: personally, I would prefer us merging and improving already working things in subsequent small PRs rather then arguing about abstract things in single long PR.

406916 is a userId, so the image itself is always fresh.

smacker · 2018-01-19T11:58:59Z

server/model/models.go

+type Answer string
+
+const (
+	Yes   Answer = "yes"


On one hand yes, no, maybe sounds common enough. On another hand, I'm not 100% sure it's the best options for our tool.
I remember you spent time trying other tools. Could you please remind me what do they use as names for action?
It's very basic part of the tool, I just want to do it right. Thanks!

That values should not be exposed in the UI, never-ever.

TL;DR
(this answer is based in #2 (comment) but feel free to keep answering me if the conclusions below does not satisfy you)

MTurk

In MTurk, the Assignment tracks the Answer of a HIT, as we do, but the Answer of an Assignment is not defined by the platform itself but by the Requester.

In MTurk the Answer schema is defined by the Project itself,
An MTurk Answer schema could be whatever nested thing, a quite simple could be:
{name:string, age:int, gender:bool, desc:string, croppedImage:[]blob}
but we don't have that concept because we were requested:

to use a single Project type, whose Answer schema would be: {answer:int[1-4]},

to avoid unnecessary normalization;

because of that requirements, we decide by #2 (comment) :

to have Experiment instead of Project,

to avoid Answer model, and just embed it in the Assignment

Considering that our accepted answers are an enum of size 4, we just use that "strings" instead of using int[1-4].
Because of that:

the internal value of the Assignment.Answer should not be exposed to the UI.

the Front should have a mapping ( DB/API ⇄ UI ) between the enum values used in the DB/API, and labels that the user will see.

Supervise.ly

Supervise.ly focuses on image labeling (so they does not have Projects neither) and their answers are class oriented. Because of the same reason than in MTurk, the class names are always defined by the Requester, and they can be whatever random word or group of chars (example: Car, Dog, Cake, UFA4E4AS...

yeah. It's only internal name. But I still want to have it "correct".
Looks like yes, no, maybe, skip are ok if we consider it as enum 🎉
Good to go!

dpordomingo · 2018-01-19T12:03:45Z

answering @bzz question #16 (comment)
I tried to follow the initial skeleton but you're right, I'll ping you when it had been reordered.
It would be:

/
  public # web stuff
  src # web sources
  cli
    import/import.go
    export/export.go
    server/server.go
    examples/import/example.sql # it may be moved to of /cli/import/examples/example.sql
  server
    handler
    serializer
    model
    repository
    dbutil # it may be moved outside of /server (to the root path)

Comment with 👍 if you agree, or provide a new suggestion 😉

bzz · 2018-01-19T15:10:13Z

@smacker could you summarize only the main points you see blocking us from merging this PR in a check-list form?

Feel free to update this one:

change package structure a bit
add Logrus for logging
reconsider how we write handlers. (kill empty returns)
better error handing

I think that would help us to decide with the order of merging things, i.e this, then #22 or other way round.

Update: was updated by Maxim.

@dpordomingo does it make sense to you, to fix this asap and merge then?

smacker · 2018-01-23T13:13:55Z

cli/server/server.go

+	corsOptions := cors.Options{
+		AllowedOrigins: []string{"*"},
+		AllowedMethods: []string{"GET", "POST", "PUT", "OPTIONS"},
+		AllowedHeaders: []string{"Location", "Authorization"},


we also need content-type header

addressed by 28c54c4

smacker · 2018-01-23T13:15:56Z

cli/server/server.go

-	// protected handlers
-	r.Route("/api", func(r chi.Router) {
-		r.Use(jwt.Middleware)
+	r.Route("/experiments/{experimentId}", func(r chi.Router) {


should be inside api and protected

addressed by 28c54c4

smacker · 2018-01-23T13:16:37Z

cli/server/server.go

+		r.Get("/me", handler.Me(userRepo))
 	})

 	r.Get("/login", handler.Login(oauth))


Recoverer and logger middlewares must be used for all routers

addressed by 28c54c4

smacker · 2018-01-23T13:18:57Z

server/handler/auth.go


 // OAuthCallback makes exchange with oauth provider, gets&creates user and redirects to index page with JWT token
 func OAuthCallback(oAuth *service.OAuth, jwt *service.JWT, userRepo *repository.Users, uiDomain string) http.HandlerFunc {
+	logger := service.NewLogger()


should it be a dependency? for example in tests you want to use another logger

I started making the logger injectable/configurable, so you could define which one to use in different scenarios.
But I rollbacked to remove initial complexity.
Do we want that for this first iteration?

where is a complexity to change

func OAuthCallback() { logger := service.NewLogger() }

to

func OAuthCallback(logger *logrus.FieldLogger) { }

?

It is now:

func OAuthCallback(oAuth *service.OAuth, jwt *service.JWT, userRepo *repository.Users, uiDomain string) http.HandlerFunc {

I didn't want to add more params to some handlers, but I can do so for sure.
I saw that chi/logger uses the context to store the logger, what could be another approach.
Which one would be your option?

a: one parammeter more,

b: using the context as chi does

the easiest way to just add one more param. The better way is to convert it to structure.
for logger it's okay to store it in context (but you will need a helper, which returns "default" logger if there is no logger in context), especially if you want to have request scoped logger (and I want).

But remember Context.Value should inform, not control. Here is a good article about it: https://medium.com/@cep21/how-to-correctly-use-context-context-in-go-1-7-8f2c0fafdf39

so a, and lets proceed with b in the next iteration

addressed by 28c54c4

smacker · 2018-01-23T13:20:10Z

server/middleware/appender.go

+	return append(ms, Appender)
+}
+
+func Appender(next http.Handler) http.Handler {


we definitely need comments here. It isn't obvious what Appender does

Yes, you're right.
And the name is not clear at all.
Options:

now,

merge and document.

Anyhow, I planned to add it to the login/callback so all errors, logging... will be homogeneous

I think we need to document it now. Because I don't understand it completely even for review.
Like what would happen if I use w as a normal writer. Looks like it will just fail even if you implement http.ResponseWriter methods instead of current panic.
Is it normal behavior? Is it a bug?
Without documentation I don't know.

If you're using that middleware, the w you receive won't accept writes.
I'm writing a couple of lines with doc; I'll ping you in a few mins when ready :D

then it's completely incorrect code.
handler accepts http.ResponseWriter. But it isn't http.ResponseWriter anymore.

functionality dropped

smacker · 2018-01-23T13:22:20Z

server/handler/experiments.go

+
+	experimentID, err := strconv.Atoi(chi.URLParam(r, "experimentId"))
+	if err != nil {
+		postponed.AddError(&serializer.Error{500, "Wrong format in experiment ID sent", err.Error()})


http.StatusInternalError

Yes, I planned to review all errors codes and texts to use constants and make them more clear.
Should I do it before merging? Just say so 💃

it's a very simple change. Why postpone?

I didn't want 500 for everything because there could be any other with more sense. But yes, that enhanced can be postponed too.
Adding http constants now

smacker · 2018-01-23T13:22:30Z

server/handler/experiments.go

+
+	experiment, err := repository.GetExperimentById(experimentID)
+	if err != nil {
+		postponed.AddError(&serializer.Error{404, "No experiment found", err.Error()})


http.StatusNotFound

smacker · 2018-01-23T13:27:57Z

server/handler/assignments.go

+
+func GetAssignmentsForUserExperiment(w http.ResponseWriter, r *http.Request) {
+	//TODO: !ok
+	postponed, _ := w.(middleware.PostponedResponse)


what is the point to "postpone" response? I checked handlers and I don't see where you might need it. Every time you return you can create normal response from the line just above that return.

the postponed Responses are the way to avoid writing things randomly; that way the handlers push the results to the postponed Response, and at the end of the flow it is processed and properly serialized, the errors are checked...

you are talking about transformation. Not postponing.

Postponing is something like

func handler(w) { writeSomethingTo(w) // do long computation maybe in goroutine writeMoreTo(w) }

The Response is written after the handler is run

I get it. I'm saying the name is confusing. If you want to keep using this approach (I don't) the name like Responser is much better. Because it's interface and consumer doesn't need to know how it works internally (does it postpone anything or not)

functionality dropped

smacker · 2018-01-23T13:33:24Z

server/middleware/appender.go

+	})
+}
+
+type postponed struct {


wrapping http.ResponseWriter isn't such an easy task. You can read about it more here for example.

I tried to emulate the same flow that chi/middleware/compress and chi/middleware/logger follows. Both wrap it to ensure the response is created in a controlled and homogeneous way.

Thanks for the resource, I'll read a bit more to consider unhandled circumstances.

functionality dropped

dpordomingo · 2018-01-23T21:38:27Z

Done, here is the summary:

change package structure a bit as agreed Backend Api mocks #16 (comment)
add Logrus for logging 2400963
reconsider how we write handlers and error handing 28c54c4

I'll review the constants for status because there could be some missing

dpordomingo · 2018-01-23T21:39:16Z

PS:
Many thanks to all of you for being nitpicking 🗡️

smacker · 2018-01-23T21:46:19Z

cli/server/server.go

+		middleware.Recoverer,
+		cors.New(corsOptions).Handler,
+		middleware.RequestLogger(&middleware.DefaultLogFormatter{Logger: logger}),
+	}


better write as in documentation:

r := chi.NewRouter() r.Use(middleware.Recoverer) r.Use(cors.New(corsOptions).Handler) r.Use(middleware.RequestLogger(&middleware.DefaultLogFormatter{Logger: logger})) r.Get("/login", GetHandler(handler.Login(oauth))) r.Get("/oauth-callback", GetHandler(handler.OAuthCallback(oauth, jwt, userRepo, conf.UIDomain, logger))) r.Route("/api", func(r chi.Router) { r.Use(jwt.Middleware) ... })

not because current code is wrong, but because it isn't that easy to understand.
Like I need to scan all code below this line for ms.

If you want to exclude static (though I don't see why, I think it should be logged also and FrontendStatics may panic also). You can wrap it in another r.Group. But r.Use, r.Use is much more readable than array.

smacker · 2018-01-23T21:54:23Z

server/handler/assignments.go

+)
+
+func GetAssignmentsForUserExperiment() RequestProcessFunc {
+	return func(r *http.Request) (resp *serializer.Response, errs []*serializer.Error) {


https://github.com/golang/go/wiki/CodeReviewComments#named-result-parameters

also you lose all the profit of go type checking.

consider code:

func(r *http.Request) (resp *serializer.Response, errs []*serializer.Error) { ...a lot of code... if somecheck { resp = postResponse() } ...a lot of code... resp = getResponse() return }

code above compiles and produces wrong response.

this signature and always return new value avoids the problem:

func(r *http.Request) (*serializer.Response, []*serializer.Error) {

smacker · 2018-01-23T21:58:26Z

server/service/jwt.go

 	"github.com/src-d/code-annotation/server/model"

-	jwt "github.com/dgrijalva/jwt-go"
+	"github.com/dgrijalva/jwt-go"


I'm sure my editor will fix it back as soon as I open this file.
I'm too lazy to look for a link right now, but it's good practice to use named import if name of package != import path.
With jwt-go it's not that bad. But sometimes names are much different and it's just difficult to understand where this package came from.

smacker · 2018-01-23T22:01:51Z

server/handler/auth.go

+	return func(r *http.Request) (resp *serializer.Response, errs []*serializer.Error) {
 		url := oAuth.MakeAuthURL()
-		http.Redirect(w, r, url, http.StatusTemporaryRedirect)
+		resp = serializer.NewTemporalRedirect(url)


what is the point of using multiple layers of logic for it if you actually just do http.Redirect anyway?

smacker · 2018-01-23T22:05:00Z

server/handler/renderer.go

+	status := http.StatusOK
+
+	if len(errs) > 0 {
+		status = http.StatusInternalServerError


something is wrong here.
if handler returns error not found the status should be not found not internal server error

smacker · 2018-01-23T22:06:45Z

server/handler/renderer.go

+		status = http.StatusInternalServerError
+		response = &serializer.Response{Status: status, Errors: errs}
+		for _, e := range errs {
+			//TODO: Not all errors should be logged as .Error()


4xx statuses are client errors, you don't need to log them
5xx statuses are server errors, we need to log them

smacker · 2018-01-23T22:14:56Z

server/handler/static_files.go

+	filepath := "build" + r.URL.Path
+	if _, err := os.Stat(filepath); err == nil {
+		http.ServeFile(w, r, filepath)
+	}


and here we go. empty return = mistake.

it was easy to spot in a handler with 5 lines of code.
But in with more code it's' more difficult.

That's why I'm so aggressively against of empty return when we don't have to use them.

The problem was caused because the return from the original sourced dissapeared (:unamused:) when the original code was moved to the handler package.

I don't see how could it be avoided by a qualified return; example, given the following code:

func () string { if (cond) { doSomething() return "true" } return "false" }

the one above will compile too, and has the same problem.

func () string { if (cond) { doSomething() } return "false" }

func FrontendStatics(w http.ResponseWriter, r *http.Request) { http.ServeFile(w, r, func() string { filepath := "build" + r.URL.Path if _, err := os.Stat(filepath); err == nil { // you can't write filepath without return, it won't compile return filepath } return "build/index.html" }()) }

but I don't suggest to change this handler. It's small, so no need to do it more complex. But for other handlers it makes sense.

P.S. Of course you can always make a mistake, just the chance is lower with non-empty returns.

smacker · 2018-01-23T22:31:25Z

server/handler/assignments.go

+	return func(r *http.Request) (resp *serializer.Response, errs []*serializer.Error) {
+		experimentID, err := strconv.Atoi(chi.URLParam(r, "experimentId"))
+		if err != nil {
+			errs = append(errs, &serializer.Error{500, "Wrong format in experiment ID sent", err.Error()})


sorry I don't remember in which document it's described. But go vet won't allow this code with warning like composite literal uses unkeyed fields https://golang.org/cmd/vet/#hdr-Unkeyed_composite_literals

And it makes a lot of sense!
If you change the order of fields (last 2) in your struct - code still compiles but works incorrectly.
Also it will be very painful to add a new optional field.

smacker · 2018-01-23T22:42:51Z

server/repository/repositories.go

+	return assignments, nil
+}
+
+func UpdateAssignment(assignmentId int, assignment *model.Assignment) error {


looks like you missed my comment before.
What behavior should I expect from this method if assignmentId != assignment.ID?
I don't have a correct answer.
That's why I suggested to change func signature.

From the specs #3 (comment), the assignment request comes with no ID, so it is not considered
Same happens with answer.PairID: it is ignored.
It will be ensured in the tests too.

Anyhow, I'll rollback to

func UpdateAssignment(assignmentId int, answer string, duration int) error

That is more clear and guarantee our requirements by design

we need to add id to an assignment. Because FE needs to update it. RESTful way to do it is to call PUT .../assignments/{id}

smacker · 2018-01-23T22:49:38Z

server/serializer/serializers.go

+	Description string `json:"description"`
+}
+
+func NewExperimentResponse(e *model.Experiment) *Response {


you just broke name convention.
If a package has public ExperimentResponse structure, NewExperimentResponse should return it. Not Response.
ReturningResponse will make sense if ExperimentResponse is private.

dpordomingo · 2018-01-26T18:17:07Z

superseded by #37

dpordomingo requested review from carlosms and smacker January 18, 2018 15:27

bzz added the review label Jan 18, 2018

carlosms reviewed Jan 18, 2018

View reviewed changes

carlosms approved these changes Jan 18, 2018

View reviewed changes

smacker suggested changes Jan 19, 2018

View reviewed changes

bzz removed the review label Jan 19, 2018

smacker mentioned this pull request Jan 19, 2018

Authorization #22

Merged

dpordomingo mentioned this pull request Jan 20, 2018

Schema design #2

Closed

dpordomingo force-pushed the api branch from a0175d2 to 9f9bcc0 Compare January 23, 2018 13:03

dpordomingo added 6 commits January 23, 2018 14:07

Serve mocked data

65df203

Follow Makefile conventions

de762ba

Separate static handlers and use chi/cors

a258691

Save Assignments

1169b81

Use logrus logger in API

2400963

Clean and reorder imports

485c16b

dpordomingo force-pushed the api branch from 9f9bcc0 to d26dc9c Compare January 23, 2018 13:11

smacker reviewed Jan 23, 2018

View reviewed changes

Enhance error messages and JSON outputs

28c54c4

dpordomingo force-pushed the api branch from d26dc9c to 28c54c4 Compare January 23, 2018 21:34

smacker reviewed Jan 23, 2018

View reviewed changes

dpordomingo added 5 commits January 24, 2018 12:11

Clean middlewares

24d54e6

Make repository.UpdateAssignment more explicit

f79ba41

Extract router to a function, so it would be easily tested

d29f628

Serve /index.html for non /static/* requests

351ca7f

intermediate steep

ca149e9

dpordomingo assigned dpordomingo and unassigned dpordomingo Jan 26, 2018

dpordomingo mentioned this pull request Jan 26, 2018

Prepare Router skeleton #37

Merged

dpordomingo closed this Jan 26, 2018

dpordomingo reopened this Jan 30, 2018

dpordomingo closed this Jan 30, 2018

dpordomingo mentioned this pull request Jan 30, 2018

Implement handlers #49

Merged

bzz mentioned this pull request Feb 6, 2018

Use DB in the backend #52

Merged

2 tasks

dpordomingo deleted the api branch February 9, 2018 18:28

Backend Api mocks #16

Backend Api mocks #16

Uh oh!

Conversation

dpordomingo commented Jan 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bzz commented Jan 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

smacker commented Jan 19, 2018

Uh oh!

dpordomingo commented Jan 19, 2018

Uh oh!

smacker left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

dpordomingo commented Jan 18, 2018 •

edited

Loading

bzz commented Jan 19, 2018 •

edited

Loading

bzz Jan 19, 2018 •

edited

Loading