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

Revision and workflow demo data #438

Merged
merged 1 commit into from
Sep 1, 2023

Conversation

Stormheg
Copy link
Member

@Stormheg Stormheg commented Aug 10, 2023

Depends on #436

I've created some page revisions manually for the recipe page 'Hot Cross Bun' and home page 'Welcome to the Wagtail Bakery!'. I also went through the moderator approval workflow so there is some demo data available for that feature too.

A draft revision for Person snippet 'Muddy Waters' was also created.

Furthermore, I fabricated some revisions using the following code:

import random
from django.contrib.auth.models import User
from wagtail.models import Page

users = User.objects.all()

# Exclude the root page (id=1) and the home page (id=60)
for page in Page.objects.all().exclude(pk__in=[60, 1]).specific():
    # Choose a random user who 'performed' the action
    user = random.choice(users)
    page.save_revision(log_action="wagtail.create", user=user).save()
    page.save_revision(log_action="wagtail.edit", user=user).publish(user=user)

@Stormheg Stormheg self-assigned this Aug 10, 2023
Copy link
Member

@laymonage laymonage left a comment

Choose a reason for hiding this comment

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

I think instead of including the revisions in the .json data, we should put your code to generate the revisions into the load_initial_data command. The revisions data is quite big and can always be regenerated.

@Stormheg
Copy link
Member Author

@laymonage not all demo data is generated, for some pages I saved some revisions manually and actually changed some fields. I also used the workflow feature which relies on some revisions existed.

Do you think we can handle generating the following steps programmatically?

  • Change some content
  • Go through the workflow process

@laymonage
Copy link
Member

laymonage commented Aug 10, 2023

Do you think we can handle generating the following steps programmatically?

  • Change some content
  • Go through the workflow process

We can 😄

To change the content, we can do something like:

page.title = f"{page.title} (edited)"
page.save_revision(user=some_user)

For workflows, we can try following the tests in: https://github.com/wagtail/wagtail/blob/main/wagtail/tests/test_workflow.py#L121 which do not rely on the test client.

@laymonage
Copy link
Member

That said, honestly I'm not sure if the workflows data should be in bakerydemo's initial data. I agree the revisions would be handy, as some of the UI in Wagtail assumes that all pages have at least one revision. However, for workflows, it might be a bit weird to start off a bakerydemo instance and seeing that you have pages in workflows already. And it's not going to take a long time to test the workflows yourself now that we have the editor and moderator users in the fixtures.

@thibaudcolas
Copy link
Member

@laymonage why would the revision data being big be an issue? I suggested doing this with revisions data because as far as I could tell doing this programmatically in load_initial_data would cause problems. It means exporting fixtures will always generate data we don’t want for models that have a revision associated.


For workflows – from my perspective this is the same issue as any other feature of the CMS, where yes we can manually input data, but then it means all kinds of testing scenarios where we need this data are slower (even if just by a bit), and prone to inconsistencies.

@thibaudcolas
Copy link
Member

For reference, we’ve discussed this today in a meeting and generally felt like having workflow data, including an in-progress workflow, would be useful enough to warrant addition – so we reflect the data of a site that’s under active maintenance.

For revisions, Sage and I discussed the options and are generally happy with the idea of having them in fixtures.

@thibaudcolas
Copy link
Member

thibaudcolas commented Aug 15, 2023

@Stormheg I’ve tried to rebase your PR after merging #436 and review the changes but for some reason I don’t see any revisions or workflow content, I’m not sure what went wrong.

Could you take a look and fix / tell me what I did wrong?

Also – for reference, on guide.wagtail.org, we document the four panels on the dashboard:

  • Your pages in a workflow
  • Awaiting your review
  • Your most recent edits
  • Your locked pages

So ideally with our changes here the admin account would have at least one entry in each, and the "awaiting your review" page would have one or two comments so we can demonstrate that feature. Aside from documents (#440), this is the last blocker to using the vanilla bakerydemo for our documentation.

@Stormheg Stormheg force-pushed the feature/more-demo-data branch 2 times, most recently from 9025672 to d801d4b Compare August 24, 2023 09:32
@Stormheg
Copy link
Member Author

Hi @thibaudcolas

for some reason I don’t see any revisions or workflow content, I’m not sure what went wrong.

It looks like the revision and workflow data relies on the page log which I did not include in the dump. I've fixed that now 😓

I've also...

  • Add two recipe PDF documents for the recipes you linked to in Documents in demo site #440. I've added links them on their the respective recipe pages.
  • Added two comments on the 'welcome to the wagtail bakery' page
  • Added the welcome page to a workflow
  • Added the Muddy Waters person snippet to a workflow by the german user
  • Manually changed some data on the 'bread and circuses' page to test the comparison view
  • Locked the 'contact us' page

The dashboard now looks like this for the admin user. Could you re-review please?

image

@Stormheg Stormheg mentioned this pull request Aug 24, 2023
Copy link
Member

@laymonage laymonage left a comment

Choose a reason for hiding this comment

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

Thanks! This is working really well.

I have a few thoughts, these are non-blocking but I'd wait for Thibaud's opinion before approving this:

  • The PDFs are for the same recipe as the ones we have, instead of a "similar recipe". This is probably okay, but I previously raised a concern about the possibility of people assuming the PDF is automatically generated from the page data, which could cause confusion when the page data gets out-of-sync with the PDF data.
  • The Muddy Waters person snippet doesn't have a live (or previous) revision before it was submitted to the workflow. This doesn't cause any issues because Wagtail handles this gracefully. However, in real life use of Wagtail, if a page/snippet has a "live" version, they likely also have a corresponding live revision (unless the data is created outside of the CMS, e.g. via fixtures).
    • This actually also applies to all DraftStateMixin-enabled models. Ideally each instance of these models, if live, should have a live revision. Not sure if it's worth including in the fixtures, though. It might also be useful not to, so that we easily test that this specific case doesn't break.
    • Still, I think for pages/snippets that we specifically manipulate in this PR, should have an initial live revision.
  • Perhaps instead of (or in addition to) "Manually changed some data on the 'bread and circuses' page to test the comparison view", we should make it so that the page/snippets in workflows also have changes. It would be more realistic this way, as submitting pages/snippets to workflows without any changes doesn't seem like something people would do.
  • I saw that all the workflows were started by Admin User. I think it would make more sense if they were started by an editor user. For snippets, this means we'll need to add grant edit permissions for the Person snippet to the Editors group, though.

Finally, this is probably not necessary but if the above concerns were to be addressed, we might want to start over so that the revisions and log entries from previous iterations of this PR are erased.

@thibaudcolas
Copy link
Member

Alright, I believe I’ve updated the fixtures according to the test content from Storm, with the feedback from Sage taken into account.

Changes from previous version:

  • Different "related" PDFs rather than same recipes (c0fed93)
  • Muddy Waters and all other revision-aware snippets have at least a "created" revision", and a "published" revision for draft-aware models.
  • Muddy Waters is submitted for moderation, started by the editor user (whose group now has permissions to submit for moderation all draft-aware models).
  • The homepage and Muddy Waters workflows are started by the editor user, and all things in workflows include changes.

@thibaudcolas thibaudcolas merged commit 2edb874 into wagtail:main Sep 1, 2023
3 checks passed
@Stormheg Stormheg deleted the feature/more-demo-data branch September 2, 2023 09:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants