BUG: Worksheet.isSheetHidden is not updated when Worksheet.hide() or Worksheet.show() used

[alifeee]

Describe the bug
Worksheet.isSheetHidden is not updated when Worksheet.hide or Worksheet.show (i.e., Worksheet._set_hidden_flag) is used.

To Reproduce

Create a spreadsheet with minimum 2 worksheets.

import gspread

gc = gspread.service_account(filename="./creds.json")
spreadsheet = gc.open("gspread test spreadsheet")
sheet = spreadsheet.sheet1

print("Sheet hidden (true/false):)")
print("initial:", sheet.isSheetHidden)

spreadsheet.worksheets()[1].hide()
print("after hide:", sheet.isSheetHidden)

spreadsheet.worksheets()[1].show()
print("after show:", sheet.isSheetHidden)

Expected behavior

Sheet hidden (true/false):)
initial: False
after hide: True
after show: False

Actual behavior

Sheet hidden (true/false):)
initial: False
after hide: False
after show: False

Environment info:

• Operating System: Windows
• Python version: 3.11.0
• gspread version: v5.9.0

Additional context
Worksheet._properties is set only when Worksheet is initialised.

Thus, whenever a property is changed, the correct API request must be sent, and then the property must be manually updated. For example, Worksheet.update_title runs self._properties[["title"]] = title:

    def update_title(self, title):
        ...
        body = {
            ...
        }

        response = self.spreadsheet.batch_update(body)
        self._properties["title"] = title
        return response

Internal behavior comparison

I had a look to see which "meta field updaters" remember (or don't!) to update the internal Worksheet._properties object with the update.

I call a "meta field updater" anything that updates things about the spreadsheet that are not the cells, i.e., name, tab color, etc. (i.e., spreadsheet properties). They can be found by searching for requests which send an "updateSheetProperties" request.

This is not an exhaustive list

meta field updaters which do change Worksheet._properties

• Worksheet.update_title
• Worksheet.update_tab_color

meta field updaters which do not change Worksheet._properties

• Worksheet.resize
• Worksheet.update_index
• Worksheet.freeze
• Worksheet._set_hidden_flag (and thus Worksheet.hide & Worksheet.show)

Thus, this is why the bug exists. In fixing this, it is probably worth fixing all 4 of these at once.

[alifeee]

Addendum: interaction with tests

Tests which test for only internal Worksheet._properties change

Some tests do not check that the API request actually works. For example, WorksheetTest.test_update_tab_color:

    def test_update_tab_color(self):
        ...
        red_color = {
            "red": 1,
            "green": 0,
            "blue": 0,
        }
        self.assertEqual(self.sheet.tab_color, None)
        self.sheet.update_tab_color(red_color)
        self.assertEqual(self.sheet.tab_color, red_color)

This only tests the final line of the Worksheet.update_tab_color method:

    def update_tab_color(self, color):
        ...
        body = {
            ...
        }

        response = self.spreadsheet.batch_update(body)
        self._properties["tabColorStyle"] = {
            "rgbColor": color,
        }
        self._properties["tabColor"] = color
        return response

For an example of testing google's API and the internal _properties store, see the updated WorksheetTest.test_update_tab_color from 'alifeee/gspread/blob/master`.

Tests which already test both

• WorksheetTest.test_worksheet_update_index ✅

Tests which test only _properties and not Google's API response

• WorksheetTest.test_update_tab_color ❌ (fixed by Feature: Add "Remove tab colour" method #1199)

Tests which test only Google's API response and not _properties

• WorksheetTest.test_hide_show_worksheet ❌ (does not test Worksheet.isSheetHidden)
• WorksheetTest.test_resize ❌ (does not test Worksheet.row_count or Worksheet.col_count)
• WorksheetTest.test_freeze ❌ (does not test Worksheet.frozen_row_count or Worksheet.frozen_col_count)

Methods without tests

• Worksheet.update_title has no test ❌

Note: Manual property updates

This problem arises from that one must update self._properties manually, as it is not updated by Spreadsheet.fetch_sheet_metadata. I am not sure the best solution.

As I see it, either:

• it remains manual, in which case it may be forgotten in future
• Spreadsheet.fetch_sheet_metadata is called upon changing the property, to fetch the new value (as in Feature: Add "Remove tab colour" method #1199)
• Spreadsheet.fetch_sheet_metadata is called upon fetching the property (bad?)

[lavigne958]

Hi let's take it one by one 😉

1. The initial issue with hide/show sheets: This is a good feature, we can make a PR that updates the properties upon each call to the method _set_hidden_flag.
2. The issues with the tests: yes some of tests actually only test that the given parameters are set, and not the API call resulting is right. We do need to change that (in this way we can catch other issues like the tab color !)
3. The main, abstract issue with: when updating the spreadsheet should I update the local property of the spreadhseet ? 👀

The answer to the last point (3.) we can't... 😢 for a single reason: right after updating the spreadsheet, someone (using web UI), something (using a second gspread script) might update the spreadsheet.

so while you hide the first worksheet, and then save locally that this first sheet is hidden, someone can show the first worksheet right after you. This is the major issue with distributed systems. So there's this big question about updating the local properties of a spreadsheet that comes back from time to time.

So far I chose not to update it and let the user refresh the sheet properties in order to first check the state of a property before updating it.

But knowing that quiet few people are asking for it, we can change that behavior and store the new property on each change knowing that it might already be outdated (or not ? we don't know until we fetch it form the API).

That's a major issue that we can't solve, but we can make either choice, whichever is best for the users.

[alifeee]

This is interesting.

I agree that someone could change any of these properties on the web, etc.. Thus keeping a local copy of the state (title/color/hidden) of the sheet up to date does not make much sense.

But then, what is the point of having the @properties (e.g., title) in the first place if they can be outdated? I suppose, that they are nice for when you first fetch a worksheet, but then quickly can become outdated.

Perhaps, we could change them so they are always the "newest" when asked for. So, instead of

current

    @property
    def title(self):
        """Worksheet title."""
        return self._properties["title"]

new

    @property
    def title(self):
        """Worksheet title."""
        return self._get_sheet_property(self, "title", "")

Of course, this would increase API calls a lot if someone wanted to get all the metadata on startup. But it would always be up to date, and would remove internal state of _properties.

[lavigne958]

yep that's a good question.

To me we should:

• update the local properties when we change them, it makes no sense to make a change then not update the local knowledge of it as this is the latest known state (hidden or the value of title etc).
• In the next major release (or later) we can think a new way to initiate a Spreadsheet and a Worksheet and offer the choice to either:
  • pull data every times we need it.
  • only pull date the first time and that's it (ex: for user who know they only interact with the spreadsheet using the current script).
• it will be a trade-off that the user can choose by itself which the best we can offer.
• It add a lots fo if etc around the properties but I think we can factorize it somewhere when the times come.

[alifeee]

update the local properties when we change them, it makes no sense to make a change then not update the local knowledge of it as this is the latest known state (hidden or the value of title etc).

I will make a branch/PR to do this, it should be an easy change. And was the original meaning of this issue.

For the rest we will have to have a think. How should we record this think?

[lavigne958]

we can create a new issue with a summary of the infos we have here, that should do it.

[alifeee]

I will do that now.

[alifeee]

okay. further discussion can happen in #1212
and merging #1211 will close this issue