burnash / gspread

Google Sheets Python API
https://docs.gspread.org
MIT License
7.06k stars 945 forks source link

Fixed `APIError: [403]` when copying permissions #1515

Open wobYY opened 1 week ago

wobYY commented 1 week ago

This PR fixes the bug raised in the issue #1507.

tox test (replaced the first part of the path with local_dir to remove personal information):

>> tox -e py
.pkg: _optional_hooks> python local_dir\gspread\.venv\Lib\site-packages\pyproject_api\_backend.py True flit_core.buildapi
.pkg: get_requires_for_build_sdist> python local_dir\gspread\.venv\Lib\site-packages\pyproject_api\_backend.py True flit_core.buildapi
.pkg: build_sdist> python local_dir\gspread\.venv\Lib\site-packages\pyproject_api\_backend.py True flit_core.buildapi
py: install_package> python -I -m pip install --force-reinstall --no-deps local_dir\gspread\.tox\.tmp\package\5\gspread-6.1.2.tar.gz
py: commands[0]> pytest tests/
======================================================================= test session starts ========================================================================
platform win32 -- Python 3.12.4, pytest-8.3.3, pluggy-1.5.0
cachedir: .tox\py\.pytest_cache
rootdir: local_dir\gspread
configfile: pyproject.toml
plugins: vcr-1.0.2
collected 141 items

tests\cell_test.py .......                                                                                                                                    [  4%]
tests\client_test.py .............                                                                                                                            [ 14%]
tests\spreadsheet_test.py .................                                                                                                                   [ 26%]
tests\utils_test.py ........................                                                                                                                  [ 43%]
tests\worksheet_test.py ................................................................................                                                      [100%]

========================================================================= warnings summary ========================================================================= 
tests\worksheet_test.py:39
  local_dir\gspread\tests\worksheet_test.py:39: PytestRemovedIn9Warning: Marks applied to fixtures have no effect
  See docs: https://docs.pytest.org/en/stable/deprecations.html#applying-a-mark-to-a-fixture-function
    @pytest.fixture(autouse=True)

-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
================================================================== 141 passed, 1 warning in 3.87s ================================================================== 
.pkg: _exit> python local_dir\gspread\.venv\Lib\site-packages\pyproject_api\_backend.py True flit_core.buildapi
  py: OK (9.83=setup[4.83]+cmd[5.00] seconds)
  congratulations :) (9.97 seconds)

Also tested by copying a random spreadsheet in my workspace. For debugging I added a print in the if block:

                # .list_permissions() returns a list of permissions,
                # even the folder permissions if the file is in a shared folder.
                # We only want the permissions that are directly applied to the
                # spreadsheet file, i.e. 'writer', 'commenter' and 'reader'.
                perm_details = {
                    p_details.get("permissionType"): p_details.get("inherited")
                    for p_details in p.get("permissionDetails")
                }
                if p.get("role") in ("organizer", "fileOrganizer") and (
                    perm_details.get("file") or perm_details.get("member")
                ):
                    print(
                        f"`{p.get("id")[:3] + "..."}` has the `{p.get("role")}` role which is a folder permission, skipping..."
                    )
                    continue

                print(f"`{p.get("id")[:3] + "..."}` has the `{p.get("role")}` role, adding permission")

The result (screenshot): 011... has the organizer role which is a folder permission, skipping... 027... has the fileOrganizer role which is a folder permission, skipping... 029... has the commenter role, adding permission 033... has the fileOrganizer role which is a folder permission, skipping... 061... has the organizer role which is a folder permission, skipping... 083... has the fileOrganizer role which is a folder permission, skipping... 085... has the fileOrganizer role which is a folder permission, skipping... 087... has the organizer role which is a folder permission, skipping... 131... has the writer role, adding permission 134... has the fileOrganizer role which is a folder permission, skipping... 146... has the fileOrganizer role which is a folder permission, skipping... 152... has the organizer role which is a folder permission, skipping...

Correctly shared: image

alifeee commented 1 week ago

hi ! thanks for the change :)

I have fixed a couple of linting issues. There is a typing issue that mypy does not like, that confuses me a little. @lavigne958 will be able to look at it

otherwise, I think your change makes sense. we will try to get the workflows fixed and then think about merging :)