Repository navigation
Conversation
|
Unlike #654 to #656, this one was never reviewed before, and it asks two open questions (above). I'll merge it only once those two questions are settled; if they are by Monday, 2 November 2026, it goes in that day, right after #654 to #656, rebased onto them. Until then, reviews and answers to the questions are very welcome. |
|
Please create stacked PRs in GitHub (https://docs.github.com/en/pull-requests/how-tos/create-pull-requests/creating-stacked-pull-requests), target |
d60453d to
c389c99
Compare
|
You're right, @jrmajor, sorry for the huge diffs. Pointing to the last commit in the description wasn't enough. I've turned #655, #656 and this PR into drafts. They'll come up one at a time: when the PR before is merged, the next one is taken out of draft, rebased if needed, and then shows only its own changes. So for now only #654 is up for review. |
c389c99 to
6068220
Compare
What is the third question? |
| if ($latitude < -90 || $latitude > 90) | ||
| throw new InvalidParam('latitude', 'latitude must be between -90 and 90.'); | ||
| } | ||
| if ($longitude !== null) { |
There was a problem hiding this comment.
why not checking !== '' also ?
|
|
||
| $size_int = null; | ||
| if ($size2 !== null && $size2 !== '') { | ||
| $size_map = ['none' => 7, 'nano' => 8, 'micro' => 2, 'small' => 3, |
There was a problem hiding this comment.
I'll have to refactor these too sometime :-)
|
I think once implemented, making it work also for OCPL in the future should be a much easier task. |
|
Some concerns from @kojoty here #570 (comment) seem to be well addressed in An update method that required all cache parameters to be present would have posed greater problems. There is another catch: third party apps may implement the functionality in two ways:
I say "apparently" because there is no guarantee that the app won't do some internal processing / conversion on at least some of the fields, though, "apparently" no modification is visible. |
| try { | ||
| # Update caches table | ||
| if (!empty($update_parts)) { | ||
| Db::query("UPDATE caches SET " |
There was a problem hiding this comment.
I presume the "last modified" information is handled by a trigger?
| if ($hint2 !== null && $hint2 !== '') | ||
| $desc_parts[] = "hint = '".Db::escape_string($hint2)."'"; | ||
| if (!empty($desc_parts)) { | ||
| $desc_parts[] = "last_modified = NOW()"; |
There was a problem hiding this comment.
so description gets last modified explicitely
| <p>New cache status. Must be one of: Available, Temporarily unavailable, Archived.</p> | ||
| </opt> | ||
| <opt name='alt_wpts'> | ||
| <p>JSON array of waypoints. Replaces all existing waypoints with the provided list. |
There was a problem hiding this comment.
Please emphasize the "replace"
| { | ||
| if (Settings::get('OC_BRANCH') == 'oc.de') { | ||
| Db::query("DELETE FROM coordinates WHERE cache_id = '".Db::escape_string($cache_id)."' AND type = 1"); | ||
| } else { |
There was a problem hiding this comment.
so this already supports other branches?
| { | ||
| # This is a simplified implementation; actual deletion logic depends on how waypoints are named | ||
| # For now, we'll just log/acknowledge this | ||
| # Full implementation would require matching waypoint names to database records |
There was a problem hiding this comment.
so ... not yet implemented
| <p>JSON array of waypoints to add without removing existing waypoints.</p> | ||
| </opt> | ||
| <opt name='remove_alt_wpts'> | ||
| <p>Pipe-separated list of waypoint names to remove (e.g., "P1|S1").</p> |
There was a problem hiding this comment.
apparently not yet implemented in the code?
|
|
||
| if (Settings::get('OC_BRANCH') == 'oc.de') { | ||
| $type_map = [ | ||
| 'parking' => 1, |
There was a problem hiding this comment.
will refactor this too, in the future :-)
| ) | ||
| "); | ||
| } else { | ||
| $type_map = [ |
There was a problem hiding this comment.
will have to compare OCPL code in detail.
I would leave these else {} branches commented out for now.
| <li>All existing log entries remain in the system as historical records (they are NOT deleted)</li> | ||
| <li>Users can still view the listing and its history (read-only access)</li> | ||
| </ul> | ||
| <p>The term "Delete" here means: retire the cache from active play. The listing |
There was a problem hiding this comment.
I recommend explicitely renaming this method and everything connected to it to "archive".
"Delete" as terminology means something completely different.
| # Set status to Archived. The DB triggers (cachesBeforeUpdate/cachesAfterUpdate) | ||
| # handle last_modified, listing_last_modified, search index updates, | ||
| # and notifications automatically. | ||
| Db::execute(" |
There was a problem hiding this comment.
I presume last modified is handled by a trigger?
| UPDATE caches | ||
| SET status = '".Db::escape_string($archived_id)."' | ||
| WHERE cache_id = '".Db::escape_string($cache['cache_id'])."' | ||
| "); |
There was a problem hiding this comment.
OCDE shows nothing in the logs when archiving? (website logic?)
|
As I commented in the code, please rename "caches/delete" -> "caches/archive". Also, in the spirit of the "archive" method, "caches/enable" and "caches/disable" should be rather easy to implement. Also, should we exclude create and edit, the arhive/enable/disable should be easy to deploy across branches. |
|
Question 1: assuming more is done by OKAPI, some options present themselves. At this time I would opt for b) . There would be more code maintenance overhead, but website has primary control over it's data. Question 2: I would fold into a single method. Existing apps continue to use existing support, they can extend functionality according to available support advertized by the API. Please comment the else part of "if branch = ocde {} else { }" for now, I mean for the initial deployment. |
|
Thanks a lot, @andrixnet, for the careful review. Your questions led to real fixes. They are all in one new commit, e2c6fd7. The PR stays a draft until #654 to #656 are merged. "Three questions": my mistake, sorry. There are only two. The third was about the Question 1: for now, option b: the website keeps its own code and full control, and OC team actions stay out of OKAPI. The PR now follows the website's rules more closely (see below). Longer term I'd like to get to your option c: one place that holds the rules and writes the data, with the websites using it too. Then rules like "a status change writes a log" exist once instead of in the website, in OKAPI and in triggers. That's a bigger discussion than this PR. I've tried to write these methods so they could be that one place later; for example, archive/enable/disable already reuse Question 2: I'd rather not fold
I tested them on OCPL. With the test user as the owner of a test cache:
Sending back unchanged fields: Your inline points:
Also fixed:
Tested with the suite on OCDE and both OCPL installations (MariaDB 10.11 and 10.0), read-only and
My OCDE test setup runs OKAPI without the website, so |
e2c6fd7 to
790589a
Compare
|
I've seen the code being folded into sevearl common functions. Well done. Question 1:
Question 2: On changing status I presume the log is generated using the website default language, - this would be desireable on OCPL, but in OCDE case it should follow the website view the users prefers. (ie generate a log in french, not german, for someone who is using the french view and is a french player). Actually IMO "status=enable" on an available cache should fail verification and return error. Else it really is a spurious log. Example: how many times do I enable an available cache and get log spammed? I haven't seen yet the logs/submit HTML purifier, I am just wandering if it is the same as applied to descriptions by the website or with more or less strict purification. Asking because I had problems with OCPL description HTML purifier (website) killing some actually useful features. How are smileys like : - ) (remove spaces) handled? Don't know the OCDE publishing rules, but OCPL has several configurable rules on how to allow publishing. |
|
Thanks, @andrixnet. Answers in order; the code changes are in 4eb4048, 7ad336a, 142e260 and 487a661. Question 1: agreed: option b, OCDE only for now, and a per-site setting when OCPL wants it. Question 2: agreed. Language of status logs: OKAPI writes no text of its own into a status log. The text is exactly what the owner sends as Spurious status logs: agreed. The three methods now refuse a change to the status the cache already has (
Archived caches: OCDE's website lets owners unarchive (its status menu only hides statuses 4, 5 and 7), OCPL doesn't. So
HTML purifier: cache descriptions in Smileys: both are stored as typed. OCDE turns Publishing rules: OCDE has no rules based on find counts or reviews. Its "hide a cache" form does have rules, and
Descriptions in
Cache types and waypoints: Tested: suite on OCDE 23 passed, 0 failed, 4 skipped. The status logs can't be written on my OCDE setup ( |
Sync with opencaching/okapi (opencaching#667, opencaching#671) and the current opencaching#657 state
|
caches/update potential problem regarding descriptions in practical terms. tags already ? If so, doesn't this make most descriptions non-editable? |
|
@andrixnet: good point, and you're right. I measured it on 526 descriptions of active caches around Stuttgart on opencaching.de:
So with the rule as it was, only about 1% of the existing descriptions could have been changed. Changed in 159ab71: Tested on the OCDE test installation with a cache whose description I set to website-style HTML:
The other 80% stay website-only, by design. The main goal is to edit caches that were created through OKAPI. Whatever an app created, it can correct later, and all of those descriptions are plain. These methods are meant for the simplest possible onboarding app on a phone: hide your first cache in the field, fix a typo afterwards, disable it while the container is missing. They are not a listing editor and won't become one. Formatting, images, attributes and further languages are what the website is for. |
159ab71 to
d481d07
Compare
- services/draftlogs/upload_fieldnotes takes a field notes file as written by Garmin devices and apps (GC log type names, records of other platforms included), maps the log types to the website's draft types and stores the OC records as drafts; it returns their draft_ids. - services/draftlogs/query and services/draftlogs/delete list and remove the user's drafts. - services/apisrv/installation: has_draft_logs, and on OCDE cache_types and log_types. OCDE only: OCPL has no draft logs table, so the services refuse to run there and has_draft_logs is false. Tests: query on the read-only level; upload, query and delete on --write.
log_types is the list services/logs/submit accepts, the same on every installation, not what can be submitted there; field notes use their own log type names. The cache type example is Multi, not Multi-cache.
services/lists/create, update, delete, query, add_caches, remove_caches and get_caches manage the user's cache lists, as on the OCDE website. services/apisrv/installation reports has_lists. OCDE only (the services refuse to run elsewhere; has_lists is false there). Tests: --write creates a list, adds and removes the cache, deletes the list.
…without success flags - ListsCommon: ownership check, the website's name rules (unique per user, 80 characters, 10 for public lists), plain-text description stored escaped (the website keeps HTML), password up to 80 characters for private lists, pipe-separated codes - query: an array as documented; status (0/2/3) instead of is_public, the user's own is_watched, the password of a private list, optional include_caches with cache_codes - create: list_watch (the code read is_watched), list_id only - update: parameters left out keep their value, an empty description removes it, the name rules also on a status change; returns the updated list - add_caches: only caches OKAPI shows, as the website allows only visible caches - get_caches: cache_codes as an array - docs rewritten (links via methodref), suite check extended
services/caches/create, update and delete let an owner manage their own caches through the API: create a cache, change its listing, archive it (delete never removes a cache, it sets the status to Archived). OCDE only; the services refuse to run elsewhere. Tests: --write creates its own test cache, updates it and archives it at the end; on other installations it falls back to writeCache for the per-user checks.
services/apisrv/installation reports has_cache_editing (true on OCDE), as has_draft_logs and has_lists do for their services, so clients can check before offering cache editing. The three methods' docs and their error on other installations point to the flag. Tests: the cache steps of --write run only where the flag is set.
…site's rules - caches/delete is now caches/archive. caches/disable and caches/enable join it. All three publish the owner's status log through logs/submit, so they behave like the existing status logs on every installation. - caches/update no longer changes the status. - Descriptions: plain text is escaped and HTML is purified, as for log comments. The hint is stored HTML-escaped, as on the website. - Values are validated instead of defaulted or clamped. An empty value clears only the short description and the hint. latitude and longitude must be given together. - update writes only values that differ from the stored ones and returns the list of changed parameters. It changes one description language. - remove_alt_wpts removes waypoints by their alt_wpts names. Waypoint types are checked. - create: difficulty, terrain and size are mandatory. It also stores the log password, sets the status from the hidden date, and returns the cache page URL. - Removed: the unreachable non-OCDE code, the success/message fields, and cache_id.
…s comments; create publishes like the website; passwd in update archive, disable and enable refuse a change to the status the geocache already has and send the comment as plain text. create: publish now, later or not now, the website's date rules and its duplicate-coordinates check; plain-text descriptions are stored as HTML like the website's. update: passwd.
… formatting alone create and update take the description as plain text; description_format is gone. update replaces a description only if the stored one is plain, so no client removes formatting written on the website.
Other types are created on the website: on OCDE additional waypoints are always public, so the stages and the final of a multi can't be kept secret here.
…at additional waypoints are public
…te checks three types
…econd); picture skip gone
d481d07 to
c1d2371
Compare
The sixth and last of the changes from OpencachingDeutschland/okapi, cut into separate PRs (see #632).
Draft, stacked on #654, #655 and #656. Those three PRs are also in this branch, because all of them add to
OkapiServiceRunnerand the tests. Once they are merged, I'll take this out of draft and rebase it, so that only its own commits remain.What it is for
The main use case is a smartphone app. A player should be able to play the whole game from the phone, and hiding a cache is part of the game. Out in the field, with the container just placed, the owner needs to:
These methods cover exactly that.
They will never match the website, and they are not meant to. The website stays the place for everything beyond a simple listing. The official Geocaching app makes a similar cut: it takes descriptions in plain text only ("the app does not support rich text formatting") and sends everything else to the website. These methods follow that model:
The methods
services/caches/create(level 3, OCDE only) creates a cache owned by the user, with the rules of the website's "hide a cache" form:publish:now(the default),later(at a given day and full hour), ornotnow, as on the website, for every cache type. Withnotnowthe owner finishes the listing on the website and publishes it there. An unpublished cache isn't visible through OKAPI, not even to its owner, butupdateworks on it;cache_codeand the URL of the cache page.services/caches/update(level 3, OCDE only, owner only) changes those fields of an own cache, and the log password.services/caches/geocachereturns.<p>,<br>) as the website's editor writes a few lines of text. One with more formatting (links, images, bold, lists) is refused and stays as it is.services/caches/editstays exactly as it is.services/caches/archive,services/caches/disable,services/caches/enable(level 3, owner only, every installation) write the status log entry ofservices/logs/submit("Archived", "Temporarily unavailable", "Ready to search"), with its rules. In addition:finalwaypoint gives the final away. The methods can do it; the docs ofcreateandupdatewarn against it. (OCPL can hide waypoints; OCDE can't yet.)has_cache_editinginapisrv/installation, likehas_draft_logsandhas_lists. It coverscreateandupdate. Older installations don't return it; treat that as false.has_cache_editingis false, andcreateandupdatereturn HTTP 400 pointing to the flag. archive, disable and enable work there as OCPL's status logs do. Nothing else changes for OCPL.Same rules in two places (@kojoty's concern in #570): for now, the website keeps its own code and full control, and these methods follow its rules (option b in the discussion below). The longer-term aim is one place that holds the rules for everyone (option c).
Review history: andrixnet's reviews on 2026-10-19 and 2026-10-20 (see the comments below).
Tested with the suite from #653 (this branch, including #654 to #656), on an OCDE installation and two OCPL installations (the official 2018 devel dump with all updates, on MariaDB 10.11 and on MariaDB 10.0), PHP 8.2-FPM behind Apache:
--writeOn OCDE the write run does the following with a test cache:
My OCDE setup runs OKAPI without the website, so it can't write log entries; the status logs were tested on OCPL, with the test user as the temporary owner of a test cache:
disable,enableandarchivewrote log types 11, 10 and 9;A formatted description was refused, and the other fields could still be changed. On every installation, a status change on someone else's cache is refused. All changed PHP files pass
php -lon PHP 7.4. No database changes.@kojoty, @jrmajor, @andrixnet, @stefopl: reviews are welcome. It changes OKAPI itself, so it stays open for review and gives OCPL time to look before anything is merged.