Skip to content

Add exception-handling for value-less-than-or-equal-to-zero #114

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

McDonnough
Copy link
Member

Resolve isssue #113 on the server-side. A corresponding change still has to be implemented in the web client.

@McDonnough McDonnough self-assigned this Oct 17, 2017
@McDonnough McDonnough requested a review from lroellin October 17, 2017 18:35
@thde
Copy link
Member

thde commented Oct 18, 2017

Great @McDonnough! What do you think about changing the spec to accept values equal to zero? @McDonnough & @lroellin It would make an implementation much easier in my view (also ours).

@teiler teiler deleted a comment Oct 18, 2017
@McDonnough
Copy link
Member Author

McDonnough commented Oct 18, 2017

@thde @lroellin
In the case of a compensation, I would leave the check as it is right now. A compensation with an amount of zero or less doesn't make much sense and thus a corresponding message should be displayed to the user in question.
Talking about expenses things work a bit differently, as two checks are made at once and either of them lead to the aformentioned exception. Firstly the amount of the expense is checked for being zero or less (similar to what happens in the case of a compensation) and secondly all the profiteers' shares are checked for being zero or less as well. We could get rid of the second check, only verify that all shares add up to the full expense-amount and ignore all profiteers with a share of zero or less.

Considering this, I propose the following changes to be made [WIP]:

  • Change the shares-less-than-or-equal-to-zero-check to only check negative (i.e. less-than-zero) values in ExpenseUtil#checkValuesGreaterThanZero.
  • Adapt the logic in ExpenseService#createExpense and ExpenseService#editExpense to be capable of handling profiteers with a share-amount of zero.
  • Update the API-documentation:
    • Change VALUE_LESS_THAN_OR_EQUAL_TO_ZERO to VALUE_LESS_THAN_ZERO from here.
    • Remove VALUE_LESS_THAN_OR_EQUAL_TO_ZERO to VALUE_LESS_THAN_ZERO from here.
  • Extend the web-client to
    • display a fitting message if a compensation with an amount of zero is submitted to the server.
    • a negative amount is submitted for either a compensation or an expense.

@lroellin
Copy link
Member

I agree with you, but we still need to check for negative values. Keep in mind that the web client, which checks this, is not the only consumer of our API. So I would opt to go with a "Value less than zero" approach.

@McDonnough
Copy link
Member Author

McDonnough commented Oct 19, 2017

You're absolutely right about the negative values. I totally forgot about that as I was fixed on the zero-situation. I updated my comment above to reflect your input.

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.

3 participants