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

Protect/unprotect #1273

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from
Draft

Protect/unprotect #1273

wants to merge 6 commits into from

Conversation

PFython
Copy link

@PFython PFython commented Aug 15, 2023

closes #1208

  • Added Worksheet.protect and Worksheet.unprotect. I've suggested keeping the names 'protect' and 'unprotect' instead of 'protect_all' and 'unprotect_all' because the scope is already clear ie. the whole Worksheet, and this avoids extra typing. The lack of arguments should also make it abundantly clear this doesn't protect/unprotect specific ranges.
  • Still need a way of fetching client.auth._service_account_email to send to Worksheet.add_protected_range(). This attribute seems to only be created on initialisation and I wasn't able to work out how to fetch it eg. from auth. One approach might be to add .email attribute to Spreadsheet when object is first initialised?
  • Added domain_users_can_edit=False as parameter to Worksheet.add_protected_range(). Without it, a range is created but other (non system) users can still edit ie. not "View only" mode.
  • Added Worksheet.list_protected_ranges as a helper method to match Spreadsheet.list_protected_ranges and avoid people having to know or guess code for the "round trip" ie. up to the parent spreadsheet, then back down to the original worksheet using id.
  • Added Worksheet.column_count as alias for .col_count. A pet peeve for many non-English speakers are abbreviations where an explicit (full English) variable name would avoid them having to guess.
  • Not sure what tests to add - my test was visual inspection of the sample Worksheet: Is there a padlock icon on the specified sheet? Is the protected range shown in the right hand panel (Data - Protect sheets and ranges)? This was what alerted me to things not working without 1domain_users_can_edit=False` (above).
  • Updated README

+Worksheet.protect +Worksheet.unprotect
Protecting/Unprotecting Worksheet
Copy link
Collaborator

@alifeee alifeee 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 this PR! good stuff :)

We will have a think about how to get the email. I'm not sure how all the auth works, but... is there always an email? For example if you are using a different type of authentication.

Further to-dos:

  • add tests
    • for protecting entire worksheet
    • for unprotecting entire worksheet
    • for protection kwarg domain_users_can_edit
    • for worksheet.list_protected_ranges()

@@ -1910,6 +1910,7 @@ def add_protected_range(
description=None,
warning_only=False,
requesting_user_can_edit=False,
domain_users_can_edit=False,
Copy link
Collaborator

Choose a reason for hiding this comment

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

this should be added to the Sphinx docs below (in function docstring)

@@ -2964,3 +2966,26 @@ def cut_range(
}

return self.spreadsheet.batch_update(body)

def column_count(self):
Copy link
Collaborator

Choose a reason for hiding this comment

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

I separated this out into #1274. It can be removed from here

Comment on lines +2978 to +2986
def protect(self):
"""Protect all ranges in current Worksheet"""
email = get_email_from_somewhere_tbd() # TODO ?
last_cell = rowcol_to_a1(self.row_count, self.col_count)
self.add_protected_range(
f"A1:{last_cell}",
email,
description="LOCKED by gspread user",
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Did we try to call the API to add a new protected range but with no actual range. Something like, instead of giving: sheet1!A1:Z99 what if we give it: sheet1 as "range" does the API understand that we want to cover everything and then protects everything?? 🙃

@alifeee alifeee added this to the 5.11.0 milestone Aug 22, 2023
@lavigne958 lavigne958 removed this from the 5.11.0 milestone Sep 1, 2023
@lavigne958
Copy link
Collaborator

I removed the milestone from this PR, we won't finish it on time for the next release.

@alifeee alifeee modified the milestones: 5.12, 6.0.0 Sep 28, 2023
@alifeee alifeee added Feature Request help wanted Need investigation This issue needs to be tested or investigated labels Oct 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature Request help wanted Need investigation This issue needs to be tested or investigated
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Suggestions for Wrapper and Convenient Methods (add protect/unprotect all)
3 participants