search

boonebgorges / buddypress-docs

GNU General Public License v3.0
106 stars 44 forks source link

save() refactoring breaks some activity posting #588

Closed boonebgorges closed 6 years ago

boonebgorges commented 6 years ago

It looks like the save() refactoring broke some of the activity item logic. Specifically, the $query->doc_id check at the beginning of bp_docs_post_activity() is failing, at least for doc edits.

It's hard to see this in the unit tests because of a 'headers already sent' failure; I can't tell if that problem (which is linked to a call to setcookie) is also linked to this same bug - ie, an unexpected failure in the save process - or whether it's OK to squelch it with @. Anyway, for the purposes of testing, feel free to change it to @setcookie().

@dcavins Would you mind having a first look? You'll be able to understand the nature of the change more quickly than I will.

dcavins commented 6 years ago

I think that I haven't refactored far enough. There's still a context-sensitive portion that I didn't excise (the use of BP_Docs_Query::doc_slug to determine whether something is new or not) that I believe can go.

Otherwise, the tested behaviors (creation of activity items for changed content or titles) work OK, though the tests can be simplified using the new save() behavior. I'll make up a changeset.

boonebgorges commented 6 years ago

Thank you sir!

dcavins commented 6 years ago

Hi Boone-

This was a good thing to look at, since it helped identify ways that I could further improve the save() method (and some associated bits) when used directly. Please take a look at the commits in this branch and let me know what you think: https://github.com/dcavins/buddypress-docs/commits/issue-588

boonebgorges commented 6 years ago

Thanks for working on this. The changes look good on a read-through, and it appears to fix the automated tests. I have not done extensive testing on the front-end, but if you're satisfied, then I am 👍

dcavins commented 6 years ago

The changes affect programmatic saving more than standard front-end saving. The really important change is the is_new_doc calculation here: https://github.com/dcavins/buddypress-docs/commit/5a69bcbb598980437c12c26c11ab295783d245c5

If that logic seems sound to you, then I'm happy to merge the changes. :)

boonebgorges commented 6 years ago

🚢

dcavins commented 6 years ago

Resolved in a series of commits, starting at 5e1697c64f0ecefcf29ffe857fe0b3cd203fadac.

Thanks for the feedback!