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 pagination, docs, and better tests for Rules #272

Merged
merged 2 commits into from
Jul 9, 2018

Conversation

joshcanhelp
Copy link
Contributor

  • Add pagination to getAll method in Rules
  • Throw CoreException for missing or empty required params in get, create, update, and delete
  • Rewrite tests for more isolation and better coverage

}

if (empty($data)) {
throw new CoreException('Empty "data" parameter; nothing to do.');
Copy link
Member

Choose a reason for hiding this comment

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

Can you make your errors consistent? In the create method you have
Missing required "script" field.
in this one it's
Empty "data" parameter; nothing to do.

If it's empty or invalid it's still invalid, as these are essential I would just have something simple like
{param} is missing

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 indicating 2 different things. In create, we require 2 specific fields in that $data array to create a Rule. In update, we don't require any specific fields but if it's empty then there is no reason to send the request. The error messages reflect that.

Copy link
Contributor

@lbalmaceda lbalmaceda Jul 4, 2018

Choose a reason for hiding this comment

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

Jumping in here.

  1. Empty or invalid "id" parameter. I agree with martin. That's "invalid" anyway. You can have a helper function somewhere that receives the value and the name of the parameter like I do and so keep a universal message like "Invalid {param name} parameter" somewhere.

  2. For the PATCH I know it makes no sense at all to make the call if the value is empty, but it's not that the call will fail with an error from the server. AFAIK It just won't do anything. Anyway, I also agree messages should be similar. If you are going to consider this an error scenario then just use the same message as in (1).

An alternative for (2) however is, knowing in advance the $data is empty, you can just log a message (not throw an exception) and avoid making the request. How does that sound?

Copy link
Contributor Author

@joshcanhelp joshcanhelp Jul 5, 2018

Choose a reason for hiding this comment

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

Thanks for the discussion here but I still don't see this as being inconsistent. I'm fine with changing Empty or invalid to just Invalid but where the message is different, the condition is different as well.

  • For get, delete, and create, the ID parameter needs to have a value (not null or "") or it's empty. It needs to also be a string (not [12345678] or SomeClassName) or it's invalid. Again, I'm fine with changing the exact wording.
  • For create, that forces an array to be passed so we know the type is right. But there are 2 specific keys/fields that it needs. If I say Invalid $data parameter, then I'm leaving out helpful information for debugging. I could say Invalid $data parameter ("name" and "script" keys are required) ... does that seem more consistent to you guys?

@joshcanhelp
Copy link
Contributor Author

@cocojoe - Updated exception language and removed the one for update

Copy link
Member

@cocojoe cocojoe left a comment

Choose a reason for hiding this comment

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

Errors look better, although you should think about unifying the errors generated. So have a function that you just pass a value and returns the standardised text etc

@joshcanhelp joshcanhelp added this to the v5-Next milestone Jul 9, 2018
@joshcanhelp joshcanhelp merged commit 385b65a into master Jul 9, 2018
@joshcanhelp joshcanhelp deleted the add-pagination-rules branch July 26, 2018 17:04
@github-actions
Copy link
Contributor

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 24, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants