Skip to content
This repository has been archived by the owner on Dec 27, 2022. It is now read-only.

Schedule UI for snapshot in customizer #68

Merged
merged 62 commits into from Aug 11, 2016

Conversation

PatelUtkarsh
Copy link
Member

No description provided.

@coveralls
Copy link

coveralls commented Jul 28, 2016

Coverage Status

Coverage increased (+0.2%) to 87.585% when pulling 30df6bd on feature/schedule-ui-customizer into 68e8f34 on develop.

event.preventDefault();
component.sendUpdateSnapshotRequest( { status: 'draft' } );
if ( $( this ).html() === component.data.i18n.scheduleButton && ! _.isEmpty( component.snapshotScheduleBox ) && component.getDateFromInputs() && component.isScheduleDateFuture() ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

$( this ).html() seems dirty. Can't you look at snapshotStatus instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

snapshotStatus can be draft or future we can remove that check as we are checking for the future date on the same condition. And will also add isSnapshotHasUnsavedChanges check.

@westonruter
Copy link
Contributor

ESLint errors:

js/customize-snapshots.js: line 212, col 54, Error - 'e' is already declared in the upper scope. (no-shadow)
js/customize-snapshots.js: line 261, col 72, Error - 'component' is already declared in the upper scope. (no-shadow)
js/customize-snapshots.js: line 531, col 62, Error - No magic number: 1. (no-magic-numbers)
js/customize-snapshots.js: line 575, col 55, Error - No magic number: 1. (no-magic-numbers)
js/customize-snapshots.js: line 613, col 39, Error - Expected to return a value at the end of this function. (consistent-return)
js/customize-snapshots.js: line 626, col 6, Error - Blocks are nested too deeply (4). (max-depth)
js/customize-snapshots.js: line 633, col 5, Error - Unexpected if as the only statement in an else block. (no-lonely-if)
js/customize-snapshots.js: line 648, col 2, Error - Missing JSDoc return description. (valid-jsdoc)
js/customize-snapshots.js: line 660, col 46, Error - No magic number: 1000. (no-magic-numbers)
js/customize-snapshots.js: line 691, col 46, Error - No magic number: 1000. (no-magic-numbers)

And phpunit failure:

1) CustomizeSnapshots\Test_Ajax_Customize_Snapshot_Manager::test_ajax_update_snapshot_cap_check with data set #4 ('administrator', array(true, array(null, array(true))))
Failed asserting that Array &0 (
    'success' => true
    'data' => Array &1 (
        'errors' => null
        'setting_validities' => Array &2 (
            'anyonecanedit' => true
        )
        'snapshot_publish_date' => '0000-00-00 00:00:00'
    )
) is identical to Array &0 (
    'success' => true
    'data' => Array &1 (
        'errors' => null
        'setting_validities' => Array &2 (
            'anyonecanedit' => true
        )
    )
).

@coveralls
Copy link

coveralls commented Aug 10, 2016

Coverage Status

Coverage decreased (-1.4%) to 88.084% when pulling 244debe on feature/schedule-ui-customizer into b53f954 on develop.

return false;
}

remainingTime = component.dateValueOf( date );
Copy link
Contributor

Choose a reason for hiding this comment

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

Somewhat of a problem here. date is a Date object. However, the component.dateValueOf() function takes a string as its param. It looks like dateValueOf needs to take a Date as its argument as well?

diff --git a/js/customize-snapshots.js b/js/customize-snapshots.js
index 40cf91e..6ecd668 100644
--- a/js/customize-snapshots.js
+++ b/js/customize-snapshots.js
@@ -774,19 +774,21 @@
    /**
     * Get the primitive value of a Date object.
     *
-    * @param {string} dateString The post status for the snapshot.
+    * @param {Date|string} [date] The post status for the snapshot. Defaults to current date.
     * @returns {object|string} The primitive value or date object.
     */
-   component.dateValueOf = function( dateString ) {
-       var date;
+   component.dateValueOf = function( date ) {
+       var dateObj;

-       if ( _.isUndefined( dateString ) ) {
-           date = new Date();
+       if ( _.isUndefined( date ) ) {
+           dateObj = new Date();
+       } else if ( 'string' === typeof date ) {
+           dateObj = new Date( date );
        } else {
-           date = new Date( dateString );
+           dateObj = date;
        }

-       return date.valueOf();
+       return dateObj.valueOf();
    };

    component.init();

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed dateValueOf function.

@westonruter
Copy link
Contributor

@PatelUtkarsh something is wrong with the time being entered. I change a date and then click Reset and it always is showing for me as 16:25, no matter what the current time is:

date/time

@PatelUtkarsh
Copy link
Member Author

PatelUtkarsh commented Aug 10, 2016

@westonruter Maybe that is the time when snapshot was saved in DB, reset only reset time to snapshot's publish date if publish date is empty it resets to current date/time,

Should we tweak it like this: if snapshot date is future reset to that date or else reset to current date/time?

@westonruter
Copy link
Contributor

westonruter commented Aug 10, 2016

The (Reset) link should be initially shown if the date is not 0000-00-00 00:00:00? Instead, it is initially hidden:

@westonruter
Copy link
Contributor

@PatelUtkarsh ah, thanks for that info. OK, it does make sense to me now I think. The reset link is initially hidden because it behaves differently than it does in Customize Posts. Instead of resetting it to NOW it resets it to the original value. I was expecting Reset to undo the scheduling, by setting the date back to 0000-00-00 00:00:00 so that it would un-schedule and make it go live presently.

@coveralls
Copy link

coveralls commented Aug 10, 2016

Coverage Status

Coverage decreased (-0.4%) to 89.133% when pulling ae7189d on feature/schedule-ui-customizer into b53f954 on develop.

@coveralls
Copy link

coveralls commented Aug 10, 2016

Coverage Status

Coverage decreased (-0.4%) to 89.133% when pulling 59a7484 on feature/schedule-ui-customizer into b53f954 on develop.

@westonruter westonruter mentioned this pull request Aug 10, 2016
@coveralls
Copy link

coveralls commented Aug 11, 2016

Coverage Status

Coverage decreased (-1.0%) to 88.561% when pulling 4ea308b on feature/schedule-ui-customizer into b53f954 on develop.

@westonruter
Copy link
Contributor

How in the world did the coverage go DOWN when I added MORE tests?!

@coveralls
Copy link

coveralls commented Aug 11, 2016

Coverage Status

Coverage decreased (-1.0%) to 88.561% when pulling 3e5ce8a on feature/schedule-ui-customizer into b53f954 on develop.

@coveralls
Copy link

coveralls commented Aug 11, 2016

Coverage Status

Coverage increased (+1.2%) to 90.753% when pulling 535cbd5 on feature/schedule-ui-customizer into b53f954 on develop.

@valendesigns valendesigns merged commit 0763819 into develop Aug 11, 2016
@valendesigns valendesigns deleted the feature/schedule-ui-customizer branch August 11, 2016 08:34
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants