Cache.state does not correctly reflect the status of a geocache

[GeoTime61]

The Cache.state variable does not correctly reflect the enabled status of a geocache. Geocaches that are disabled or archived return a state of True. The problem seems to be in the load() method:

        self.state = root.find("ul", "OldWarning") is None

Current geocache pages use <div ...> with id="ctl00_ContentBody_disabledMessage" or id="ctl00_ContentBody_archivedMessage". Maybe the load() method should be:

self.state = not (root.find(id="ctl00_ContentBody_disabledMessage") or
                  root.find(id="ctl00_ContentBody_archivedMessage"))

This solution wouldn't correctly handle unpublished geocaches, but I don't know if that is a concern.

A geocache can have a status of Unpublished, Active, Disabled, Locked, or Archived. Would it be valuable to make all of these available in Cache()? Maybe through a new attribute, possibly Cache.status?

[FriedrichFroebel]

Thank your for the report.

I am actually a bit surprised why cache.state is just a boolean, while we have a corresponding enum pycaching.cache.Status. From my perspective it would be the correct way to not use a boolean value here, but this would be a breaking change. Having both cache.state and cache.status could prevent this, but this might lead to confusion - it we do this, we might want to make the boolean value a property which is derived from the actual pycaching.cache.Status dynamically.

pycaching.cache.Status is currently missing the Locked value. What is the issue with correctly handling unpublished caches?

@tomasbedrich What do you think about this?

[GeoTime61]

@FriedrichFroebel no issue with handling unpublished caches, it was just that my initial proposed solution didn't cover that status. After further research I propose something like this:

        # Cache status
        if root.find(id="ctl00_ContentBody_disabledMessage"):
            self.status = Status.Disabled
        elif root.find(id="ctl00_ContentBody_archivedMessage"):
            self.status = Status.Archived
        elif root.find(id="unpublishedMessage") or root.find(id="unpublishedReviewerNoteMessage"):
            self.status = Status.Unpublished
        elif root.find(id="ctl00_ContentBody_lockedMessage"):
            self.status = Status.Locked
        else:
            self.status = Status.Active

Then as you suggest, cache.state could be derived from cache.status:
self.state = self.status == Status.Active

And an update to Status() to follow the Groundspeak API status values:

class Status(enum.IntEnum):
    """Enum of possible cache statuses."""
# NOTE: extracted from https://api.groundspeak.com/documentation#geocache-states
# GEOCACHE STATUSES
    Unpublished = 1
    Active = 2
    Disabled = 3
    Locked = 4
    Archived = 5

[FriedrichFroebel]

Instead of proposing the changes here, could you please open a pull request which makes it easier to refer to specific details? We might want to add this parsing logic to a Status as a classmethod from_cache_details or similar to reduce the code inside the load method. Changes should be done for all loading methods we have, not only for load. Additionally, we probably should have some tests for this.

[tomasbedrich]

Hello, while I am not against publishing a new major version to change this API, my preference would be adding cache.status as enum and marking cache.state as deprecated (there is a decorator for it). We might want to implement backwards compatibility via __bool__ on Status enum.

[GeoTime61]

Does it make sense to create a method in cache.status to convert the enum to a string? Just to simplify user code.

I don't know enough about Python to implement the suggested improvements and create tests for them. I will leave it for someone else to implement these changes. Thank you for the feedback.

[tomasbedrich]

@GeoTime61 the issue is, that if you once implement a new interface – like this one – the ability to convert cache.status to str – people can rely on it. :) So that there should be a strong justification for that. In this case, comparing cache.status to Status enum itself seems like a little complication to pycaching user code, while providing greater future flexibility.

Confronting myself with "adding new interfaces" argument: I see the move from cache.state to cache.status as:

1. An issue, which needs to be solved. We are unable to distinguish multiple cache states now. Essentially it is a feature request.
2. A backwards-compatible move – leveraging deprecation policy (as mentioned in the docs).
3. A step towards better naming. I'd expect cache.status to match Status class name.

This seems to me like a good-enough justification to add a new cache.status interface. :)

[BelKed]

If it's possible, I'd like to implement those changes.
So if you, @GeoTime61, agree with that, I'll take a look at it this or the next week and open a pull request.

[GeoTime61]

Yes, please do. I don't have the skills to create a test script or implement the deprecation proposal.

…

On Mon, Mar 28, 2022 at 10:19 AM BelKed ***@***.***> wrote: If it's possible, I'd like to implement those changes. So if you, @GeoTime61 <https://github.com/GeoTime61>, agree with that, I'll take a look at it this week and open a pull request. — Reply to this email directly, view it on GitHub <#183 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AYLHHJYCUBCILS6FCGCS34LVCHS2XANCNFSM5RLRJV6A> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[FriedrichFroebel]

@BelKed Has there been any progress on this from your side?

[BelKed]

I did something a long time ago, but somehow I wasn't able to create tests. I tried, but it didn't work (I guess I wasn't able to record cassettes), so I gave up and almost forgot about it.
But if I'm not mistaken, it works, although I couldn't test it on unpublished caches since only the owner and reviewer can view them.

[FriedrichFroebel]

Thanks for your feedback. If you are still interested in helping with this, feel free to open a corresponding PR, so we can actively check how to work around your issues with the tests. It usually is much easier if we have a specific issue which is problematic when creating the tests instead of a generic one. I am open to support you there as well.

If you tried to add your tests to existing classes which overwrite setUpClass (or tried to re-use existing cassettes which had been recorded for such a class), you might run into #186 as well. A dedicated test case usually solves these issues.