Skip to content
This repository has been archived by the owner on Sep 2, 2024. It is now read-only.

integrate frontend service with rating service #581

Merged
merged 2 commits into from
Dec 3, 2020

Conversation

minherz
Copy link
Collaborator

@minherz minherz commented Dec 2, 2020

Setup HTML templates to have dynamic values of the rating.
Modify home page and product page handlers to populate product rating(s).
Add functionality to submit a new product rating.

setup HTML templates to have dynamic values of the rating.
modify home page and product page handlers to populate product rating(s).
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Dec 2, 2020
make frontend service not to fail in CI workflow validation by:
- defining RATING_SERVICE_ADDR environment variable
- making sure that failed call to rating service does not fail
  GET / request to frontend.
@minherz
Copy link
Collaborator Author

minherz commented Dec 2, 2020

If you want to try the integration, you can access a test environment using http://35.225.4.62/.
I will shutdown the environment after the review process is over.

Copy link
Member

@daniel-sanche daniel-sanche left a comment

Choose a reason for hiding this comment

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

I'm not too familiar with go, but overall this looks good to me. Left a couple small comments

}
return nil
} else {
return errors.Errorf("unhandled response code %q", result.StatusCode)
Copy link
Member

Choose a reason for hiding this comment

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

I'd suggest treating any 2xx (or possibly 3xx) as success if there's no error

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

A question is what would be the right behavior regarding payload processing. Should I try to read and parse the payload the same way for all 2xx? Since many 3xx statuses imply redirect, should the code support?
WDYT to handle all 2xx responses by trying to parse the response and to keep 3xx handled the same way?

.github/workflows/ci.yml Show resolved Hide resolved
renderHTTPError(log, r, w, errors.New("product id not specified"), http.StatusBadRequest)
return
}
rating, _ := strconv.ParseInt(r.FormValue("rating"), 10, 32)

Choose a reason for hiding this comment

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

what is the meaning of 10 and 32 here? Perhaps, worth adding a comment if GO doesn't support named parameters.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I just copy/pasted it from another method in this file:

func (fe *frontendServer) addToCartHandler(w http.ResponseWriter, r *http.Request) {
log := r.Context().Value(ctxKeyLog{}).(logrus.FieldLogger)
quantity, _ := strconv.ParseUint(r.FormValue("quantity"), 10, 32)

Looked for this in the internet and it seems to be a usual practice.

Should we deal with this?

src/frontend/rest.go Show resolved Hide resolved
src/frontend/rest.go Show resolved Hide resolved
@minherz minherz merged commit dd5dcf1 into master Dec 3, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants