[Wordpress] Theme & Plugin Last Updated badge

[JoeIzzard]

Related to #5493, adds the new last update badge for both plugins and themes.

[shields-ci]

	Messages
📖	✨ Thanks for your contribution to Shields, @JoeIzzard!

Generated by 🚫 dangerJS against 03d56d6

[calebcartwright]

More schema questions, but not familiar with the service/API details in this case. Will this field strictly and always exist, regardless of things like version, whether the theme (in this case, plugin below) has only been published once without any updates, etc.?

[calebcartwright]

Also, I feel like given the utilization of this value down below in the transform function of the service class, we probably want to tighten up this schema a bit to account for the shape of the string that the service class is expecting.

We're in the process of trying to write down our guidelines for schemas, but this is one of the circumstances we've been discussing.

cc @chris48s for any additional thoughts on schema structure

[JoeIzzard]

In terms of if this will always exist, I believe so. I think it just returns the last action date, so for a brand new plugin/theme with no updates, it should return the initial publish date. I can't say for sure though as most of the Wordpress API isn't documented past endpoints you can query.

As for the transform point, just to note that the plugin and theme return different formatting, so we need to keep that in mind.

[calebcartwright]

As for the transform point, just to note that the plugin and theme return different formatting, so we need to keep that in mind.

It's not about what they return, but about the input. date here is the value of a key from the response object that currently has a very relaxed Joi.string() schema. However, the transform function is clearly expecting this string to be in a very specific format and it will cause runtime errors if date does not conform to that assumed shape.

What I'm requesting here is that Joi.string() be updated (probably by adding a regex) to ensure that the value of date will conform to the structure that transform requires and is currently just assuming

    transform(date, extensionType) {
      let d
      if (extensionType === 'plugin') {
        d = date.split(' ')
        d = d[0]
      } else {
        d = date
      }

      const ymd = d.split('-')

      const out = ymd[0] + ymd[1] + ymd[2]
      return out
    }

[JoeIzzard]

I see!

So would it be better to do that at the schema level, or in the class that handles that badge? In theory, if it ever came back not matching, would it being in the schema also break other badges that don't rely on that piece of info?

[calebcartwright]

So would it be better to do that at the schema level, or in the class that handles that badge?

We want to do input validation as part of the schema

In theory, if it ever came back not matching, would it being in the schema also break other badges that don't rely on that piece of info?

Yes, if someone requested a badge for a given theme/plugin, and the API response failed to match the input validation then Shields would return an invalid response data badge (by design). Since all the Wordpress badges are using the same shared schema, yes, a request for any of the Wordpress badges would fail in this circumstance even if they don't care about that particular field. That's why it's important when using a single, shared schema for multiple badges instead of service/badge specific schemas that all of the fields included in the schema will always be validated regardless of the badge for that family.

[JoeIzzard]

Awesome. So I have added two patterns, one for themes, and one for plugins and added those to the schema. Also updated a mocked test which was using the wrong pattern and causing it to fail this new schema check.

[chris48s]

cc @chris48s for any additional thoughts on schema structure

Sorry. Probably should have jumped in on this one a couple of days ago.

This regex/splitting strings up and joining them back together approach doesn't seem like the best way to deal with this either on the validation or formatting side tbh. We should really lean on a library here IMO. Lets put aside #5573 for now. Adding a couple more lines of moment.js code isn't going to cause us a huge issue in the grand scheme of things. How about we do something like:

const moment = require('moment')

...

last_updated: Joi.string().required()

...

const date = moment(last_updated, "YYYY-MM-DD hh:mma GMT") // think this is the right format string for the plugin case

if (date.isValid()) {
  throw new InvalidResponse({prettyMessage: 'invalid date'});
}

// do something with date.format('YYYY-MM-DD')

While its nice to do the validation in the schema if possible rather than throwing an InvalidResponse() manually, Joi.date() isn't flexible enough for us here due to the format used.

I've got a much higher level of confidence that const date = moment(last_updated, "YYYY-MM-DD hh:mma GMT") does what we want than I have looking at [0-9]{4}-[0-9]{2}-[0-9]{2} [0-9]{1,2}:[0-5][0-9](am|pm) [A-Z]{3}. For a start I can actually read it :) . By the looks of it you need two different format strings for themes than plugins, but the moment.js docs are great https://momentjs.com/docs/#/parsing/string-format/

Maybe dates/times are another topic for #5619

[calebcartwright]

While I do have some minor reservations about increasing our moment.js footprint, I do agree that it's minor enough to not be an issue here and like this proposal even better!

Would you agree though @chris48s that the original loose schema and transform logic would've been problematic for what we're driving towards on schema guidelines? For example if this had been some other type of field where we couldn't rely on a 3rd party lib like moment

[chris48s]

I guess if this was a string that describes a completely arbitrary thing (not a datetime) but we wanted to split it up on space and dash, something like that would have been fine. In that case, we'd want to ensure that:

• The characters we're going to split on exist
• If we're going to reference myvar[2] after splitting it, that index is going to exist
• Whatever we end up passing to formatDate() (except it would be formatSomethingElse() ) isn't going to give the formatting function a problem

I guess this is a situation where it is beneficial to think about readable code and documenting our understanding of the upstream API as well as just the least strict validation possible.

[chris48s]

nice 👍 lets get this one merged too

[github-actions]

This pull request was merged to master branch. This change is now waiting for deployment, which will usually happen within a few days. Stay tuned by joining our #ops channel on Discord!

After deployment, changes are copied to gh-pages branch: