Page MenuHomePhabricator

Legalpad "Updated" date is wrong
Closed, ResolvedPublic

Description

from https://discourse.phabricator-community.org/t/legalpad-incorrectly-reports-when-a-document-was-updated/124

Legalpad doc View mode has a "Updated n days ago" tag, which appears to use the date the doc was created - editing the content doesn't reduce it to 0.

I've seen it on sort-of current master, and @tmakarios tested in on Phacility.

D18253 is attempting to fix this, but there's a comment in the code explaining that it should be correct as-is.

Event Timeline

The create and modified dates are supposed to be identical, since each edit creates a new copy.

ok, that explains the Versions field - it appears only to increase when updating the Body and not the Preamble.

I think there's some hack in there though, because my legalpadbody table only contains the latest version of the body (with the original creation time).

There may be a bug with the body versioning (maybe it got broken by accident when we swapped to ModularTransactions, or maybe it never worked), but if there is we should fix that bug. The creation date and modification date of a body should always be the same, like they are for a Phriction document body.

I should be able to bisect this if I brokened it.

I think you're innocent and that it never worked but I only looked at it for like 2 minutes.

In case it helps: I think D13982 introduced the code being edited by D18253. But it sounds like some other code isn't behaving the way you were expecting it would.

The creation date and modification date of a body should always be the same, like they are for a Phriction document body.

Am I right in thinking that the difference arises from these lines in /src/applications/phriction/editor/PhrictionTransactionEditor.php:

    $new_content = id(new PhrictionContent())
​      ->setSlug($document->getSlug())
​      ->setAuthorPHID($this->getActor()->getPHID())
​      ->setChangeType(PhrictionChangeType::CHANGE_EDIT)
​      ->setTitle($this->getOldContent()->getTitle())
​      ->setContent($this->getOldContent()->getContent());
​    if (strlen($this->getDescription())) {
​      $new_content->setDescription($this->getDescription());
​    }
​    $new_content->setVersion($this->getOldContent()->getVersion() + 1);​

and these lines in /src/applications/legalpad/editor/LegalpadDocumentEditor.php:

​      $body = $object->getDocumentBody();
​      $body->setVersion($object->getVersions());
​      $body->setDocumentPHID($object->getPHID());
​      $body->save();

?

For Phriction, a new PhrictionContent is created, but for Legalpad, an existing body is modified.

Seems a reasonable place to start. I'll look at this soon.

I may be mistaken, but it seems to me that D18280 will fix the problem only for new edits, made after that revision is applied. Is that right?

If so, then to make the display correct for documents that were edited before D18280 was applied, won't you need to either

  1. also apply D18253, or
  2. write a database migration to set the dateCreated on existing document bodies to equal the dateModified?

If you're going for the second option, do you also want to reconstruct the rows in the legalpad_documentbody table that @avivey pointed out were missing? I have no strong opinion on this question.

I see no way of reconstructing the rows. It appears versioning never worked (and nor do we surface it anywhere). We can add a migration to fix the current times though.

In T12933#230136, @chad wrote:

I see no way of reconstructing the rows.

Is the necessary data not available in the legalpad_transaction table? But even if it is possible, it might not be worth reconstructing the missing rows, if nothing is ever going to read them.

I don't know and I've spent way too much time here already.

Fair enough; I'll stop bothering you now. D18280 looks good to me.

No worries, and appreciate the help.

Closing for now, it's reasonable to post populate the table if we do decide we need the information. I'll note it on T5505