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

Google Sheets Integration #142

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

VSydor
Copy link
Contributor

@VSydor VSydor commented Jun 22, 2023

@VSydor VSydor requested a review from brmeyer June 22, 2023 13:48
@VSydor VSydor self-assigned this Jun 22, 2023
@VSydor VSydor force-pushed the feature/google-sheets-integration branch from 21aa1bc to 4254497 Compare June 27, 2023 14:50
@VSydor VSydor changed the title Google Sheets Integration (In Progress) Google Sheets Integration Jun 27, 2023
@@ -343,7 +343,7 @@ protected String getAccessToken() throws Exception {
log.info("token expired; jwt={} now={}; refreshing...", jwt.getExpiresAt().getTime(), now);

try {
TokenResponse tokenResponse = new RefreshTokenRequest(new NetHttpTransport(), new JacksonFactory(),
TokenResponse tokenResponse = new RefreshTokenRequest(new NetHttpTransport(), GsonFactory.getDefaultInstance(),
Copy link
Contributor

Choose a reason for hiding this comment

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

@VSydor Are we positive this won't break anything? There are caveats to how Jackson vs. Gson handle default mappings. IIRC, Jackson defaults to camelCase while Gson defaults to snake_case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, good point - will double check this part.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@brmeyer xero-java is using an older version of com.google.api-client:google-api-client lib (same lib required for google sheets integration)
I think the best we can do is to upgrade the version of xero java (use the newest one) that relies on the most recent version of google-api-client.
Updated nucleus-parent to use most recent version of xero-java:
impactupgrade/nucleus-parent#8

Copy link
Contributor

Choose a reason for hiding this comment

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

@VSydor Forgive me for not remembering this: since nucleus-parent is updated, do we still need this switch to Gson in Xero?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Short answer is: yes - we do

Long one:
Both Google Sheets lib (google-api-services-sheets) and Xero lib (xero-java) depend on (google-oauth-client)
Sheets is using (1.34.1) and Xero uses (1.23.0)

We either need to update xero lib version (will also use newer version of google-auth), or just update the code to use Sheets version of google auth (Gson instead of Jackson)


private final Sheets sheets;

public GoogleSheetsClient(EnvironmentConfig.GoogleSheets googleSheetsConfig) throws GeneralSecurityException, IOException {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you detail for us how all these details are initially found? Do we need an Oauth connection process in Portal? Does it require a Google app?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Requires connected app in Google Cloud and service account with granted access to the given spreadsheet.
I will add these details as a comment in the code.

@VSydor VSydor force-pushed the feature/google-sheets-integration branch from 4254497 to cce36e6 Compare July 3, 2023 21:36
@VSydor VSydor requested a review from brmeyer July 3, 2023 21:41
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