Errors should only show if the unpublished schedule is bad

[adrhumphreys]

getIsUnPublishScheduled returns true if the date is set and in the future which means it is error free, therefore we should only show the notice for errors when it returns false

Should fix issue 37

[chrispenny]

Hi @adrhumphreys, thanks for the PR! I don't think this is going to have the intended affect though.

There is meant to be a message displayed here when an expiry date is set (in the future, or otherwise) - it is not an error message, it is more of a status message. I believe the issue here is simply that the message content is incorrect.

This record has an expiry date set, and cannot currently be edited. You will need to remove the scheduled embargo date in order to edit this record. Expiry: 2018-10-10 10:10:00

getEmbargoExpiryNoticeMessage() needs to be a bit smarter in the way that it determines this final message. If there is an expiry with no embargo, then the message should simply read something like:

This record has an expiry date set. Expiry: 2018-10-10 10:10:00

[adrhumphreys]

Hey @chrispenny, sorry if I'm understanding this wrong would it just be the case of changing the message in line 674 to be "This record has an expiry date set." and undoing the change I made?

[chrispenny]

Hi @adrhumphreys. No worries!

So, I think the issue is that the "last message" actually needs 2 versions. One where an embargo date is set (the message stays as it is now), and one where there is only an expiry set (uses the new message).

Psudo(ish) code (see the comments in the snippet):

public function addEmbargoExpiryNoticeFields(FieldList $fields)
{
    $conditions = [];

    if ($this->getIsPublishScheduled()) {
        $time = strtotime($this->owner->PublishOnDate);

        // Previously I was using a translatable string for the $key. I don't know why... Just make it 'embargo'.
        $conditions['embargo'] = [
            'date' => $this->owner->PublishOnDate,
            'warning' => ($time > 0 && $time < time()),
        ];
    }

    if ($this->getIsUnPublishScheduled()) {
        $time = strtotime($this->owner->UnPublishOnDate);

        // Previously I was using a translatable string for the $key. I don't know why... Just make it 'expiry'.
        $conditions['expiry'] = [
            'date' => $this->owner->UnPublishOnDate,
            'warning' => ($time > 0 && $time < time()),
        ];
    }

    // existing stuff
    ...
}

And then when we go to generate the message:

/**
 * @param $conditions
 * @return string
 */
public function getEmbargoExpiryNoticeMessage($conditions)
{
    if ($this->isEditable()) {
        // This stays the same.
        ....
    }

    if ($this->checkRemovePermission()) {
        // This stays the same.
        ....
    }

    // A new check to see if 'embargo' is one of our conditions. This uses the existing message.
    if (array_key_exists('embargo', $conditions)) {
        return sprintf(
            _t(
                __CLASS__ . '.EMBARGO_NONREMOVABLE_NOTICE',
                'This record has an %s date set, and cannot currently be edited. An administrator will need
                to remove the scheduled embargo date before you are able to edit this record.'
            ),
            implode(' and ', array_keys($conditions))
        );
    }

    // Uses the new message.
    return sprintf(
        _t(
            __CLASS__ . '.EMBARGO_NONREMOVABLE_NOTICE',
            'This record has an %s date set.'
        ),
        implode(' and ', array_keys($conditions))
    );
}

And yup, you'll want to remove the change you made.

[adrhumphreys]

Alright alright, hopefully we're onto a winner here @chrispenny

[chrispenny]

Initial assessment is: Looks good!

I'll have to pick this up on Monday though - running out of time today.

Thanks again :)