Skip to content

Conversation

@wkdewey
Copy link
Contributor

@wkdewey wkdewey commented Aug 20, 2024

contains all the work I've been doing on the API (see #128 which this supercedes)

wkdewey and others added 30 commits December 9, 2022 13:28
this is not the most semantically correct, but it doesn't break anything and orchid seems to expect it
this is necessary to make this sort of query work
@wkdewey wkdewey linked an issue Dec 9, 2024 that may be closed by this pull request
@wkdewey wkdewey linked an issue Jul 25, 2025 that may be closed by this pull request
Copy link
Member

@techgique techgique left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noting what I found before I work on the code to make revisions.

- Make auth code more readable with less string interpolation
  and separate multiple args for methods across lines
- Check if elasticsearch credentials are present to determine whether
  to construct addition auth header and merge with json header
- Compare and match against a fresh `rails new --api` app
- Remove net-smtp gem from Gemfile and Changelog
- Remove extra platforms
Remove old new_framework_defaults initializers for 5.0 and 5.2
Update of Gemfile required logger be added to boot.rb now

Listen gem version was causing Rails app boot to fail.
Hadn't run bundle update again after removing the extra platforms,
so suspect this was the problem we'd seen before with those 🤔
Note production is modified to keep non-public production domains out of
source control
Add back RestClient::BadRequest exception so both display errors
from Elasticsearch in API response rather than returning
an opaque Rails error page
- Fix typo in comment
- Reformat other comments so there is a space between # and text
- Refactor `if nested` code together and bypass extraneous variable
  when value only used once after
  - Likewise for path variable
- Fix indentation of aggregation hash
- Remove extraneous returns
- Remove extraneous use of buckets variable when direct returns work
- Version to 2.0.0
- Config example license and docs links updated. Old year removed from
  `api_update` example
- Changelog review
  - Set version for release and compare url to `v2.0.0`
  - Change explicit code references to `code` rather than "quotes"
  - Capitalize first words, remove trailing periods
  - Remove duplicate entries (`Migration` section, `api_version` added
    to `res`)
  - Drop Rails and Ruby entry as v1.0.5 for v1 already upgraded to them
  - Move text for list item including json snippets to before
    the snippets as Markdown doesn't connect the indented text after
    with prior list items
  - Add json syntax highlighting to json snippets
  - Reorder some of the major change items to the top of sections
  - Add v1.0.5 release that was forgotten with that release
Resolve merge conflicts
- Keep all changes from `local_testing` for Changelog, Gemfile,
  Gemfile.lock
- Others accepted from `dev`
@techgique
Copy link
Member

techgique commented Dec 20, 2025

@wkdewey, please review the changes I've made to double-check I haven't made any mistakes. We might want to test this on the dev server with the dev Rails apps utilizing it as well.

The changes I made to handle Elasticsearch both with and without auth mean we can delete the no_security_testing branch after this is merged and deployed. Since that's what's currently deployed, note the differences between them before I committed changes here after my review and this comparison after my changes were committed. I didn't see anything that should be kept from no_security_testing but let me know if you spot anything.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

2 participants