Skip to content

Add Edit Sheet service to Google Sheets#79149

Closed
tkdrob wants to merge 18 commits into
home-assistant:devfrom
tkdrob:google_sheets_options
Closed

Add Edit Sheet service to Google Sheets#79149
tkdrob wants to merge 18 commits into
home-assistant:devfrom
tkdrob:google_sheets_options

Conversation

@tkdrob
Copy link
Copy Markdown
Contributor

@tkdrob tkdrob commented Sep 27, 2022

Breaking change

Proposed change

Add Edit Sheet service to Google Sheets

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • New feature (which adds functionality to an existing integration)
  • Deprecation (breaking change to happen in the future)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Additional information

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • The code has been formatted using Black (black --fast homeassistant tests)
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
  • Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

  • No score or internal
  • 🥈 Silver
  • 🥇 Gold
  • 🏆 Platinum

To help with the load of incoming pull requests:

Copy link
Copy Markdown
Contributor

@chishm chishm left a comment

Choose a reason for hiding this comment

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

Apart from some suggested static type changes, this looks good to me.

I'm not sure what's going on with the code coverage, it seems to be flagging files in homeassistant/components/recorder, which are not touched by this PR.

Comment thread homeassistant/components/google_sheets/const.py Outdated

DOMAIN = "google_sheets"

CONF_SHEETS_ACCESS = "sheets_access"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you please make this a Final constant?

Comment thread tests/components/google_sheets/test_init.py Outdated
"""Return the desired sheets feature access."""
return [
FeatureAccess.file.value,
FeatureAccess[config_entry.options[CONF_SHEETS_ACCESS]].value,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you help me understand in more detail how the read and write scopes will be used? I get the general idea of read vs write scopes, but I don't quite get how these will be used compare to the file scope.

The way it appears right now is that I see is the user will create a single sheet (using the file scope) -- but it's also given a read-only scope for everything in the account. I don't see a way to access the other sheets or why giving broad read/write access can even be used.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The read scope gives read access to all spreadsheets. The file scope ensures users still have write access to the document that the integration created. I plan on extending the append service to allow the user to add a document id so they can specify a different document other than the integration made one.

The plan is to add an edit service which makes more sense for any other documents that people have access to that they want to modify. It should be the same schema for both services.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yeah I understand the scope differences, just wanting to see how they connect to the feature to ensure they all make sense together.

Ok, I'd say let's do the scope changes with the features it will enable? I don't mind viewing the larger PR. I just want to make sure it all connects.

@tkdrob tkdrob changed the title Add options flow to Google Sheets Add Edit Sheet service to Google Sheets Oct 9, 2022
@tkdrob tkdrob requested a review from allenporter October 14, 2022 12:38
Copy link
Copy Markdown
Contributor

@allenporter allenporter left a comment

Choose a reason for hiding this comment

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

I'm a bit confused about the options screen. The default value is it creates its own sheet and allows you to have read/write access to it. But if you open this up, you can only choose read or write access. Is there an option missing here with the default value of read/write to the default sheet?

Maybe it would be worth updating the help documentation to explain how this works as a reply -- since right now it also doesn't really talk about the default sheet.

@tkdrob tkdrob requested a review from allenporter December 24, 2022 20:24
@tkdrob tkdrob marked this pull request as draft July 26, 2023 23:48
@frenck
Copy link
Copy Markdown
Member

frenck commented Oct 6, 2023

Because there hasn't been any activity on this PR for quite some time now, I've decided to close it for being stale.

Feel free to re-open this PR when you are ready to pick up work on it again 👍

@frenck frenck closed this Oct 6, 2023
@github-actions github-actions Bot locked and limited conversation to collaborators Oct 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants