-
Notifications
You must be signed in to change notification settings - Fork 542
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
Conversation
There was a problem hiding this 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.
055d086
to
e1e7445
Compare
@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?
|
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. |
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. |
@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 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. |
e1e7445
to
45dd698
Compare
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. |
@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:
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. |
9025672
to
d801d4b
Compare
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...
The dashboard now looks like this for the admin user. Could you re-review please? |
There was a problem hiding this 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.
- This actually also applies to all
- 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.
e7931c3
to
e7d0c53
Compare
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:
|
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: