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

Add a note type for logging post events, that displays inline with user-input internal notes #9

Merged
merged 19 commits into from
Mar 10, 2022

Conversation

coreymckrill
Copy link
Collaborator

@coreymckrill coreymckrill commented Mar 1, 2022

Introduces a second type of note for logging changes to a post. These log notes appear in the sidebar along with user-created internal notes, and can provide extra context to what is happening with a post.

On the backend, this plugin provides logging for some generic event types that happen when posts are updated, but the idea is that other plugins (e.g. the Pattern Directory) can add their own logging as well.

In the UI, log notes have a more compact style in the notes list so that they are de-emphasized compared to user-created internal notes. They also can't be deleted.

I have made an attempt to have new log notes appear automatically in the sidebar when changes to a post are saved, but right now, it's causing a problem because the new log is getting loaded twice, even though the note is only getting created once. It must be some kind of race condition with the way React component updates happen, but so far I haven't been able to figure out how to avoid it. Fixed thanks to @StevenDufresne

Screencast

https://d.pr/v/iVeFjS

To test

  1. You might need to rebuild your local dev environment. The 0-sandbox.php file has been updated to add post support for the logging. Check out the updated readme for more info about how enabling logging works.
  2. Create a new post, then switch to the Notes sidebar. You'll see some initial events get logged.
  3. Try changing the post title and/or content.
  4. Try changing the post's taxonomy terms.
  5. Try adding a featured image, and changing the post's template.
  6. Log notes should not be deletable.
  7. Add an Internal Note using the "Add Note" form in the sidebar. Once you have both internal notes and log notes, try using the filter button to show only one or the other.

@coreymckrill coreymckrill self-assigned this Mar 1, 2022
@coreymckrill coreymckrill requested review from ryelle and dd32 March 3, 2022 01:53
@coreymckrill coreymckrill marked this pull request as ready for review March 3, 2022 01:53
When the prepending functionality was in a component within the
sidebar, the state didn't get updated correctly if the sidebar was
closed. This way even if the Post sidebar is open and you're changing
e.g. a post's categories, log notes will still get loaded into state.
Copy link
Contributor

@ryelle ryelle left a comment

Choose a reason for hiding this comment

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

It works great for post title, content, and featured image, but the taxonomy log seems a little broken - should it say which terms were added/removed?

Screen Shot 2022-03-09 at 3 44 04 PM

I left a few comments, but nothing major. This is looking good!

// Gutenberg doesn't have a specific way to check whether the post was just saved, so we store the
// previous value of isSaving and when isSaving goes from true -> false, we prepend the notes.
const postWasJustSaved = prevIsSaving && ! isSaving;
if ( postWasJustSaved ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It's important to note that this doesn't make any assumptions as to whether it succeeded. I'm not familiar with all the use cases of this code/plugin, but if you wanted to make sure it succeeded first, check:

select('core/editor').didPostSaveRequestSucceed()

This check does not represent the "next" state after saving so it needs to be checked after a save was performed. For example, if you check this on load, it may return true.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's a good point. In this case, though, I don't think it matters whether or not it succeeded. If the routine to prepend more log notes runs, but doesn't find any new logs, it should just fetch an empty array, rather than throwing an error of some kind.

@coreymckrill
Copy link
Collaborator Author

the taxonomy log seems a little broken - should it say which terms were added/removed?

@ryelle That's really odd. It is definitely showing the term names in my local dev environment. When you first create a new post, is there a log showing that the "Uncategorized" category was added? Or is that one blank too?

@ryelle
Copy link
Contributor

ryelle commented Mar 9, 2022

Honestly I didn't try creating a new post, I was testing on the Hello World post - I removed Uncategorized and added a new category. Oh… I think in both cases (tags and cats) I was adding terms that didn't already exist when the page loaded, could that have something to do with it?

The totalNotes value that appendNotes receives is inaccurate because
it represents found_posts for the offset query. So it's not used.
Removing it simplifies the code a touch and avoids confusion.
@coreymckrill
Copy link
Collaborator Author

I was adding terms that didn't already exist when the page loaded, could that have something to do with it?

🤔 I just tried that, and I still got the term names included in the logs...

@coreymckrill
Copy link
Collaborator Author

I'm going to go ahead and merge this, since I can't reproduce the missing term names issue. We can open up a new issue if it reappears.

@coreymckrill coreymckrill merged commit 88ab017 into trunk Mar 10, 2022
github-actions bot pushed a commit that referenced this pull request Mar 10, 2022
…s inline with user-input internal notes (#9)

Introduces a second type of note for logging changes to a post. These log notes appear in the sidebar along with user-created internal notes, and can provide extra context to what is happening with a post.

On the backend, this plugin provides logging for some generic event types that happen when posts are updated, but the idea is that other plugins can add their own logging as well.

In the UI, log notes have a more compact style in the notes list so that they are de-emphasized compared to user-created internal notes. They also can't be deleted.

Co-authored-by: Steven Dufresne <steve.dufresne@automattic.com>
@coreymckrill coreymckrill deleted the try/internal-log branch March 10, 2022 01:30
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.

3 participants