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

Add error treatment to apiUmbrellaWeb[...].createApiBackend - closes #357 #527

Merged
merged 6 commits into from
Oct 30, 2015

Conversation

mauriciovieira
Copy link
Contributor

@elnzv or @frenchbread please 🔍

@55
Copy link
Contributor

55 commented Oct 16, 2015

  1. Error message disappears too fast, extend time of showing or leave message until user closes it.
  2. It is not clear that there is a check after submission - form is cleaned and only after couple seconds error message is showing. I think we need to put a loading icon or some indication that the form is submitting.
  3. If there is an error - form is cleaned, I think we need to keep it and just indicate the field which causing the error.

If I get an error, server console says this:

screenshot from 2015-10-16 12 37 15

What is this [object Object]?

@mauriciovieira mauriciovieira force-pushed the feature/api-backend-error-handling branch from 229a7de to 4a815ee Compare October 20, 2015 11:11
@mauriciovieira
Copy link
Contributor Author

  1. Ok, I'll do this.
  2. This happens because first autoform inserts the new API to meteorjs mongo db and then we try to insert it in the API Umbrella through brylie:api-umbrella rest-client package. I agree with the loading icon and will do this ASAP.
  3. Yeah, I didn't not research how to send the errors back to the client, manually, without the autoform magic. I hope it is not impossible, or we would have to abdicate the use of autoform in this complex screen.
  4. I removed that unuseful Return response ... log.

@55 55 added the WIP label Oct 20, 2015
@55
Copy link
Contributor

55 commented Oct 20, 2015

@mauriciovieira, let me know when PR is ready to be reviewed by removing WIP label.

@mauriciovieira
Copy link
Contributor Author

@bajiat I am trying to fix this as @elnzv commented.

@@ -1,10 +1,43 @@
AutoForm.hooks({
apiBackends: {
onSuccess: function (formType, backendId) {
onSuccess: function (formType, apiBackendId) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We might need to change this to use before or onSubmit AutoForm hooks, so that we can validate the API Umbrella response.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem here is that I am afraid we might not have the apiBackend saved in mongoDB prior sending it to api umbrella. I will try this

@55
Copy link
Contributor

55 commented Oct 26, 2015

@mauriciovieira here is one approach to show that form is submitting:

AutoForm.hooks({
  apiBackends: {
    beginSubmit: function () {
      // Disable form elements while submitting form
      $('[data-schema-key],button').attr("disabled", "disabled");
    },
    endSubmit: function () {
      // Enable form elements after form submission
      $('[data-schema-key],button').removeAttr("disabled");
    }
  }
});

It simply disables all fields during the submission.

@mauriciovieira
Copy link
Contributor Author

@elnzv @brylie thank you guys for the tips. I already added some fixes to the code.

@brylie brylie modified the milestones: Sprint 15, Sprint 14 Oct 26, 2015
@brylie
Copy link
Contributor

brylie commented Oct 26, 2015

I note that there are two objects in you examples:

{
  default: {} 
}
{
  errors: {} 
}

What is the difference that causes the object to sometimes be named default and other times be named errors?

@mauriciovieira
Copy link
Contributor Author

@brylie, it is not:

{
  default: {} //object
}

but

{
  default: 'string with errors' 
}

That means, all we get from apiUmbrellaWeb.adminApi.v1.apiBackends.createApiBackend(constructedBackend); is a string, the error.message. As it is not treating the error and returning a json object, but only a string while brylie:api-umbrella issue#1 is not resolved. After it is resolved, we might get a json object

{
  errors: {} 
}

with the errors for each.

So, the format

{
  errors: {} 
}

is never returned untill we fix the meteor-package error handling.

@mauriciovieira mauriciovieira force-pushed the feature/api-backend-error-handling branch from ec903f5 to b75ed3e Compare October 27, 2015 11:12
@brylie brylie removed the WIP label Oct 28, 2015
@brylie
Copy link
Contributor

brylie commented Oct 28, 2015

@elnzv please review again. @mauriciovieira and I did pair programming over the past two days to get things working.

$('[data-schema-key], button').removeAttr("disabled");
},
before: {
// Replace `formType` with the form `type` attribute to which this hook applies
Copy link
Contributor

Choose a reason for hiding this comment

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

Cleanup comment

// };

if (apiUmbrellaWebResponse.http_status === 200) {
//Return asynchronously
Copy link
Contributor

Choose a reason for hiding this comment

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

Give a more meaningful comment here. E.g. 'submit the form on success'

55 pushed a commit that referenced this pull request Oct 30, 2015
Add error treatment to apiUmbrellaWeb[...].createApiBackend - closes #357
@55 55 merged commit 4fe7fae into develop Oct 30, 2015
@55 55 removed the in progress label Oct 30, 2015
@55 55 deleted the feature/api-backend-error-handling branch October 30, 2015 03:29
@mauriciovieira mauriciovieira restored the feature/api-backend-error-handling branch October 30, 2015 10:03
mauriciovieira added a commit that referenced this pull request Oct 30, 2015
Feature/api backend error handling - redoes #527
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.

None yet

3 participants