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

Feature/comm 2233 clone api #38

Merged
merged 58 commits into from
Sep 5, 2017
Merged

Conversation

ivanzusko
Copy link
Contributor

No description provided.

}),
state => {
const plugins = selector(state, 'plugins');

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd remove this empty line

const lens = R.lensPath(['plugins']);
// substitude the plugin.config.limit
const updatedApi = R.set(lens, updatedPlugins, api);
// const x = api.plugins.map((item, index) => item.name === )
Copy link
Contributor

Choose a reason for hiding this comment

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

Just remove the commented code

return {
// initialValues: transformFormValues(state.apiReducer.api),
initialValues: transformFormValues(updatedApi),
// ***FOR SOME REASON NAME of the plugin DISCARDS while saving
Copy link
Contributor

Choose a reason for hiding this comment

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

// FIXME name of the plugin is discarded while saving

};
const getValues = key => initialValues.proxy[key];
const optionsTransformer = config => {
const opts = config.map(item => ({
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe return first return config.map(item => ({})

updateEndpoint,
} from '../../../store/actions';

import ApiItem from './ApiItem';

const mapStateToProps = state => ({
api: state.apiReducer.api,
selectedPlugins: state.apiReducer.selectedPlugins,
// selectedPlugins: state.apiReducer.api.plugins.map(item => item.name),
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd remove commented code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is my personal Dragon I'm fighting with 🐉

plugins,
excludePlugin,
selectPlugin,
selectedPlugins,
} = props;
console.error('NEW API', props);
const parse = value => (value === undefined ? undefined : parseInt(value));
Copy link
Contributor

Choose a reason for hiding this comment

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

const parse = value => value && parseInt(value);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Damn... you are right! Don't understand why I did do that by myself 😞

@@ -22,14 +24,20 @@ const propTypes = {
class NewApiItem extends Component {
componentWillMount() {
this.props.resetEndpoint();
console.error('------', this.props.location.pathname);
Copy link
Contributor

Choose a reason for hiding this comment

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

curious why logging error here?

renderForm = () => {
if (this.hasToBeCloned()) {
if (!R.isEmpty(this.props.api)) {
console.error('go to EDIT =>c ', this.props);
Copy link
Contributor

Choose a reason for hiding this comment

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

console.error

console.log('FETCH_ENDPOINT_ERROR', 'Infernal server error', error);
try {
const response = await client.get(`apis${pathname}`);
const preparedPlugins = response.data.plugins.map(plugin => {
Copy link
Contributor

Choose a reason for hiding this comment

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

If this function is the same of the above in willClone method just extract this ad reuse it in both places

Copy link
Contributor Author

Choose a reason for hiding this comment

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

almost, but has some differences. I'll try to find the way to extract the common part in separate PR

const preparedApi = R.set(R.lensPath(['plugins']), preparedPlugins, api);

// return client.post('apis', api)
return client.post('apis', preparedApi)
Copy link
Contributor

Choose a reason for hiding this comment

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

here you could make use of async/await

try {
  apis = await client.post('apis', preparedApi)
  dispatch(....)
} catch (e) {
  switch true {
    case error.response:
    case error.request:
    default:
  }
}

})
.catch((error) => {
if (error.response) {
return client.put(`apis${pathname}`, preparedApi)
Copy link
Contributor

Choose a reason for hiding this comment

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

async/await 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.

Yeap, @rafaeljesus , this is done is separate PR#39


return R.set(lens3, getSelectedPolicy(plugin.config.policy), pluginWithPolicyFromSchema);
}
if (plugin.name === 'request_transformer') {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we could use a switch statement

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeap, I'll do this in a separate PR in which I will also try to extract the common parts. This one is already too big 😷

value: 'local',
},
{
label: 'destributed',
Copy link
Contributor

Choose a reason for hiding this comment

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

typo distributed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Genau, @rafaeljesus ... 😞

try {
const response = await client.get(`apis${pathname}`);
const preparedPlugins = response.data.plugins.map(plugin => {
if (plugin.name === 'rate_limit') {
Copy link
Contributor

Choose a reason for hiding this comment

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

switch case 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.

Yeap, I'll do this in a separate PR in which I will also try to extract the common parts. This one is already too big 😷

}
});
};
export const preparePlugins = api => api.plugins.map(plugin => {
Copy link
Contributor

Choose a reason for hiding this comment

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

This function should not be public/exported right? Only for internal usage

Copy link
Contributor Author

@ivanzusko ivanzusko Sep 5, 2017

Choose a reason for hiding this comment

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

it's exporting for future. to make it tested by Jest.

But I've got your point => it should be extracted to separate file and tested there, but not here.

Caraca! 🇧🇷

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just as reminder for myself: issue#40

// substitude updated list of plugins
const preparedApi = R.set(R.lensPath(['plugins']), preparedPlugins, api);

return client.put(`apis${pathname}`, preparedApi)
Copy link
Contributor

Choose a reason for hiding this comment

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

async/await 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.

⏫ it's done and is waiting for you here, @rafaeljesus : #39

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah perfect I will review after lunch then 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ta bom!

@rafaeljesus
Copy link
Contributor

Don't for get to create a Jira Ticket or something for doing the improvements we've pointed out bitte

@rafaeljesus
Copy link
Contributor

👍

@ivanzusko ivanzusko merged commit cc56e99 into master Sep 5, 2017
@ivanzusko ivanzusko deleted the feature/COMM-2233__clone-api branch September 5, 2017 13:59
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.

2 participants