-
Notifications
You must be signed in to change notification settings - Fork 1
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
Input verification #48
Conversation
…les, even though they are not quite set up right, and a trivial test of correct setup (which does not currently pass).
… contests by name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comments/requests for changes are all around commenting.
Added note: for Contest entity, should attributes be private with appropriate getters added?
We will need to remember to remove the TODO's/any code that will become redundant (if any) when we make requests based on contest name.
Tests seem very thorough!
src/main/java/au/org/democracydevelopers/raireservice/RaireJavaApplication.java
Outdated
Show resolved
Hide resolved
public long countyID; | ||
|
||
/** | ||
* Version. Currently not used. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will be used for optimistic locking. Consider adding comment "Version, for optimistic locking."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should it also be nullable=false and updatable=false?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also note, for county_id attribute, comment refers to "this Assertion".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, should all attributes be nullable=false and updatable=false?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree that in Contest all attributes other than version should be nullable=false and updatable=false, which I've now done.
Need to think more about versions for Contest. We certainly won't update it, but perhaps someone else (i.e. colorado-rla) might, so I'm not sure what the updatable=false annotation implies about that case.
Actually on that point, if name is updated by canonicalization, is that consistent with the updatable=false annotation?
...n/java/au/org/democracydevelopers/raireservice/persistence/repository/ContestRepository.java
Outdated
Show resolved
Hide resolved
...n/java/au/org/democracydevelopers/raireservice/persistence/repository/ContestRepository.java
Outdated
Show resolved
Hide resolved
...n/java/au/org/democracydevelopers/raireservice/persistence/repository/ContestRepository.java
Outdated
Show resolved
Hide resolved
...est/java/au/org/democracydevelopers/raireservice/request/GenerateAssertionsRequestTests.java
Outdated
Show resolved
Hide resolved
...est/java/au/org/democracydevelopers/raireservice/request/GenerateAssertionsRequestTests.java
Outdated
Show resolved
Hide resolved
src/test/java/au/org/democracydevelopers/raireservice/request/GetAssertionsRequestTests.java
Show resolved
Hide resolved
src/test/java/au/org/democracydevelopers/raireservice/request/GetAssertionsRequestTests.java
Outdated
Show resolved
Hide resolved
src/test/java/au/org/democracydevelopers/raireservice/request/GetAssertionsRequestTests.java
Outdated
Show resolved
Hide resolved
Thanks for that really careful review - I've marked as 'resolved' all the things I've just immediately changed. See above discussion for the slightly more complicated ones. |
…ssertionsRequest.java into an abstract superclass called ContestRequest.java
Refactored common elements of GenerateAssertionsRequest.java and GetA…
src/main/java/au/org/democracydevelopers/raireservice/persistence/entity/Contest.java
Show resolved
Hide resolved
src/main/java/au/org/democracydevelopers/raireservice/persistence/entity/Contest.java
Show resolved
Hide resolved
src/main/java/au/org/democracydevelopers/raireservice/persistence/entity/Contest.java
Show resolved
Hide resolved
...n/java/au/org/democracydevelopers/raireservice/persistence/repository/ContestRepository.java
Show resolved
Hide resolved
...n/java/au/org/democracydevelopers/raireservice/persistence/repository/ContestRepository.java
Show resolved
Hide resolved
import java.math.BigDecimal; | ||
import java.util.List; | ||
import org.springframework.data.annotation.ReadOnlyProperty; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment on describing contests from other files, also what is a GetAssertionsRequest?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I cleaned this up a bit, but lmk if it still doesn't make sense.
*/ | ||
|
||
package au.org.democracydevelopers.raireservice.request; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Worth adding a class description to indicate what this kind of exception means.
...a/au/org/democracydevelopers/raireservice/persistence/repository/ContestRepositoryTests.java
Show resolved
Hide resolved
@@ -1,3 +1,4 @@ | |||
spring.datasource.driver-class-name=org.testcontainers.jdbc.ContainerDatabaseDriver | |||
spring.jpa.hibernate.ddl-auto=create |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have a look at what I have done for this file in the CVR retrieval branch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes that's a huge improvement, thanks. I won't pull it into this branch, but definitely main should be that.
@@ -0,0 +1,16 @@ | |||
-- Contest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have a look at what I have done for this file in the CVR retrieval branch (as well as the added corla.sql schema).
Completes #38 and #34.
Partially completes #39.