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

Make it clear how to persist updated AccountancyEntrys #412

Closed
martinmo opened this issue Nov 14, 2022 · 1 comment
Closed

Make it clear how to persist updated AccountancyEntrys #412

martinmo opened this issue Nov 14, 2022 · 1 comment
Assignees
Labels
meta: contribution welcome Issues ideal for people new to the project module: accountancy Accountancy type: enhancement Improvements and new features
Milestone

Comments

@martinmo
Copy link
Contributor

In Accountancy, the add(…) method currently has two purposes: 1) adding a new entry and 2) updating/saving an existing entry. However, the latter purpose isn't clear to Salespoint framework users, neither from the method name (i.e., something like save(…) would be better) nor from the javadoc. The AccountancyEntryRepository, which has a save(…) method, is package private.

@martinmo martinmo added type: enhancement Improvements and new features module: accountancy Accountancy labels Nov 14, 2022
@odrotbohm
Copy link
Member

odrotbohm commented Dec 1, 2022

AccountancyEntry instances are supposed to be immutable (as that's how bookkeeping works), so I guess we need to make sure we don't write any non-new entries.

  • Add check for AccountancyEntry.isNew() and reject parameter with an IllegalArgumentException (Spring's Assert class might be helpful here).
  • Add test case that tries to call Accountancy.add(…) with an already existing AccountancyEntry and verify it's rejected in AccountancyTests.

@odrotbohm odrotbohm added this to the 8.1 milestone Dec 1, 2022
@odrotbohm odrotbohm added the meta: contribution welcome Issues ideal for people new to the project label Dec 1, 2022
@vrckr vrckr self-assigned this Feb 14, 2023
vrckr added a commit that referenced this issue Feb 14, 2023
Change ensures that an AccountancyEntry that is already in the repository can not be added again by checking if the id of the AccountancyEntry already exists. Also adds a test case which checks for an IllegalArgumentException in case the same entry is added
odrotbohm pushed a commit that referenced this issue Feb 17, 2023
Change ensures that an AccountancyEntry that is already in the repository can not be added again by checking if the id of the AccountancyEntry already exists. Also adds a test case which checks for an IllegalArgumentException in case the same entry is added
odrotbohm added a commit that referenced this issue Feb 17, 2023
We're now avoiding the additional repository lookup by simply checking whether the AccountancyEntry is new. Polished test case using an a bit more readable assertion. Added missing authors in Javadoc.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
meta: contribution welcome Issues ideal for people new to the project module: accountancy Accountancy type: enhancement Improvements and new features
Projects
None yet
Development

No branches or pull requests

3 participants