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

Transfer Trip Surveys to S3 #267

Merged
merged 47 commits into from
Dec 3, 2024
Merged

Transfer Trip Surveys to S3 #267

merged 47 commits into from
Dec 3, 2024

Conversation

binh-dam-ibigroup
Copy link
Collaborator

@binh-dam-ibigroup binh-dam-ibigroup commented Nov 12, 2024

Checklist

  • Appropriate branch selected (all PRs must first be merged to dev before they can be merged to master)
  • Any modified or new methods or classes have helpful JavaDoc and code is thoroughly commented
  • [na] The description lists all applicable issues this PR seeks to resolve
  • The description lists any configuration setting(s) that differ from the default settings
  • All tests and CI builds passing

Description

This PR adds support for transferring responses and deleting from TypeForm, compiling them into one CSV file, and upload that as a ZIP file to S3.

New config params: TRIP_SURVEY_API_TOKEN, for downloading survey metadata and responses.

@binh-dam-ibigroup binh-dam-ibigroup changed the base branch from dev to post-deviated-trip-notification November 12, 2024 15:09
Base automatically changed from post-deviated-trip-notification to dev November 13, 2024 16:25
@binh-dam-ibigroup binh-dam-ibigroup marked this pull request as ready for review November 14, 2024 17:02
Copy link
Contributor

@JymDyerIBI JymDyerIBI left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me.

Copy link
Contributor

@br648 br648 left a comment

Choose a reason for hiding this comment

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

Some minors and perhaps consider moving the type form stuff into it's own package.

}
}

private static HttpResponseValues apiRequest(HttpMethod method, String subPath, String queryParams, String topic) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider moving all typeform related classes and methods to its own package similar to bugsnag. To me this seems cleaner and separates typeform interactions from business logic. Lemme know what you think.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point, updated in c27a5de.

@br648 br648 assigned binh-dam-ibigroup and unassigned br648 Nov 26, 2024
Copy link
Contributor

@br648 br648 left a comment

Choose a reason for hiding this comment

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

Thanks for the changes. I'm approving, but see changes in order to get test to pass in a different time zone.


@Test
void canMakeResponsesParams() {
// October 25, 2024 00:00, US Pacific Daylight Time
Copy link
Contributor

Choose a reason for hiding this comment

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

In order to use the config param OTP_TIMEZONE: America/Los_Angeles and for this test to pass (outside of this time zone), the following lines have to be added to this test:

// Load default env.yml configuration.
ConfigUtils.loadConfig(new String[]{});

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You are right, the class can actually extend OTPMiddlewareTestEnvironment that includes loading the config (18ecd9f).

@br648 br648 assigned binh-dam-ibigroup and unassigned br648 Dec 3, 2024
@binh-dam-ibigroup binh-dam-ibigroup merged commit c904f1a into dev Dec 3, 2024
2 checks passed
@binh-dam-ibigroup binh-dam-ibigroup deleted the transfer-trip-surveys branch December 3, 2024 14:59
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.

3 participants