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

UP-431 - Adds product recommendations method #4

Merged
merged 6 commits into from
Aug 31, 2020

Conversation

uschischroder
Copy link
Contributor

Draft: need the types from the previous PR.

@uschischroder uschischroder force-pushed the UP-431-method-product-recommendations branch from 4a59a65 to 6002b0e Compare August 25, 2020 07:30
.join('&');
}

export async function getProductRecommendations(
Copy link
Contributor

Choose a reason for hiding this comment

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

tests are missing

): Promise<Result<BaseProduct, HttpError>> {
const inputParams = makeSearchParams(params);
try {
const recommendations = await get(`/recommendations${inputParams}`);
Copy link
Contributor

Choose a reason for hiding this comment

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

before search params string there should be ? sign

@uschischroder uschischroder marked this pull request as ready for review August 27, 2020 16:42
@uschischroder uschischroder force-pushed the UP-431-method-product-recommendations branch from fb55251 to 25a3f81 Compare August 28, 2020 07:02
import { get, post, put, patch } from './methods';


const PRODUCTS_API = 'https://products.35up.com/';
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be stored in env

import { createMethod } from '@caseable/http-client';


const BASE_URL = 'https://products.35up.com/';
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be in env

src/types.ts Show resolved Hide resolved
Copy link
Contributor

@Drafter500 Drafter500 left a comment

Choose a reason for hiding this comment

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

Just move APIs to env and we are good to go

@@ -0,0 +1,44 @@
import { expect } from 'chai';
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess this was already discussed in other admin panel PR that here we are basically testing createMethod which is a 3rd party library function.
So this test suit could be removed
@enjoythelive1 @Drafter500

@@ -0,0 +1,42 @@
import { InputParameters, ProductRecommendations } from '../types';

export const getMockRecommendations = (params: InputParameters): ProductRecommendations => ({
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
export const getMockRecommendations = (params: InputParameters): ProductRecommendations => ({
export const getMockRecommendations = (
params: InputParameters,
): ProductRecommendations => ({

@@ -39,3 +42,14 @@ export function makeSearchParams(input: InputParameters): string {
.join('&');
}

export async function getProductRecommendations(
params: InputParameters,
): Promise<Result<ProductRecommendations, HttpError>> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we go for Result or RemoteData ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I incline for remote data

Copy link
Contributor

@enjoythelive1 enjoythelive1 left a comment

Choose a reason for hiding this comment

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

Is the mechanism to load env in the build implemented?

@uschischroder uschischroder force-pushed the UP-431-method-product-recommendations branch from c927bf6 to 0b1c138 Compare August 28, 2020 09:39
@Drafter500 Drafter500 merged commit 4092eec into master Aug 31, 2020
@Drafter500 Drafter500 deleted the UP-431-method-product-recommendations branch August 31, 2020 08:42
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.

4 participants