Skip to content
This repository has been archived by the owner on Feb 8, 2018. It is now read-only.

Show as closed #4292

Merged
merged 5 commits into from
Jan 14, 2017
Merged

Show as closed #4292

merged 5 commits into from
Jan 14, 2017

Conversation

chadwhitacre
Copy link
Contributor

Following on from #4291 (which is included here), this adds a danger zone to the top of the project page, which is still accessible to owners and admins.

screen shot 2017-01-13 at 6 53 16 pm

- fix bugs in balance checking (`> 0` vs `!= 0`)
- allow admins to close accounts w/ balance
- refactor test suite a little bit
- add tests
Still a bit confobulated due to surrounding markup in various contexts.
For owners and admins, to avoid confusion.
@chadwhitacre
Copy link
Contributor Author

Another one, @mattbk et al.

Copy link
Contributor

@nobodxbodon nobodxbodon left a comment

Choose a reason for hiding this comment

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

LG other than those small things.

assert P('alice').is_closed

def test_close_can_be_overriden_for_negative_balance_too(self):
alice = self.make_participant('alice', claimed_time='now', balance=D('10.00'))
Copy link
Contributor

Choose a reason for hiding this comment

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

-10.00?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! Fixed in 0d2deb6.

assert response.code == 400
assert not P('alice').is_closed

def test_posting_with_balance_succeeds_for_admin(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

append "_with_negative_balance" in method name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Meh. We have tests above for the full matrix of {positive,negative} * {owner,admin}. The point here is to test with any balance. The fact that this one is negative while the previous test uses a positive balance is just extra.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because here we're running through the simplate, where the positive/negative isn't so much an issue as the permissions for various roles.

Copy link
Contributor

Choose a reason for hiding this comment

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

There's a principle in testing that making test case mimimal/simplest to achieve the test goal. Any additional conditions would add noise and unnecessery maintanance burden (consider other contributors having the same confusion as me).

Of course that's in theory :)

@chadwhitacre
Copy link
Contributor Author

I'm good to go here if you're good with 0d2deb6, @nobodxbodon. 👍

@nobodxbodon
Copy link
Contributor

!m @whit537 I'm good with 0d2deb6

@mattbk
Copy link
Contributor

mattbk commented Jan 14, 2017

@mattbk
Copy link
Contributor

mattbk commented Jan 14, 2017

Where does the balance go if you close?

Oh, I see. It just hangs out in the database, but the account is closed. I guess we're assuming that this isn't likely to be a problem, because people will either want their money before closing or it will be small amounts. It should be easy enough to look up how much "abandoned" money there is in closed accounts in the future.

I'm satisfied with this. Pushing the button.

@mattbk mattbk merged commit b925128 into master Jan 14, 2017
@mattbk mattbk deleted the show-as-closed branch January 14, 2017 18:30
@chadwhitacre
Copy link
Contributor Author

@mattbk Participants can't close their account with a balance outstanding, but #4291 allows admins to do so, which is necessary for #3602 (comment).

@chadwhitacre chadwhitacre mentioned this pull request Jan 16, 2017
5 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants