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!
