Skip to content

Conversation

@daros10
Copy link
Contributor

@daros10 daros10 commented Apr 21, 2020

Issue realted to #89.

@daros10 daros10 force-pushed the 89-Edit-and-delete-customers branch from 0a5ff9a to 4e1bc8f Compare April 21, 2020 22:45
import { CustomerState, CreateCustomer } from 'src/app/modules/customer-management/store';
import * as models from 'src/app/modules/shared/models/index';
import { LoadCustomers } from './../../../../store/customer-management.actions';
import * as models from 'src/app/modules/shared/models/index';
Copy link
Contributor

Choose a reason for hiding this comment

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

import should be
import { Customer } from 'src/app/modules/shared/models';

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

import { CustomerState, CreateCustomer, LoadCustomers } from 'src/app/modules/customer-management/store';
import { getStatusMessage } from './../../../../store/customer-management.selectors';
import { getStatusMessage, getCustomerById } from './../../../../store/customer-management.selectors';
import { Customer } from 'src/app/modules/shared/models/index';
Copy link
Contributor

@jorgecod jorgecod Apr 21, 2020

Choose a reason for hiding this comment

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

import should be
import { Customer } from 'src/app/modules/shared/models';

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

if (this.customerToEdit) {
const customer = {
...customerData,
id: this.customerToEdit[key],
Copy link
Contributor

Choose a reason for hiding this comment

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

I think a better solution would be this.
const customer = { ...this.customerToEdit, ...customerData }; o const customer = { ...customerData, id: this.customerToEdit.id };
with this we would eliminate line 57
const key = 'id';

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

action: CustomerManagementActions
): CustomerState {
const customersList = [...state.data];
const key = 'id';
Copy link
Contributor

Choose a reason for hiding this comment

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

Please, let's apply the previous comment here too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

customerIdToEdit: '',
};

export function customerManagementReducer(
Copy link
Contributor

Choose a reason for hiding this comment

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

we must use arrow functions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

export const getCustomerById = createSelector(allCustomers, customerIdtoEdit, (customers, customerId) => {
const key = 'id';
return customers.find((customer) => {
return customer[key] === customerId;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you should add an "id" in the Customer interface to avoid this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

const url = `${this.baseUrl}/${customerData.id}`;
const body = {
...customerData,
};
Copy link
Contributor

Choose a reason for hiding this comment

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

body object it's not necessary, please remove it and send directly customerData.
Please remove it also remove in createCustomer method, and remove the tenantId.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

});

export const getCustomerById = createSelector(allCustomers, customerIdtoEdit, (customers, customerId) => {
return customers.find((customer) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

could add the validation for (customers, customerId)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All changes were applied. @enriquezrene when you have a free time, please, check it.

@enriquezrene enriquezrene merged commit 915c875 into master Apr 22, 2020
@enriquezrene enriquezrene deleted the 89-Edit-and-delete-customers branch April 22, 2020 20:18
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.

5 participants