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

184 change report client UI to put all reports into the same drop down #189

Conversation

vteague
Copy link
Member

@vteague vteague commented Aug 24, 2024

I modernized the Http calls too.

* @param raireUrl the url where the raire-service is running
* Needs to be synchronized just in case someone is foolish enough to call it twice in parallel
* with the same ZipOutputStream.
* @param zos an output stream (to become a zip file)
* @param suffix requested file type: "csv" or "json"
*/
Copy link
Member

Choose a reason for hiding this comment

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

Looks like both directory and prefix in inputs can be final.

* CSV suffix.
*/
final String csv = "csv";

Copy link
Member

Choose a reason for hiding this comment

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

Same comment as on another PR, can the port number be defined in test settings? Otherwise, what if 8111 clashes with something running on someone's computer?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea - have refactored the TestClassWithDatabase a bit to make sure it does actually read test. properties (which wasn't being read before) and that those port numbers are in there instead of spread all over the different classes.

* Endpoint for getting assertions.
*/
final String getAssertionsEndpoint = "/raire/get-assertions";

Copy link
Member

Choose a reason for hiding this comment

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

It would be clearer to make the suffix constants capitalised.


/**
* Test data for the two valid IRV contests, with json and csv. This gives the successfully-sanitized version of
* the contest names.
Copy link
Member

Choose a reason for hiding this comment

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

JSON/CSV extensions are defined as constants but the actual values are used here.


/**
* Good urls with bad endpoints.
*/
Copy link
Member

Choose a reason for hiding this comment

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

Same comment as above -- may as well use the defined csv/json constants.

}

/**
* Bad urls.
Copy link
Member

Choose a reason for hiding this comment

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

Same comment as above RE csv/json.

@vteague vteague merged commit 9fc6362 into main Aug 29, 2024
1 check passed
@vteague vteague deleted the 184-change-report-client-ui-to-put-all-reports-into-the-same-drop-down branch August 29, 2024 04:44
@vteague vteague restored the 184-change-report-client-ui-to-put-all-reports-into-the-same-drop-down branch August 30, 2024 00:43
@vteague vteague deleted the 184-change-report-client-ui-to-put-all-reports-into-the-same-drop-down branch August 30, 2024 00:46
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