-
Notifications
You must be signed in to change notification settings - Fork 45
Issue 179 #181
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
Issue 179 #181
Conversation
Thanks for the PR. Unfortunately your changes fail in the CI which seems to be an issue with the Betamax cassettes being outdated and requiring an update as well. |
If you think the problem lies in the testing itself, I won't waste my time figuring out what's wrong with my code changes, so thanks for that remark. |
The problem is not in the testing itself, but in the Geocaching.com site changes. For this reason your tests fail at the moment and you have to re-record the corresponding cassettes. Otherwise I will not be able to merge your changes. |
Thanks for pointing out. Will see if I understand how that works, and will give it a try this weekend. |
The print-page doesn't show a pm_only property anymore, as has been documented in the load_by_guid() function's header. I think that, to make the assert tests fail, a work-around was implemented in lines 879-880: However, this tag has now also been dropped. As far as I can tell, there's no way to distinguish a pm_only from a regular cache on this print-page anymore. We have two options to fix the failing tests:
Attached two html files (saved as txt files, because this interface doesn't allow html files to be dropped), of the print-page of a PM-Only cache, and a regular cache. Tell me which is which, and maybe you found a third way to fix it :-) Any ideas? |
The PM-only property can only be detected if you are logged in as a basic user, which is implicitly documented in the For basic members the print page of a PM-only cache will be mostly empty and contain the following snippet: <div id="pnlErrorMessage">
<p class="Warning">You must be a Premium Member to view this page.</p>
</div> For non PM-only caches it will look like this: <div id="pnlErrorMessage">
</div> I will have a look at updating the test cassettes later. If you are okay with it and you have allowed reviewers to make changes to your pull request (and I manage to get this to work on my side), I can update the corresponding cassettes on your branch for you to check if your changes are okay or need a fix. |
Hmm, yeah, that makes sense. But then, am I supposed to switch between PM and non-PM accounts, while running the tests? Did I overlook that in the test description? |
I have updated the existing cassettes. It seems like If you want, feel free to add a note to the contribution guide that updating the Betamax cassettes requires a basic member account. |
I am leaving the review up to you @FriedrichFroebel. Please ping me as soon as you need a release. Thanks! |
…asic Member account
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your patience.
@tomasbedrich We should be able to release a new version with this change now. This should finally fix the broken RTD landing page as well. |
#179 This PR should fix this issue. Checked other self.name references, but they work fine as is.