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

myst-177 proposal list #76

Merged
merged 6 commits into from Jan 2, 2018

Conversation

Projects
None yet
5 participants
@shroomist
Copy link
Contributor

commented Dec 29, 2017

No description provided.

@@ -22,8 +22,8 @@ func NewProposalsEndpoint(mc server.Client) *proposalsEndpoint {

func (pe *proposalsEndpoint) List(resp http.ResponseWriter, req *http.Request, params httprouter.Params) {

nodeKey := params.ByName("nodeKey")
proposals, err := pe.mysteriumClient.FindProposals(nodeKey)
nodeId := req.URL.Query().Get("nodeid")

This comment has been minimized.

Copy link
@donce

donce Dec 29, 2017

Contributor

node_id?

This comment has been minimized.

Copy link
@shroomist

shroomist Dec 29, 2017

Author Contributor

perhaps yes


type proposalsDto struct {
Proposals []dto_discovery.ServiceProposal `json:"proposals"`
}

This comment has been minimized.

Copy link
@donce

donce Dec 29, 2017

Contributor

This struct is duplicate with server.dto.ProposalsResponse, maybe we can keep only one of those?

This comment has been minimized.

Copy link
@shroomist

shroomist Dec 29, 2017

Author Contributor

your are right, but as of Waldz reqiurement, we want to decouple tequila endpoint structures from internals of app and from discovery.

This comment has been minimized.

Copy link
@tadovas

tadovas Dec 29, 2017

Member

Two different apis should use their own internal serialization/deserialization structures as these are absolutely independent ones - no matter that data is the same at the moment. Agree with @shroomist

This comment has been minimized.

Copy link
@donce

donce Dec 29, 2017

Contributor

Totally agree - I had this idea in mind, just wanted it to be clarified :D


for _, proposal := range client.proposalsMock {
var filterMatched = true
if nodeKey != "" {

This comment has been minimized.

Copy link
@tadovas

tadovas Dec 29, 2017

Member

What about moving this check outside loop and making like this:
if (nodeKey == "") return client.proposalsMock

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 29, 2017

Member

I did not wanted conditional code with&without filter

This comment has been minimized.

Copy link
@tadovas

tadovas Dec 29, 2017

Member

And now you have conditional code with and without filter inside a loop

This comment has been minimized.

Copy link
@shroomist

shroomist Jan 2, 2018

Author Contributor

@tadovas this is client mock, do you think its important to optimize here?

This comment has been minimized.

Copy link
@tadovas

tadovas Jan 2, 2018

Member

Well it's not about optimization, more about readability. And yes you are right - since it's just for testing - no point to do some optimizations here, we can focus more on readability.

This comment has been minimized.

Copy link
@donce

donce Jan 2, 2018

Contributor

Plus, it's extendable - if we add more filters, it will be easy to extend this method by adding more ifs inside loop :)


type proposalsDto struct {
Proposals []dto_discovery.ServiceProposal `json:"proposals"`
}

This comment has been minimized.

Copy link
@tadovas

tadovas Dec 29, 2017

Member

Two different apis should use their own internal serialization/deserialization structures as these are absolutely independent ones - no matter that data is the same at the moment. Agree with @shroomist

utils.SendError(resp, err, http.StatusInternalServerError)
return
}
proposalsDto := proposalsDto{proposals}

This comment has been minimized.

Copy link
@tadovas

tadovas Dec 29, 2017

Member

I don't think it's a good idea to pass json structure as is from discovery service to tequila endpoint response - any change inside discovery service response will automatically break tequila api consumers. What about creating our own ProposalDto and putting only required data? For example node id, proposal number ant some human readable/ decision important info like price etc.

This comment has been minimized.

Copy link
@shroomist

shroomist Dec 29, 2017

Author Contributor

done


func (pe *proposalsEndpoint) List(resp http.ResponseWriter, req *http.Request, params httprouter.Params) {

nodeId := req.URL.Query().Get("nodeid")

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 29, 2017

Member

Lets call it "provider_id".
Our API naming convention until now: nore_register, node_key, not snake case

assert.JSONEq(
t,
`{
"proposals": [`+string(proposalBytes)+`]

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 29, 2017

Member

formatting

}

func TestProposalsEndpoint_List(t *testing.T) {
proposalsSerializable := proposalsDto{proposals}

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 29, 2017

Member

Dont use internal dto of tested function.

This comment has been minimized.

Copy link
@shroomist

shroomist Dec 29, 2017

Author Contributor

fixed


assert.JSONEq(
t,
string(proposalsBytes),

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 29, 2017

Member
{
    "proposals": [
        0: serializeProposal(proposals[0]),
        1: serializeProposal(proposals[1])
    ]
}

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 29, 2017

Member

Or jsut freeze wanted static string of tested endpoint

This comment has been minimized.

Copy link
@shroomist

shroomist Dec 29, 2017

Author Contributor

fixed


assert.JSONEq(
t,
string(proposalsBytes),

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 29, 2017

Member

Or jsut freeze wanted static string of tested endpoint

@shroomist shroomist force-pushed the feature/MYST-177-proposal-list branch from f89f74d to 4a231f2 Dec 29, 2017

)
}

func TestProposalsEndpoint_List(t *testing.T) {

This comment has been minimized.

Copy link
@ignasbernotas

ignasbernotas Jan 2, 2018

Member

Why do we need an underscore in the name? I think we should stick to camelCase convention.

@ignasbernotas
Copy link
Member

left a comment

LGTM

if proposal, ok := client.proposalsByProvider[nodeKey]; ok {
proposals = []dto_discovery.ServiceProposal{proposal}

if nodeKey == "" {

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 2, 2018

Member

Conditionals are evil, because they work differently

Proposals []proposalTeqDto `json:"proposals"`
}

type locationTeqDto struct {

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 2, 2018

Member

Dont split so much tequila in all places

return proposalTeqDto{
Id: p.Id,
ProviderId: p.ProviderId,
ServiceDefinition: serviceDefinitionTeqDto{

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 2, 2018

Member

Add ServiceType also

ProviderId: p.ProviderId,
ServiceDefinition: serviceDefinitionTeqDto{
LocationOriginate: locationTeqDto{
ASN: p.ServiceDefinition.GetLocation().ASN,

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 2, 2018

Member

Where is country and city?

var proposals = []dto_discovery.ServiceProposal{
{
Id: 1,
Format: "service-proposal/v1",

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 2, 2018

Member

Remove fields, unnecessary for your tests: Format, PaymentMethod, PaymentMethodType, ProviderContacts


func (pe *proposalsEndpoint) List(resp http.ResponseWriter, req *http.Request, params httprouter.Params) {

nodeId := req.URL.Query().Get("provider_id")

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 2, 2018

Member

Your started using camelCase naming in Tequilapi, it's OK, just be consistent


func (pe *proposalsEndpoint) List(resp http.ResponseWriter, req *http.Request, params httprouter.Params) {

nodeId := req.URL.Query().Get("provider_id")

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 2, 2018

Member

It's not nodeId anymore ;)

@shroomist shroomist dismissed stale reviews from tadovas and ignasbernotas via 7a72640 Jan 2, 2018

@shroomist

This comment has been minimized.

Copy link
Contributor Author

commented Jan 2, 2018

@Waldz your comments fixed, can you guys approve this once again? @tadovas

@Waldz

Waldz approved these changes Jan 2, 2018

@tadovas

tadovas approved these changes Jan 2, 2018

@shroomist shroomist merged commit a35c6b2 into master Jan 2, 2018

@Waldz Waldz deleted the feature/MYST-177-proposal-list branch Jan 3, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.