I think the Rails query cache handled these anyway? But `SwfAsset` has
a `before_save` hook that checks its zone's info, and
`SwfAsset.preload_manifests` saves all the assets, on the assumption
that saving is a no-op when the record didn't change anyway. And it
basically is!
But I figure that, now that I'm realizing hooks exist, simply not
attempting to save unchanged records is probably a better
representation of what we intend to do. So I'm fixing it like that!
Another potential fix would be to preload the zones for these assets,
but I think that confuses the intent too much; the method itself isn't
using the zones, it's just a weird incidental thing that a save hook
happens to use. (Would probably be better to refactor this old save
hook into a different situation altogether, but that's for another
time!)
This is a minor nbd change, I just noticed when playing around in the
console that, unlike most other errors for this model, the `body_id`
being required is _only_ enforced in the database schema, so it isn't
returned with the usual errors. Not a big deal! Just feels like this is
clearer to work with, and more correct to what we *intend*.
This is just a bit of future-proofing! We also add a default thumbnail
URL of the cute "Neopets Circle Background", for cases where the series
name isn't known yet.
Now, for colors like Mutant or Magma where there's no paint brush
image to show, we use a sample pet image instead, to help it have equal
visual weight and clarity as the cases with the paint brushes.
We do some cleverness in here to make sure to always show the relevant
species, if possible!
NC prices, some CSS, and also a new application-level helper that adds
a feature I've long wanted and been working around for Turbo: the
ability to specific that a stylesheet is specific to the current page,
and should be unloaded when removed!
I use this to write `sources.sass` without the usual
`body.items-sources` scoping that we've historically used to control
what pages a stylesheet applies to. (In the long past, this was because
a lot of stylesheets were—and still are–routed through the
`application.sass` stylesheet! But even for more recent standalone page
stylesheets, I've done the scoping, to avoid issues with styles leaking
beyond the page they're meant for when Turbo does a navigation.)
This'll affect the recommended acquisition method by a lot!
NC Mall info like current price isn't surfaced anywhere else in the app
right now. It'd probably be good to add to the item page, and maybe
some other places too!
Currently we only load the homepage, so there's only actually one
wearable item to sync up! But here's the task to do it!
To do this, we also created the backing model NCMallRecord, where we'll
save the current NC Mall state!
TNT requested that we figure out ways to connect the dots between
people's intentions on DTI to their purchases in the NC Mall.
But rather than just slam ad links everywhere, our plan is to design an
actually useful feature about it: the "Item Getting Guide". It'll break
down items by how you can actually get them (NP economy, NC Mall,
retired NC, Dyeworks, etc), and we're planning some cute actions you
can take, like shortcuts for getting them onto trade wishlists or into
your NC Mall cart.
This is just a little demo version of the page, just breaking down
items specified in the URL into NC/NP/PB! Later we'll do more granular
breakdown than this, with more info and actions—and we'll also like,
link to it at all, which isn't the case yet! (The main way we expect
people to get here is by a "Get these items" button we'll add to the
outfit editor, but there might be other paths, too.)
Idk how we got into this state, or if it's environment-dependent or
MySQL-version-dependent or what, but setting up the dev environment on
my macOS machine is complaining that `TEXT` columns can't have default
values.
Well, in that case, let's just have it be a non-nullable field, and add
a note to our code that missing fields *can* cause item saving to fail!
(This was always true, but I'm just extra-noting it because it's
becoming *more* true.)
Simple enough to start! If `shadowbanned: true` gets set on a user,
then we show a 404 instead of the actual list page, *unless* you're
logged in as that user, or coming from a known IP of that user.
This isn't a very strong mechanism! Just something to hopefully
increase the costs of messing around with list spam.
This hasn't been causing issues as far as I know, I just noticed
*months ago* that I forgot to do this, and have had a sticky note about
it on my desk since then lol.
I tested this by temporarily setting the timeout to `0.5`, and watching
it fail!
A further optimization, this lets us use the image hash as the new hash
for the pet type if it would be useful! (whereas before this change,
we'd dip into `fetch_metadata` and just get back `nil`, which was okay
too but a little bit less helpful!)
Ahh, we recently added a step to pet loading that sends a metadata
request to `PetService.getPet`, which is now (in a sense, correctly!)
raising a `PetNotFound` error when we try modeling with a "pet" that
starts with `@` (a trick we use in situations where we can get an image
hash for a modeling situation, but not an irl pet itself).
In this change, we make it no longer a crashing issue if the pet
metadata request fails: it's not a big deal to have a `PetType` have no
image hash or not have it be up-to-date!
In the next change, I'll also add an optimization to skip fetching it
altogether in this case—but I wanted to see this work first, because
the more general resilience is more important imo!
Yay, we got the API endpoint for this! The `linkage` scope is the key.
Rather than pulling back the specific fallback behavior we had wrote
for usernames before, which was slightly different and involved
appending `neopass` in there too (e.g. `matchu-neopass-1234`), I
figured let's just use a lot of the same logic, and just use the
preferred name as the base name. (I figure the `neopass` suffix isn't
that useful anyway, `matchu-1234` kinda looks better tbh! And it's all
fallback stuff that I expect serious users to replace, anyway.)
Oh right, I never did catch this when setting up User-Agent in the app!
(I noticed this because I'm making a new request now, and went to look
how we set it in previous stuff, and was like. Oh. We don't anywhere
right now. Interesting LOL)
Oh right, if you can remove your email, there's a way to fully lock out
your account:
1. Create account via NeoPass, so no password is set.
2. Ensure you have an email saved, then disconnect NeoPass.
3. Remove the email.
4. Now you have no NeoPass, no email, and no password!
In this change, we add a validation that requires an account to always
have at least one login method. This works well for the case described
above, and also helps offer server-side validation to the "can't
disconnect NeoPass until you have an email and password" stuff that
previously was only enforced by disabling the button.
That is, the following procedure could also lock you out before,
whereas now it raises the "Whoops, there was an error disconnecting
your NeoPass from your account, sorry." message:
1. Create account via NeoPass, so no password is set.
2. Ensure you have an email saved, so "Disconnect" button is enabled.
3. Open a new browser tab, and remove the email.
4. In the original browser tab, click "Disconnect".
Ah okay, if you leave the password field blank but don't have one set,
our simple `update` method gets annoyed that you left it blank.
In this change, we simplify the model API by just overriding
`update_with_password` with our own special behavior for the
no-password case.
including validation logic to make sure it's not already connected to
another one!
The `intent` param on the NeoPass form is part of the key! Thanks
OmniAuth for making it easy to pass that data through!
Ahh I see, if you do a no-op update, it still clears the
`previously_new_record?` state, so our NeoPass controller thinks this
account already existed. Instead, let's only do this update if it's an
account that already exists, instead of depending on the no-op-iness!
That is, you're required to add a password *or* an email before
disconnecting your NeoPass, but idk, I think it's rude to demand an
email from someone for the sake of *disconnection*. Email is no longer
required for accounts that already exist!
This is more consistent with the `uses_omniauth?` we already have, and
it also will help for the next change, where I want a `uses_password?`
method (and using the name `password?` breaks some of Devise's
validation code).
Previously, the way we loaded the image hash for a given pet was to
navigate to `https://pets.neopets.com/cpn/<pet_name>/1/1.png`, but
*not* follow the redirect, and extract the image hash from the URL
where it redirected us to.
In this change, we refactor to use the AMFPHP RPC `PetService.getPet`
instead. I don't think it had this data last time I looked at it, but
now it does! Much prefer to use an actual RPC than our weird hacky
thing!
(We might also be able to use this call for other stuff, like
auto-labeling gender & mood for pet states, maybe?? That's in this data
too! We used to load petlookups for this, long long ago, before the
petlookup captchas got added.)
I guess this was like, we had some call site that was handling loading
the viewer data itself, and didn't want to have to reload it?
But whatever, not used now, let's simplify! We can rebuild this easily
if we need it again.
Locale is the big one that's not really relevant anymore (I don't want
to be loading non-English item names anymore, now that we've simplified
to only support English like TNT has!), but there was also `item_scope`
and stuff.
The timeout option is technically not used in any call sites, but I
think that one's useful to leave around; timeout stuff is important,
and I don't want to rewrite it sometime if we need it again!
Just a small thing, I guess when I was a kid I did a weird thing where
I attached `origin_pet` to `PetType`, then upon saving `PetType` I
loaded the image hash for the pet to save as the pet type's new image
hash.
I guess this does have the nice property of not bothering to load that
stuff until we need it? But whatever, I'm moving this into `Pet` both
to simplify the relationship between the models, and to prepare for
another potential refactor: using `PetService.getPet` for this instead!
Ahh, I had assumed the `uid` provided by NeoPass would be the user's
Neopets username, but in hindsight that was never gonna work out since
NeoPass doesn't think of things in terms of usernames at all!
For now, we create 100% random NeoPass usernames, of the form
"neopass-shoyru-5812" or similar. This will be an important fallback
anyway, because it's possible to have a NeoPass with *no* Neopets.com
account attached.
But hopefully we'll be able to work with TNT to request the user's main
Neopets account's username somehow, to use that as the default when
possible!
Ah right, I went and checked the Devise source code, and the default
implementation for `password_required?` is a bit trickier than I
expected:
```ruby
def password_required?
!persisted? || !password.nil? || !password_confirmation.nil?
end
```
Looks like `super` does a good enough job here, though! (I'm actually
kinda surprised, I wasn't sure how Ruby's `super` rules worked, and
this isn't a subclass thing—or maybe it is, maybe the `devise` method
adds a mixin? Idk! But it does what I expect, so, great!)
So now, we require the password if 1) Devise doesn't see a UI reason
not to, *and* 2) the user isn't using OmniAuth (i.e. NeoPass).
This had caused a bug where it was impossible to use the Settings page
*without* changing your password! (The form says it's okay to leave it
blank, which stopped being true! But now it's fixed!)
Whew, exciting! Still done nothing against the live NeoPass server, but
we've got this fully working with the development server, it seems!
Wowie!!
This is all still hidden behind secret flags, so it's fine to deploy
live. (And it's not actually a problem if someone gets past to the
endpoints behind it, because we haven't actually set up real
credentials for our NeoPass client yet, so authentication will fail!)
Okay time to lie down lol.
In this change, we wire up a new NeoPass OAuth2 strategy for OmniAuth,
and hook up the "Log in with NeoPass" button to use it!
The authentication currently fails with `invalid_credentials`, and
shows the `owo` response we hardcoded into the NeoPass server's token
response. We need to finally follow up on the little `TODO` written in
there!
If you pass `?neopass=1` (or a secret value in production), you can see
the "Log in with NeoPass" button, which currently takes you to
OmniAuth's "developer" login page, where you can specify a name and
email and be redirected back. (All placeholder UI!)
We're gonna strip the whole developer strategy out pretty fast and
replace it with one that uses our NeoPass test server. This is just me
checking my understanding of the wiring!
This is setting us up for NeoPass, but first we're just gonna try stuff
with the "developer" strategy that's built in for testing, rather than
using the NeoPass dev server!
When we moved more logic into the main app, we made some assumptions
about manifest art that were different than Impress 2020's, in hopes
that they would be More Correct for potential future edge cases.
Turns out, they were actually *less* correct for *current* edge cases!
Chips linked us to a few examples, including this Reddit post:
https://www.reddit.com/r/neopets/comments/1b8fd72/i_dont_think_thats_the_correct_image/
Fixed now!
Oh rough, when moving an item list entry from one list to another, our
logic to merge their quantities if it's already in that list was just
fully crashing!
That is, moves without anything to merge were working, but moves that
required a merge were raising Internal Server Error 500, because the
`list_id` attribute wasn't present.
I'm not sure why this ever worked, I'm assuming using `list_id` in the
`where` condition would include it in the `select` implicitly in a
previous version of Rails? Or maybe Rails used to have fallback
behavior to run a second query, instead of raising
`MissingAttributeError` like it does now?
Well, in any case, this seems to fix it! Whew!
Yay, we finally added it, the part where we include the appearance data
for the items based on both the species/color and the alt style! Now,
switching to Faerie Acara correctly filters the search only to items
that would fit (I think literally just only body_id=0 items right now,
but we're not banking on that!)
