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

Remove gsheets related code #35436

Merged
merged 8 commits into from
Nov 27, 2024
Merged

Remove gsheets related code #35436

merged 8 commits into from
Nov 27, 2024

Conversation

jingcheng16
Copy link
Contributor

@jingcheng16 jingcheng16 commented Nov 25, 2024

Product Description

Technical Summary

We no longer plan to develop gsheet feature. So in this PR I remove all the related code.
Ticket: https://dimagi.atlassian.net/browse/SAAS-16161

Feature Flag

Safety Assurance

Safety story

The deleted code are not referenced anywhere

Automated test coverage

QA Plan

No

Migrations

  • The migrations in this code can be safely applied first independently of the code since the code is not being used (there would be 500 errors if the removed URL patterns were accessed).

Rollback instructions

  • This PR can be reverted after deploy with no further considerations
    I didn't write reverse code in migration because I don't expect this PR to be reverted

Labels & Review

  • Risk label is set correctly
  • The set of people pinged as reviewers is appropriate for the level of risk of the change

@dimagimon dimagimon added dependencies Pull requests that update a dependency file reindex/migration Reindex or migration will be required during or before deploy labels Nov 25, 2024
@jingcheng16 jingcheng16 marked this pull request as ready for review November 25, 2024 17:12
@jingcheng16 jingcheng16 requested a review from a team as a code owner November 25, 2024 17:12
@jingcheng16
Copy link
Contributor Author

Checked on production, the deleted sql model don't have any data. So no need to run migration before deploy.

@jingcheng16 jingcheng16 added the product/invisible Change has no end-user visible impact label Nov 25, 2024
Copy link
Contributor

@millerdev millerdev left a comment

Choose a reason for hiding this comment

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

Looks like another make requirements is needed to fix tests.

diff --git a/requirements/dev-requirements.txt b/requirements/dev-requirements.txt
index eea7e9f..2656028 100644
--- a/requirements/dev-requirements.txt
+++ b/requirements/dev-requirements.txt
@@ -294,7 +294,6 @@ gnureadline==8.1.2
 google-api-core==2.11.0
     # via
     #   firebase-admin
-    #   google-api-core
     #   google-api-python-client
     #   google-cloud-core
     #   google-cloud-firestore

Otherwise this looks good to me.

@jingcheng16
Copy link
Contributor Author

@millerdev Weird, I just run make requirements again and there is no new changes...

Copy link
Contributor

@orangejenny orangejenny left a comment

Choose a reason for hiding this comment

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

Reviewed last two commits

@jingcheng16 jingcheng16 merged commit 4332a2d into master Nov 27, 2024
12 of 13 checks passed
@jingcheng16 jingcheng16 deleted the jc+jls/remove-gsheets branch November 27, 2024 22:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file product/invisible Change has no end-user visible impact reindex/migration Reindex or migration will be required during or before deploy
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants