Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Last updated and reviewed dates #1138

Merged
merged 6 commits into from
May 10, 2023
Merged

Conversation

yndajas
Copy link
Member

@yndajas yndajas commented Mar 31, 2023

Changes in this PR

See #1135 for context - @davidwinter has suggested broadening the scope of that, so this doesn't fully close it

Provide access to last updated date and ability to record last reviewed date for contentful pages. The last review date is added as a field in the Netlify CMS for people to complete after reviewing a page's content. The last updated date is taken from the file's last commit (if possible). A link to the relevant GitHub history page is provided for informatio

Screenshots of UI changes

When the page has been reviewed since the last update

Screenshot 2023-03-31 at 18 27 09

When the page has been updated since the last review

Screenshot 2023-03-31 at 18 27 42

Backend:

image

@netlify
Copy link

netlify bot commented Mar 31, 2023

Deploy Preview for laughing-payne-b9fbd2 ready!

Name Link
🔨 Latest commit ff47653
🔍 Latest deploy log https://app.netlify.com/sites/laughing-payne-b9fbd2/deploys/645aae67c4fb7800088e25af
😎 Deploy Preview https://deploy-preview-1138--laughing-payne-b9fbd2.netlify.app/
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@yndajas yndajas changed the title Last reviewed and modified dates Last reviewed and update dates Mar 31, 2023
@yndajas yndajas changed the title Last reviewed and update dates Last reviewed and updated dates Mar 31, 2023
@yndajas yndajas changed the title Last reviewed and updated dates Last updated and reviewed dates Mar 31, 2023
Copy link
Contributor

@davidwinter davidwinter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an exciting change! I've offered a few suggestions which might help make the PR smaller, and help in the long term with managing the playbook. Keen to hear what you think.

src/admin/config.yml Outdated Show resolved Hide resolved
src/commercial-ops/delegated-authorities.md Show resolved Hide resolved
@yndajas yndajas force-pushed the last-reviewed-and-modified-dates branch 3 times, most recently from 47a4324 to 7675bf8 Compare May 9, 2023 13:14
yndajas and others added 2 commits May 9, 2023 20:45
According to the playbook[1]:

> The owner of each section of the Playbook should lead a review of the
  sections they own at least every quarter
>
> To help facilitate regular review, we should aim for Playbook content
  to include last updated and last reviewed dates.

The latter is not currently done, and the former is likely not done
consistently. Doing the latter will help us track how well we're
meeting the expectations we set out for reviewing playbook content on a
regular basis

@johnwaterworth raised an issue related to this: #1135

---

This gem allows us to get the date of the last git commit on the file
for a given page, or failing that when the file itself was modified.
This will feed the last updated information on each page

I would have liked to include a default time format in the config, but
this didn't work using the syntax in the gem's README[2]

[1]: https://playbook.dxw.com/contributing/managing-the-playbook/#review
[2]: https://github.com/gjtorikian/jekyll-last-modified-at#setting-up
This uses a YAML anchor[1] to DRY up the Netlify admin config,
which had a bunch of repeated lines for every folder of Markdown
files

[1]: https://support.atlassian.com/bitbucket-cloud/docs/yaml-anchors

Co-authored-by: David Winter <[email protected]>
@yndajas yndajas force-pushed the last-reviewed-and-modified-dates branch 3 times, most recently from 252e28b to d5b3d8e Compare May 9, 2023 20:21
Before this change, on all Markdown files except `index.md` and
`**/index.md`, we specified a permalink of `/:path/:basename/`. The same
effect - the permalink being the path plus the kebabified name of the
file with no extension, except on index pages, for which it's just the
path - can be achieved with one config line. This updates the config and
removes the repeated line in each Markdown file
@yndajas yndajas force-pushed the last-reviewed-and-modified-dates branch 2 times, most recently from 07d86a9 to 07d94af Compare May 9, 2023 20:25
yndajas added 3 commits May 9, 2023 21:34
This adds an optional field to the Netlify CMS to allow users to enter a
last reviewed date (after completing a periodic review of a page)

It also adds the attribute to the front matter of each existing page,
since without this Netlify will set the date to the current time when
opening the page in the CMS, despite the default in the config[1]

[1]: decaporg/decap-cms#1745
This takes the last updated and last reviewed dates exposed in the
previous two commits and adds them to the bottom of each contentful page
(i.e. anything that has its own content beyond an index of pages in its
section). These are added at the end of the page after a horizontal rule

If the page has been updated since the last review, it will only show
this date. If a review has been recorded since the last update, it will
show both dates
The previous commit added the last updated date to the bottom of each
contentful page. This adds a link to the respective GitHub history page,
for more detailed information (e.g. to check that the last update was
meaningful and not just a dev fixing formatting across multiple files)

The last updated date could in some cases give a misleading sense of
security that the page is more up to date than its content, so this
provides a quick way to check
@yndajas yndajas force-pushed the last-reviewed-and-modified-dates branch from 07d94af to ff47653 Compare May 9, 2023 20:34
@yndajas yndajas requested a review from davidwinter May 9, 2023 20:35
@yndajas
Copy link
Member Author

yndajas commented May 9, 2023

@davidwinter I've tidied everything up, including adding the blank last updated date to each file, and rebasing. Should be ready for another review if you have time!

Copy link
Contributor

@davidwinter davidwinter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yndajas this is looking good 🚀

My only concern is around changing the config for the permalink structure, and whether we can be entirely confident it won't break any URLs. I completely trust that you've checked that yourself to ensure links haven't broken, but can imagine it's quite hard to verify all of the pages. I think we should create a separate issue to check for broken links (I've noticed a few during my onboarding) and ensure redirects are in place. Just wondered if you had any thoughts on that.

@yndajas
Copy link
Member Author

yndajas commented May 10, 2023

My only concern is around changing the config for the permalink structure, and whether we can be entirely confident it won't break any URLs

I've checked a sample of index and non-index pages and the URLs all look the same, including when the filename doesn't completely match the title (e.g. work-we-do/sharing-expertise.md is titled "Sharing our expertise", but both on the live site and this branch the "our" is skipped in the path)

but can imagine it's quite hard to verify all of the pages. I think we should create a separate issue to check for broken links (I've noticed a few during my onboarding) and ensure redirects are in place. Just wondered if you had any thoughts on that.

So there is an issue about this (#977) and we do have a gem to help with checking for dead links: https://github.com/dxw/playbook#checking-the-html. We don't have it integrated into CI because it has a lot of false positives - either from authenticated pages or hash fragments that it can't resolve but do work fine

I've just run it on both main and this branch and both have 41 failures, so it looks like there's no regression. I'd suggest we separately resolve the internal links that are 404ing (mostly in the staff handbook area - forgot about this automated check when working with Dawn on that!). Happy to open a PR for that

@yndajas yndajas merged commit f4c7aba into main May 10, 2023
@yndajas yndajas deleted the last-reviewed-and-modified-dates branch May 10, 2023 10:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants