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

Raire service responses #67

Merged
merged 14 commits into from
Apr 23, 2024
Merged

Raire service responses #67

merged 14 commits into from
Apr 23, 2024

Conversation

vteague
Copy link
Member

@vteague vteague commented Apr 18, 2024

This does error handling for the GenerateAssertions endpoint, for #41. This involves quite a lot of converting from raire-java errors. It also does #53.

The testing isn't yet ideal, because we don't really have a way of making most of the errors we're translating. More tests will make sense when #44 and #45 are done.

@vteague vteague requested a review from michelleblom April 18, 2024 11:27
package au.org.democracydevelopers.raireservice.response;

import au.org.democracydevelopers.raire.algorithm.RaireResult;

Copy link
Member

Choose a reason for hiding this comment

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

Can write "Get Assertions Request" as "GetAssertionsRequest"?
raire -> raire-java

* (risk limit, and an array of current risk estimates, one for each assertion).
*/
public final Metadata metadata;

Copy link
Member

Choose a reason for hiding this comment

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

raire -> raire-java

import au.org.democracydevelopers.raireservice.request.GetAssertionsRequest;
import java.math.BigDecimal;
import java.util.List;

Copy link
Member

Choose a reason for hiding this comment

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

"Get Assertions Response" -> "GetAssertionsResponse"

* The contest name.
*/
public final String contest;

Copy link
Member

Choose a reason for hiding this comment

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

I didn't quite understand the comment "This is expected .... doest not need to be"

Copy link
Member Author

Choose a reason for hiding this comment

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

I just meant that Colorado typically has the same risk limit for all the contests under audit (usually 3%) but that, in principle, raire-java will happily compute different difficulties as a function of an individual-contest risk limit. (I was justifying having a separate risk limit as part of the query for each contest, when everyone in colorado knows that it's always 3%). I have now tried to make this clearer.

* but for raire it really does not need to be.
*/
public final BigDecimal riskLimit;

Copy link
Member

Choose a reason for hiding this comment

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

Rather than saying "This is constantly ... process" perhaps clarify that the current risk estimates for each assertion are constantly updated in the database for each assertion by colorado-rla.

}

public enum RaireErrorCodes {

Copy link
Member

Choose a reason for hiding this comment

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

Put a javadoc comment above the enum definition?

Also, where raire/RAIRE is used, should we say raire-java since this is what will actually be generating the assertions?

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree that generally distinguishing 'raire-java' from the 'raire-service' from 'raire in general' is a good idea. I've changed this where you pointed it out (or where I noticed) but might not have got them all (yet).

*/
INTERNAL_ERROR,
}

Copy link
Member

Choose a reason for hiding this comment

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

"out of a raire-java error"

assertTrue(StringUtils.containsIgnoreCase(msg, "Candidate list does not match database"));
assertEquals(RaireErrorCodes.WRONG_CANDIDATE_NAMES, e.errorCode);
}

Copy link
Member

Choose a reason for hiding this comment

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

raire -> raire-java (also see later uses of raire)
Typo on elimination

* or timeout generating assertions.
*/
public class GenerateAssertionsException extends Exception {

Copy link
Member

Choose a reason for hiding this comment

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

Should we add a comment for errorCode?

// This should be part of Issue https://github.com/DemocracyDevelopers/raire-service/issues/44
} catch (IllegalArgumentException | ArrayIndexOutOfBoundsException ex) {
throw new ResponseStatusException(HttpStatus.INTERNAL_SERVER_ERROR, ex.getMessage());
}
}

Copy link
Member

Choose a reason for hiding this comment

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

Typo: repository

@vteague vteague requested a review from michelleblom April 22, 2024 03:41
@michelleblom michelleblom merged commit 14a37e6 into main Apr 23, 2024
1 check passed
@vteague vteague deleted the RaireServiceResponses branch April 23, 2024 06:05
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.

2 participants