Here, I remember the trick I learned when building the outfit viewer:
web components are great for making sure stuff stays initialized well
in a Turbo environment!
The problem was, after submitting the form and getting a new preview
loaded via Turbo, the part where we remove `inert` would get undone.
Additionally, this script only loads *once* per session, so if you
Turbo-nav to a different item then that part of the page never ran.
Instead, we use web components to remove the attributes on mount, then
again if they're ever reapplied by Idiomorph.
We mark the options as `inert` and `aria-hidden` while the JS is still
loading—and if the `noscript` tag tells us it's never coming, it covers
up the picker with a brief explainer!
The basics are working great! There's a few known missing things though:
- Add reasonable noscript behavior
- Disable options where there's no valid appearance
- Lay it out actually _good_, instead of just images dumped there
Adapting what the Impress 2020 UI does, but in Ruby instead!
I feel like this is case is really starting to show the power of doing
this stuff in Rails instead of via an API… we can *really* take
advantage of our models and our handy idioms at all points. This is
just so much less *code* than this feature takes in Node + GraphQL +
React.
We used to use this to determine what color to show by default on the
item page preview for, like, Maraquan-specific items. Now, we infer it
from our actual customization data, rather than these heuristics!
There's still a database field for `Item#manual_special_color_id`. We
can still read and write this from the support UI, and Impress 2020
still slightly uses it from the homepage, so I'm not removing from the
database right now.
The `build_on_pet_types` helper used to be reused on the items page, to
generate the list of species to display. We don't use it anymore, so
simplify and remove!
I'm about to reimplement the more-robust version of what this used to
be: how the item page used to say "sometimes" after certain zones in
the occupied list.
Now, we're going to do parity with 2020, and list the actual species!
I like that this takes away the weird `#sometimes` method on the `Zone`
class, which was always an odd hack for just this small thing.
We had this issue on Impress 2020 and I fixed it over there too. I guess
it went less noticed here on Classic, because it's a more
progressively-enhanced site in general (and this failure case is an
interesting argument for that architecture! lol).
On Impress 2020, I wasn't sure if the "waits for the document to load"
behavior of the `defer` attribute was necessary to the script, so I
chose to keep `defer` but move it _after_ the other scripts.
This time, I dug in a bit more, and found a Plausible author saying
that the choice was kinda arbitrary; and another person who had the
same issue as me, who said they switched to `async` and it worked well.
So, that's what we're doing now, too!
https://github.com/plausible/analytics/discussions/1907#discussioncomment-2754499
Closes#2, after making some tweaks to the PR to fit how JS templating
works here. Thanks @dice!!
I had to move `petThumbnailUrl` out of the closure, because this script
does a cute thing of having separate variable scopes for the separate
areas of the page—but this is used by two of them. Arguably it could
make sense to like, put this all in one larger shared IIFE closure that
wraps both of them, to preserve some of this code's intention of
avoiding adding to the global namespace on this page, but like.
*It's fine.*
Co-Authored-By: Steve C <diceroll123@gmail.com>
We were previously planning a more interesting "Add to Cart"
integration with TNT, but it hasn't panned out! For now, we'll just
link to the NC Mall homepage.
Two reasons for this new title:
1. The pitch for "Get these items!" is weaker, now that we're not
getting the power-user integrations we'd planned around.
2. I literally only just thought of it now!
Oh right, if the *label* is `:active`, that only applies if we're
clicking it with our mouse. But if the *toggle* is actively, that
applies both to mouse events on the label, and keyboard events on the
checkbox.
Specifically, if a movie layer was the top layer, the `cursor: wait` on
the preview wouldn't show, because the iframe's *contents* would take
priority, and they were using the default cursor.
I thiiiink I've seen the status of a movie `<outfit-layer>` sometimes
be `loading` even when it's clearly already loaded and running. I
haven't been able to track down where and how that happens exactly, so
this is me acting on a hunch: that maybe the
I-would-have-thought-very-unlikely event that the iframe finishes
loading before the `<outfit-layer>` connects with its children maybe
happens more often than one might think!
In this change, we set up the iframe to receive `requestStatus`
messages, which it responds to with the status immediately. And we send
one of these when the `<outfit-layer>` first discovers the iframe.
Fingers crossed!
This doesn't do a good job maintaining state across morphs, but hey
it's Working At All in terms of wiring, and that's good!!
Also need to style up the toggle as a cute button instead of a visible
checkbox and the words "Play/pause"!
This is a nice extra error handler to have, but note that it *won't*
catch the case where the iframe successfully loads but the page returns
a bad status code. In this case, we'll just show the loading state
forever.
Add some unobtrusive white background for contrast, show it when the
Turbo frame is loading too, add a spinner cursor, and fix a silliness
of how we put the `position: absolute` stuff into the component-y part
of the hanger spinner instead of this specific use case lol oops!
Oh oops, this is the first script on the page with the `defer`
attribute, which means it needs to run before other scripts with
`defer`—and in this moment, it's not loading for me, which means the
pages aren't working!
I assume Plausible told me to use `defer` rather than `async` because
it expects the page to be ready; okay! Let's just move this to the
very body of the `<head>` instead, so it isn't taking priority over
anything else.
This doesn't actually really matter, because this doesn't actually get
used in the app right now? But I figure, hey it's not hard to maintain,
let's just do it for consistency!
Oh, I didn't realize the `_elem` variant of these parts of the
`Content-Security-Policy` is newer, and so doesn't even work on my
current version of Safari on my Mac.
My rationale at the time was: `script_src_elem` is stricter against
things like imports, and I figured, ok let's do the strictest policy
that works. But since it's not fully compatible with browsers even
*I'm* using right now, and I'm not aware of an actual problem it would
prevent, let's back off that a bit! This should have the same effective
security properties for our case.
Note that the effect of this compatibility issue wasn't *weakening* the
policy; it was being *too* strict, by blocking the scripts and the
stylesheets. This is because `script_src_elem` was ignored, and
`script_src` was absent, so it fell back to `default_src none`.
The most notable thing here is that we keep the movie iframes running!
So if you're trying different pets for an animated item, the animation
keeps going while the new pet layers load alongside it.
This is also nice for like, the species/color picker form, so we're not
taking away input elements from people who depend on e.g. keyboard
focus.
Not using this on the item page preview yet, but we will!
I like this approach over e.g. a web component specifically for the
sandboxing: while I don't exactly *distrust* JS that we're loading from
Neopets.com, I don't like the idea of *any* part of the site that
executes arbitrary JS unsafely at runtime, even if we theoretically
trust where it theoretically came from. I don't want any failure
upstream to have effects on us!
I copied basically all of the JS from a related project
`impress-media-server` that I had spun up at one point, to investigate
similar embed techniques. Easy peasy drop-in-squeezy!
Oh right okay, attributes like `status="loading"` are more of an API
for the caller, whereas the internal state API is where you wanna put
things that are meant to be used in CSS selectors and stuff.
Instead of doing all this listening to Turbo events etc to know when
outfit layers might have changed, making it a custom element and wiring
in the behavior to its actual lifecycle makes it always Just Work!
The UI for it is just basic for my own testing rn: it sets the preview
background to gray while loading, then back to white when done!
This uses the new CSS `:has()` selector: we have JS manage the loading
state on each layer, then the container just restyles itself based on
whether any currently-loading layers are present.
Also adapted from the Impress 2020 logic!
Note that I refactored `compatible_pet_type` to a series of scopes on
`PetType`. I think this is a simpler, clearer, and more flexible API!
This is a cute thing that I think sets us up for other stuff down the
line: move more of the outfit appearance logic into the `Outfit` class!
Now, we set up the item page with a temporary instance of `Outfit`,
then ask for its `visible_layers`.
Still missing restricted-zones logic and such, that's next!
Nice, gotta say, this is a pretty neat way of making things feel more
app-y! There's some missing pieces here about like, loading state etc,
but the vibes are pretty good, and the implementation was dead-easy!
Just stripping out the big React component, and having Rails output it!
There's a lot of work rn in extracting the Impress 2020 dependency from
the `wardrobe-2020` React app, and I'm just curious to see if we can
simplify it at all by pulling this stuff *way* back to basics, and
deleting the item page part of `wardrobe-2020` altogether.
In this draft, we regress a lot of functionality: it just shows the
item on a Blue Acara, with no ability to change it! I'm gonna play with
putting more of that back in.
I also haven't actually removed any of the item page React code; I just
stopped calling it. That can be a cleanup for another time, once we're
confident in this experiment!
Oh huh, TIL in Ruby `^` *always* means "start of line", whereas in many
languages' regular expression engines it means "start of string" unless
you enable a special multiline flag for the pattern.
I've fixed this in a number of expressions now!
I'm noticing this in the context of doing some security training work
where this the cause of a sample vulnerability, but, looking at our own
case, I don't think there was anything *abusable* here? But this is
just more correct, so let's be more correct!
I think the parens are silly now that this paragraph is just kinda all
bonus clarification info anyway. And I wanted to explain the cost
computation for the potions, and highlight the bundle thing!
If the item names are long, it helps to give them more room to breathe!
Whereas if they're short, it looks silly and makes it harder to scan
the table.
Just an extra bit of help for e.g. Dyeworks items with long names!
Huh, I thought I'd tried some invalid dates and they gave me
*surprising* output instead of raising an error. Well, maybe it can do
both, depending on exactly the nature of the unexpected input?
In any case, I found that a bad month name like "UwU" raised an error.
So, let's catch it if so!
Oh right, if I assume "date in the past means it's for next year", then
that means that, when the date *does pass*, we won't realize it!
e.g. if Owls says "Dyeable Thru July 15", then on July 14 we'll parse
that as July 15, 2024; but on July 16 we'll parse it as July 16, 2025,
and so we'll think it's *still* dyeable. Under this logic, it's
actually impossible for a limited Dyeworks date to *ever* be in the
past, I think!
I think 3 months is a good compromise: it gives Owls plenty of time to
update, but allows for events that could last as long as 9 months into
the future, if I'm doing my math right.
The table layout algo can get a bit funky about how it assigns extra
space, I want to encourage things like "Total: 5 items" etc not to
wrap, esp in the Dyeworks case where it's quite long!
There's more and more going on in here! Let's omit the base item name,
increase the table width a bit in this case, and tweak the rest a bit
while we're here.
I uhhh literally didn't know Dyeworks was a gacha system until Kaye
from the Owls team told me lmao
I should maybe uhh read more guides instead of assuming I've osmosed
things correctly oops!
Ahh, I started a tabs-y file (as I default to these days), but copied
code from a spaces-y file, and didn't notice. (My laptop editor isn't
configured to flag this for me, oops!)
Fixed!
There's just starting to be a lot going on, so I pulled them out into
here!
I also considered a like, `Item::DyeworksStatus` class, and then you'd
go like, `item.dyeworks.buyable?`. But idk, I think it's nice that the
current API is simple for callers, and being able to do things like
`items.filter(&:dyeworks_buyable?)` is pretty darn convenient.
This solution lets us keep the increasing number of Dyeworks methods
from polluting the main `item.rb`, while still keeping the API
identical!
Silly mistake, right, we might not have a trade value listed! This is
relevant for the new Dyeworks items that just came out like a few hours
ago, which Owls doesn't have info for yet.
Lmao I've been testing with an outfit that has all the kinds of items,
so I didn't notice that this new refactor to `@items[:dyeworks]` style
of tracking the items returns `nil` when there's none, instead of `[]`.
(I always make this mistake when I use `group_by` lmao sob)
In this change, we give the `@items` hash a default value, so that will
stop happening!
Oh right, this previously logic was silly: we can't count on the
*interval itself* to be reliably resetting the FPS counter state,
because the interval might not be firing!
I think this fix worked when I tried brief tests, but didn't work when
I did an (accidental) longer test, because the browser switched to a
more aggressive throttle mode, and the previous mode was close enough
on the resets for it to be fine, whereas this time the FPS counter
state got way too old.
Now, we reset the FPS counter state *exactly* when the page comes back.
We have a feature to check the movie's FPS, and pause it if it gets too
low, as a guard against especially low-performance movies. But this was
triggering in an *expected* case, where browsers intentionally throttle
interval events when a page is in the background (e.g. you switch to
another tab).
Now, our rendering is aware of page visibility: when the page is
hidden, don't bother rendering, and keep resetting the FPS counter
state, so that we can pick up with a fresh FPS counter when the page
comes back.
Oh weird, even with `flush: true`, `content_for` will ignore an empty
block and *not* flush out the previous content. This could cause rows
whose subtitles *should* have been empty (e.g. no NC trade value) to
display the previous row's value instead.
Let's make this whole situation a bit more robust by having the
*template* clear out the subtitle right before calling the block. That
way, a previous row's value *can't* get in, no matter what.
I think it helps a bit to have only the label be dotted-underlined, to
hint that I'm offering help about what that *means*, but clear the way
for the value itself to be more visible and less cluttered.
I thought to myself, "I wonder if it's possible to use a sneaky hacky
`content_for` trick to be able to run this code in the template." And
indeed it is!
It's tricky cuz like, I want to render this template, and I want to
provide _multiple_ slots of content to it. So, in this variant, we keep
the block as being primarily for the actions, but also optionally
accept `content_for :subtitle` inside that block, too.
Executing that correctly is a bit tricky! The subtitle comes *before*
the actions. So, we `yield` the actions block immediately, save it to a
variable, and *then* get the subtitle block.
Previously, I added a Dyeworks section that was incorrect: the base
item being available in the NC Mall does *not* mean you can necessarily
dye it with a potion!
In this change, we lean on Owls to tell us more about Dyeworks status,
and only group items in this section that Owls has marked as "Permanent
Dyeworks".
We don't have support for limited-time Dyeworks items yet—I've sent out
a message asking the Owls team for more info on what they do for those
items!
I started writing this up, then sent a preview to a friend, and he was
like "oh cool, but also this is not correct?"
I didn't realize Dyeworks has limited-time support to be *able* to dye
certain items. Hey, glad we're writing this guide for people like me,
then! lol
I wonder if we can lean on Owls for this. It seems like they already
list "Permanent Dyeworks" for some items, I wonder if they say
something special for active limited-edition Dyeworks items!
In this change, instead of *always* inferring the Dyeworks base item
from the item name at runtime, we now have a database field that tracks
it, and auto-populates whenever an item *seems* to need a Dyeworks base
item but doesn't have one yet.
This will enable us to set the base item manually in cases where it
can't be inferred, and load Dyeworks base items for the Item Getting
Guide in one query with `includes(:dyeworks_base_item)`.
This migration does a bit more of the fix-em-up scripting work *in* the
migration itself than I usually do, mainly because there's so much in
this one that I think being extra-explicit is useful. We make sure to
do it gracefully though!
This works for most of the current 1,094 Dyeworks items! But there are
a few exceptions, for cases where the base item name is not quite the
same (e.g. the Dyeworks version is more concise). Maybe we'll add a
database field to override this?
- Dyeworks Baby Blue: Baby Valentine Jumper
- Dyeworks Baby Pink: Baby Valentine Jumper
- Dyeworks Black: Field of Flowers
- Dyeworks Black: Games Master Challenge 2010 Lulu Shirt
- Dyeworks Blue: Field of Flowers
- Dyeworks Blue: Stars and Glitter Facepaint
- Dyeworks Brown: Hanging Winter Candles Garland
- Dyeworks Green: Stars and Glitter Facepaint
- Dyeworks Magenta: Lovely Berry Blush
- Dyeworks Orange & Pink: Winter Lights Effects
- Dyeworks Orange: Games Master Challenge 2010 Lulu Shirt
- Dyeworks Peach: Lovely Berry Blush
- Dyeworks Purple: Baby Valentine Jumper
- Dyeworks Purple: Games Master Challenge 2010 Lulu Shirt
- Dyeworks Purple: Hanging Winter Candles Garland
- Dyeworks Purple: Stars and Glitter Facepaint
- Dyeworks Red & Green: Winter Lights Effects
- Dyeworks Silver: Hanging Winter Candles Garland
- Dyeworks Soft Pink: Lovely Berry Blush
- Dyeworks Yellow & Magenta: Winter Lights Effects
- Dyeworks Yellow: Field of Flowers
We could do a whole thing about like, checking singular vs plural, but
I'd rather just keep it simpler; I think it's clear from context that
we're talking about a category, so plural is fine even if it's not
actually more than one.
Okay so, like 30 minutes ago I added fallback behavior for cases where
we can't correctly infer the color from a PB item's name… and then I
pulled it up in the color and found that, oh, right, there are already
3 PB items that *correctly* return `nil` for `Item#pb_color`: Aisha
Collar, Elephante Hat, and Ixi Collar.
This is because they're common items that apply to many colors, like
the basics, but also many other less-special or older color variants.
They are the most likely case where we'll return `nil`.
So, I've updated our fallback UI to, instead of talk vaguely about
missing data, just assume that we're dealing with basic items. In the
rare window of time where a new color is released, and we have PB items
for it but no manual color data yet, this can just incorrectly say
"Basic Colors" and that's fine.
This is less likely than the newly-released color case for PB items,
but I figure let's be resilient anyway, especially since it's so easy
to—and also I figure this is less likely to be triggered by an *actual*
new species, and more likely to be triggered by a surprise in an item's
naming conventions.
But yeah, if `Item#pb_species` returns `nil` upstream, it'll be passed
to `Color#example_pet_type`, which will crash trying to read its ID. So
in this change, we update `Color#example_pet_type` to accept a `nil`
value, and fall back to the first Species (Acara) in that case.
This means that, if you e.g. take the Mutant Aisha Collar and delete
the word "Aisha" from the name, then load it in the Item Getting Guide,
you'll see a thumbnail of a Mutant Acara. Good enough!
Oh right, it's possible for `Item#pb?` to return true, but
`Item#pb_color` to return `nil`, if the item has the paintbrush item
description but we can't find a color whose name matches the item name.
This would be expected if a new color were added to Neopets, and PB
items for it were modeled by the community, but we hadn't manually
added the color to the database yet.
Previously, the Item Getting Guide would crash in this scenario. Now,
it correctly handles the possibility of a `nil` value for `pb_color`,
and shows some placeholder info.
To test this, I temporarily edited some item names to not contain the
color name anymore (e.g. "P-rate Elephante Shirt and Vest"), then
loaded the guide and made changes until it no longer crashed.
I noticed in the app that these queries were slowwww! I was able to
track it down to a bad query plan, as we explain in the comment.
I searched online for "mysql query performance filter on one join table
sort by another", and was surprised to find this answer suggest a
subquery, which I've often been told to expect to be slower compared to
joins? But it certainly worked in this case!
https://stackoverflow.com/questions/35679693/mysql-optimize-join-with-filter-and-order-on-different-tables
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.
Make them the same size, add spacing between them, and also put the
"Get these items!" on the right, because the list is right-aligned and
the Save button has dynamic width (the save vs saving vs saved states),
so this makes things a lot more consistent and stable!
A funny table-layout bug, where the item "Portal to the Unknown" had a
very long Owls listing ("Owls listing: Buyable - Magic Lens + Blank
Grey Tome (NP)"), and so the table layout tried to give it more room by
decreasing the width of the action cell and wrapping the "NC Trades"
action button text onto multiple lines.
The fix: don't allow that! The table layout will figure out how to
handle this being disallowed, and give the actions cell an appropriate
minimum width.
You can now go to `/outfits/new?features=get-these-items` to start
seeing the "Get These Items!" button in the outfit editor!
I haven't tested it a ton yet, but yeah here it is!
Oh oops, I added an optional `subtitle` argument for the item table
list row template, but didn't realize that it would crash in cases when
not provided! Now, we add an initializer to set it to `nil` if
undefined.
This doesn't matter a ton in production, where we already have most of
our manifests loaded! But it matters a lot on my relatively-fresh
development instance, at times like now when images.neopets.com is slow
to respond. A single item search was taking minutes before this change
(5 seconds of timeout per asset for 30 items!), but now takes a few
seconds the first time, as it should!
Note that there's a known performance issue here: we should try to
fetch all the OWLS values at once, instead of doing them in sequence
while rendering the page!
Oh jeez, idk why this was ever in here, but yeah no, I want to be using
default browser focus outlines unless specifically overridden otherwise.
Will help keyboard navigation a lot! Yikes!!
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!
Ohh yeah, this helps communicate the process much better, especially
for what the Shops/Trades links mean.
I think I'm gonna also go get the paint brush thumbnail images and add
them to the database too, to help better communicate that this is a
paint brush item situation.
Clearer focus states, row changes bg color on hover/focus to help you
track where you are, remove the redundant image thumbnail link from the
tab order (it's the same as the item name link!)
As part of this, I added a new `search_icon` helper, and a new
`button_link_to` helper, which both styles the link as a button and
accepts an `icon` parameter to make it easier to pass in an icon!
I'm not sure when TNT changed this, but the Shop Wizard has a new
canonical URL now! The previous one redirects, but it does *not* pass
along query string parameters, so these buttons would correctly go to
the Shop Wizard but not auto-fill the name!
Now, we go directly to the new canonical URL, with query params in hand!
This is just a tech update: instead of using hand-built URLs with
`CGI::escape`, I use `Addressable::Template`, which is a more reliable
way to build URLs in general.
The motivation here is that I noticed the Shop Wizard link is actually
broken! And I wanted to fix this while I was here, but I figured let's
split that into a separate commit than this refactor. See next!
This helps us be more efficient with our use of space, keep the CTAs well
aligned, show a clear total, and set up how we might do CTAs for more complex
cases like all the potential Neopoint CTAs like Wiz/Trades/Auction/etc!
Now that we have this helper, we no longer need these stylesheets to
include a `body.controller-action` wrapper to scope all the styles!
Someday we should convert more of our stylesheets to this format,
instead of slamming them all into `application.sass` like we do now.
Ah, well!
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!
This doesn't connect to anything yet, I'm just doing the beginnings of
loading NC Mall item data!
My intent is to run this regularly to keep our own NC info in the
database too, primarily for use in the Item Getting Guide. (Could be
useful to surface in other places too though!) This will help us split
items into those that can be one-click purchased with the NC Mall
integration, vs NC items that need to be acquired by other means.
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.)
Oops, prior to this commit, searching for "white peach" would return
nothing, whereas now it correctly returns the "Dyeworks White: Just
Peachy Filter", like if you search in the Infinite Closet!
This solution is a bit hacky, wrote some more in the comments about how
to maybe do this better!
Oops, right, this meta tag that runs on all pages currently crashes if
we can't read the credentials file!
Instead, let's just allow this value to be `nil` if not present.
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.)
I refresh the image and UI color here to draw attention to the change!
I also delete the `neopass-thumbnail.png` image, since it's no longer
used anywhere anymore, but I would not be surprised if we want it back
someday and need to revive it from history!
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.
Just a lil blurb to make sure it's clear that NC sales and stuff are
forbidden! I imagine the people doing it know this, but I want to make
sure we're being explicit, in case there's any element of
miscommunication.
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!
In particular, we got feedback that it was surprising to not get to
check which NeoPass you wanted to use, and that the permissions were
never prompted again. I figure let's err on the side of ample clarity!
As part of this, I've added the new `external_link_icon` global helper,
which embeds an SVG from Chakra UI. That's just the convenient place I
know to grab that icon, and I did it this way instead of an `img` tag
because that enables the `currentColor` thing to work instead of coming
out black!
Not getting a lot of takers, I think it was wise to start small just in
case, but there doesn't seem to be a floodgate problem, so let's remove
the limitations and increase the ask! (But still not a full launch yet,
because I want to funnel people through the feedback process first.)
Got the icon and background style from Neopets.com! I didn't quite copy
the whole button style, both because getting it to play nice with our
existing styles didn't *immediately* work, but also because I think
this works out as a really good compromise between our two styles
anyway!
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.)
Note: I validated this was working by temporarily changing the URI to
`https://echo.free.beeceptor.com`, which echoes the headers back, then
called `OwlsValueGuide.load_itemdata` directly.
Note: I validated this was working by temporarily changing the URI to
`https://echo.free.beeceptor.com`, which echoes the headers back, then
called `NeopetsMediaArchive.load_file_from_origin` directly.
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".
This is gonna help me in development, to stop having to add stuff to
the URL all the time!! I also considered just always making it
available in development, but I wanted to match production behavior to
help us ensure the hiding behavior is working, to avoid leaking NeoPass
without realizing.
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.
Oh right, we don't have Rails UJS going on anymore, which is what
handled the confirmation prompts for deleting lists. Turbo is the more
standard modern solution to that, and should speed up certain
pageloads, so let's do it!
Here I install the `turbo-rails` gem, then run `rails turbo:install` to
install the `@hotwired/turbo-rails` npm package. Then I move
`application.js` that's run all on pages but the outfit editor into our
section of JS that gets run through the bundler, and add Turbo to it.
I had to fix a couple tricky things:
1. The outfit editor page doesn't play nice with being swapped into the
document, so I make it require a full page reload instead.
2. Prefetching the Sign In link can cause the wrong `return_to` address
to be written to the `session`. (It's a GET request that does, ever
so slightly, take its own actions, oops!) As a simple hacky answer,
we disallow prefetching on that link.
Haven't fixed up the UJS stuff for confirm prompts to use Turbo yet,
that's next!
This `.gif` format is used in the items list "export to petpage"
feature, as the image URL for items whose URLs are known to contain
blocked words that prevent them from being used in petpages.
But when doing some Rails upgrade long ago, we didn't notice the new
security feature that blocks redirects to other sites without a special
flag being set. It was triggering 500 errors, oops.
Now, we set the flag!
I think this is the more canonical place for stuff like this these days!
It's nice to be able to just say the short name when calling `render`.
Here's the answer I looked up about it: https://stackoverflow.com/a/9892081/107415
My immediate motivation is that I'm looking at creating more About
pages, and thinking about where to put them; I think maybe we trash the
`StaticController`, move these partials out to here, and move terms
into a new `AboutController`?
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!
According to our GlitchTip error tracker, every time we deploy, a
couple instances of `Async::Stop` and `Async::Container::Terminate`
come in, presumably because:
1. systemd sends a STOP signal to the `falcon host` process.
2. `falcon host` gives the in-progress requests some time to finish up
3. Sometimes some requests take too long, and so something happens.
(either a timer in Falcon or a KILL signal from systemd, not sure!)
that leads the ongoing requests to finally be terminated by raising
an `Async::Stop` or `Async::Container::Terminate`. (I'm not sure
when each happens, and maybe they happen at different points in the
process? Maybe one happens for the actual long-running ones, vs the
other happens if more requests come in during the meantime but get
caught in the spin-down process?)
4. Rails bubbles up the errors, our Sentry library notices them and
sends them to GlitchTip, the user presumably receives the generic
500 error, and the app can finally close down gracefully.
It's hard for me to validate that this is *exactly* what's happening
here or that my mitigation makes sense, but my logic here is basically,
if these exceptions are bubbling up as "uncaught exceptions" and
spamming up our error log, then the best solution would be to catch
them!
So in this change, we add an error handler for these two error classes,
which hopefully will 1) give users a better experience when this
happens, and 2) no longer send these errors to our logging 🤞❗️
That strange phenomenon where the best way to get a noisy bug out of
your logs is to fix it lmao
