Ahh okay tricky lil thing: if you show the settings page with a partial
change to `AuthUser` that didn't get saved, it can throw off the state
of some stuff. For example, if you don't have a password yet, then
enter a new password but leave the confirmation box blank, then you'll
correctly see "Password confirmation can't be blank", but you'll *also*
then be prompted for your "Current password", even though you don't
have one yet, because `@auth_user.uses_password?` is true now.
In this change, we extend the Settings form to use two copies of the
`AuthUser`. One is the copy with changes on it, and the other is the
"persisted" copy, which we check for parts of the UI that care about
what's actually saved, vs form state.
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.
Simplified this a bit into a helper. It's kinda odd to me, but
convenient for this moment, that Rails allows views to read `params`! I
guess it's for escape hatches exactly like this! lol
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!
I'm getting ready to add handling for "what if you don't *have* a
current password*??", so it seems like the right way to do that is to
just eject the controller and start customizing!
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).
Ahh right, in development `User` and `AuthUser` will have the same ID,
but that got messed up early on for us in production DTI 😅
Here, we switch the form to reference the `User` instead of the
`AuthUser` (to get the ID right), then we also change how we compare
the IDs, because `User#to_param` appends extra text onto the ID after
the number!
Motivation is that I wanna add NeoPass stuff to here! But also like,
it's looked bad for a long time, let's clean it up!! (I just used the
Devise default without any styling at all lol)
Huh, I was writing up an API inventory doc to send to TNT, and was
gonna explain why we proxy these assets… but turns out we don't need to
anymore! Nice!
This is a bit fragile if they ever change their headers, so I'll
mention that in the doc, but for now, yeah sure let's save the planet
some computational effort!
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 beans, I didn't notice when doing my Turbo fixes in
40804c1543, that I had accidentally
prevented Chakra from applying some of its usual global styles! This
caused some minor visual regressions in various parts of the app, e.g.,
the default border color for the search field in the wardrobe app
became way darker.
Here, instead of copy-pasting the styles and missing some parts, we
scope the global styles a bit more carefully: we first use
`extendTheme` with the same code as Impress 2020 to get a good
`globalStyles` function that includes Chakra's defaults, *then* delete
the key from the theme.
Then, in `ScopedCSSReset`, we use code similar to Chakra's own global
style application code: call the `globalStyles` function with the
current theme and color mode, use Chakra's `css` function to convert
values like `green.800` to CSS values, prepend our scoping rule onto
the selectors, and drop it into our Emotion CSS.
Tbh I was worried because when I first noticed this issue while on my
trip, I saw the black item search box border, and was like "ah dang,
did I destroy all the color in the app by breaking the part where
Chakra defines its CSS color variables??" And no, that's not actually
what happened, a lot of the app still had its colors!
So this was less pressing than I had thought, but still good to get
fixed!
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.
Okay, `sub` seems to be a pretty standard place for user identifiers.
Let's start with that assumption! I override the `oauth2-mock-server`'s
default of `johndoe` with `theneopetsteam`, just to be cute :3
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!
Oh right, all these cute overrides should be scoped to the page!
I guess we skipped this because we had pulled this out into a
separate stylesheet. Curiously learning more about how Turbo handles
this kind of thing, like that it doesn't *unload* stylesheets that
*leave* the page when you navigate!
I noticed an issue where Turbo-loading between the Your Items page and
the homepage would clobber each other's copy of jQuery, breaking things
sometimes. e.g. go to Your Items, then go to home, then go to Your
Items, and the page's JS fails because `$.fn.live` isn't defined.
I briefly tested the homepage and it didn't seem to actually depend on
any features from the later version of jQuery? At least not that I
noticed! So I'll just downgrade for consistency. (I also tried
upgrading the Your Items page, but there's too much usage of
`$.fn.live`, which is replaced with a notably different syntax in
jQuery 2.0+.)
First one, Turbo reasonably yelled at us in the JS console that we
should put its script tag in the `head` rather than the `body`, because
it re-executes scripts in the `body` and we don't want to spin up Turbo
multiple times!
I also removed some scripts that aren't relevant anymore, fixed a bug
in `outfits/new.js` where failing to load a donation pet would cause
the preview thing to not work when you type (I think this might've
already been an issue?), reworked `item_header.js` to just run once in
the `head`, and split scripts into `:javascripts` (run once in `head`)
vs `:javascripts_body` (run every page load in `body`).
Finally getting around to this because, with Turbo in play, it applies
to subsequent *pages* too, oops! Previously we at least had the blast
radius of this known issue constrained to the item page itself lol :p
Got some questions in Discord about account unlinking, and seeing
people look ahead to other potential integrations. Want to clarify that
unlinking will work here (barring any surprises!), and that there's no
data sharing _just_ yet!
Someone requested this in Discord, and I figured why not! I'm still
planning to move stuff away from Impress 2020 over time, I just figure
may as well have them more linked while this is still The Reality
This doesn't really matter, I just didn't realize the `.html` part was
optional, and I guess I omitted it here without realizing? But let's
add it for consistency.
The modeling code is slow! I think in production it's being cached, and
tbh I though I had development mode caching turned on over here, but
it's quite evidently _not_ doing it if so, so. Okay! Skip for now.
