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

Migrate Backup page #9447

Merged
merged 39 commits into from Aug 24, 2018

Conversation

Projects
None yet
8 participants
@sarjon
Member

sarjon commented Aug 13, 2018

Questions Answers
Branch? develop
Description? This PR migrates Configure > Advanced parameters > Database > Backup page.
Type? improvement
Category? BO
BC breaks? yes (overrides)
Deprecations? no
Fixed ticket? n/a
How to test? Page should work the same as old one.

This change is Reviewable

namespace PrestaShop\PrestaShop\Adapter\Backup;
use DateTimeImmutable;

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Aug 13, 2018

Contributor

this doesn't exist in PHP 5.6, does it?

@mickaelandrieu

mickaelandrieu Aug 13, 2018

Contributor

this doesn't exist in PHP 5.6, does it?

This comment has been minimized.

@sarjon
@sarjon
@mickaelandrieu

A lot of minor improvements already, also I haven't finished to review as I haven't tested the behavior on my computer :)

More to come tomorrow!

@mickaelandrieu mickaelandrieu referenced this pull request Aug 13, 2018

Closed

Roadmap migration (1.7.5) #10

26 of 40 tasks complete
@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Aug 14, 2018

Contributor

Hello @sarjon you may improve again your contribution, see the Codacy report, some issues are easy to fix.

Contributor

mickaelandrieu commented Aug 14, 2018

Hello @sarjon you may improve again your contribution, see the Codacy report, some issues are easy to fix.

@sarjon

This comment has been minimized.

Show comment
Hide comment
@sarjon

sarjon Aug 14, 2018

Member

ill add missing docblocks & fix Codacy at the end, when ill address all your comments. :)

Member

sarjon commented Aug 14, 2018

ill add missing docblocks & fix Codacy at the end, when ill address all your comments. :)

@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Aug 18, 2018

Contributor

this would also introduce inconsistency as the rest of Back Office uses Kb, Mb & etc sizes everywhere, wouldn't that be a problem?

Sounds like worth to have our own class to deal with, and write unit tests for it then 👍

Contributor

mickaelandrieu commented Aug 18, 2018

this would also introduce inconsistency as the rest of Back Office uses Kb, Mb & etc sizes everywhere, wouldn't that be a problem?

Sounds like worth to have our own class to deal with, and write unit tests for it then 👍

@sarjon

This comment has been minimized.

Show comment
Hide comment
@sarjon

sarjon Aug 18, 2018

Member

so basically i have extracted some methods from data provider into separate services that modifies each record.

One of example service would be PrestaShop\PrestaShop\Core\Grid\Modifier\EmployeeDateFormatModifier. And what i noticed is that modifiers would be a good addition to Grid component. :)

The idea around modifier is this: once you get data from data store (db, api or from filesystem, doesnt matter) you should be able to perform some modifications on it. For example:

In this case, we have backups data that is provided by BackupGridDataProvider, here's an example:

// PrestaShop\PrestaShop\Core\Backup\BackupGridDataProvider.php

final class BackupGridDataProvider implements GridDataProviderInterface
{
    // ...

   public function getData(SearchCriteriaInterface $searchCriteria)
    {
       // ...

        foreach ($paginatedBackups as $backup) {
            $backupsArray[] = [
                'file_name' => $backup->getFileName(),
                'file_size' => $backup->getSize(),
                'date' => $backup->getDate(),
                'age' => $backup->getAge(),
            ];
        }

        $backupCollection = new RecordCollection($backupsArray);

        return new GridData(
            $backupCollection,
            count($backups)
        );
    }
} 

As you can see, we have file_size as plain bytes value, what i would like to do, is convert these bytes into human readable value that will be displayed. So what i did is created PrestaShop\PrestaShop\Core\Backup\Modifier\HumanReadableBackupFileSizeModifier which takes care of it.

There can be more example of it, lets say we get plain product price from db which is 15.991547, you wouldnt display that directly in list, right? So modifier would be responsible for formatting this price into something like 15.99 EUR.

Now, im wondering what would be the best place in Grid component to put these modifiers? @mickaelandrieu i would like your input on this, what do you think? :)

You can check this commit 1d8e498 although its not finished yet.

Member

sarjon commented Aug 18, 2018

so basically i have extracted some methods from data provider into separate services that modifies each record.

One of example service would be PrestaShop\PrestaShop\Core\Grid\Modifier\EmployeeDateFormatModifier. And what i noticed is that modifiers would be a good addition to Grid component. :)

The idea around modifier is this: once you get data from data store (db, api or from filesystem, doesnt matter) you should be able to perform some modifications on it. For example:

In this case, we have backups data that is provided by BackupGridDataProvider, here's an example:

// PrestaShop\PrestaShop\Core\Backup\BackupGridDataProvider.php

final class BackupGridDataProvider implements GridDataProviderInterface
{
    // ...

   public function getData(SearchCriteriaInterface $searchCriteria)
    {
       // ...

        foreach ($paginatedBackups as $backup) {
            $backupsArray[] = [
                'file_name' => $backup->getFileName(),
                'file_size' => $backup->getSize(),
                'date' => $backup->getDate(),
                'age' => $backup->getAge(),
            ];
        }

        $backupCollection = new RecordCollection($backupsArray);

        return new GridData(
            $backupCollection,
            count($backups)
        );
    }
} 

As you can see, we have file_size as plain bytes value, what i would like to do, is convert these bytes into human readable value that will be displayed. So what i did is created PrestaShop\PrestaShop\Core\Backup\Modifier\HumanReadableBackupFileSizeModifier which takes care of it.

There can be more example of it, lets say we get plain product price from db which is 15.991547, you wouldnt display that directly in list, right? So modifier would be responsible for formatting this price into something like 15.99 EUR.

Now, im wondering what would be the best place in Grid component to put these modifiers? @mickaelandrieu i would like your input on this, what do you think? :)

You can check this commit 1d8e498 although its not finished yet.

@mickaelandrieu

Good job @sarjon !

@mickaelandrieu mickaelandrieu added this to the 1.7.5.0 milestone Aug 23, 2018

@marionf marionf self-assigned this Aug 24, 2018

@marionf marionf removed the waiting for QA label Aug 24, 2018

@marionf marionf removed their assignment Aug 24, 2018

@marionf marionf added the QA ✔️ label Aug 24, 2018

@marionf

This comment has been minimized.

Show comment
Hide comment
@marionf

marionf Aug 24, 2018

Contributor

All good 👍

Contributor

marionf commented Aug 24, 2018

All good 👍

@PierreRambaud PierreRambaud merged commit 4696c02 into PrestaShop:develop Aug 24, 2018

1 of 3 checks passed

Codacy/PR Quality Review Not up to standards. This pull request quality could be better.
Details
code-review/reviewable 68 files, 18 discussions left (LouiseBonnard, mickaelandrieu, PierreRambaud, sarjon)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@PierreRambaud

This comment has been minimized.

Show comment
Hide comment
@PierreRambaud

PierreRambaud Aug 24, 2018

Contributor

Thanks @sarjon (again 🍻 )

Contributor

PierreRambaud commented Aug 24, 2018

Thanks @sarjon (again 🍻 )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment