Skip to content
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

Files list migration #14864

Merged
merged 54 commits into from Oct 10, 2019

Conversation

@RaimondasSapola
Copy link
Contributor

RaimondasSapola commented Jul 26, 2019

Questions Answers
Branch? develop
Description? Catalog > Files and Catalog > Files
Type? refacto
Category? BO
BC breaks? no
Deprecations? no
Fixed ticket? #10550
How to test? Go to /admin-dev/index.php/sell/attachments. The page should work as previously.

This change is Reviewable

@RaimondasSapola RaimondasSapola requested a review from PrestaShop/prestashop-core-developers as a code owner Jul 26, 2019
@prestonBot

This comment has been minimized.

Copy link
Collaborator

prestonBot commented Jul 26, 2019

Hello @RaimondasSapola!

This is your first pull request on the PrestaShop project. Thank you, and welcome to this Open Source community!

/**
* @return AttachmentId[]
*/
public function getAttachmentIds()

This comment has been minimized.

Copy link
@rokaszygmantas

rokaszygmantas Jul 26, 2019

Contributor

we can use return type hints 🙂

*
* @throws AttachmentConstraintException
*/
public function __construct($attachmentId)

This comment has been minimized.

Copy link
@rokaszygmantas

rokaszygmantas Jul 26, 2019

Contributor

scalar type hints can be used everywhere in method signatures

/**
* @param $attachmentId
*
* @throws \PrestaShop\PrestaShop\Core\Domain\Attachment\Exception\AttachmentConstraintException

This comment has been minimized.

Copy link
@rokaszygmantas

rokaszygmantas Jul 26, 2019

Contributor

class should be imported

/**
* Defines contract for GetAttachmentPathHandler
*/
interface GetAttachmentPathHandlerInterface

This comment has been minimized.

Copy link
@rokaszygmantas

rokaszygmantas Jul 26, 2019

Contributor

since you are handling GetAttachment query, maybe the handler should be named GetAttachmentHandler as well, for consistency

@@ -35,6 +35,9 @@
*/
final class SubmitRowAction extends AbstractRowAction
{
const MESSAGE_TYPE_STATIC = 'static';

This comment has been minimized.

Copy link
@rokaszygmantas

rokaszygmantas Jul 26, 2019

Contributor

some comments would be helpful, explaining what these constants are used for

use Symfony\Component\Form\Extension\Core\Type\NumberType;
use Symfony\Component\Form\Extension\Core\Type\TextType;
final class AttachmentGridDefinitionFactory extends AbstractGridDefinitionFactory

This comment has been minimized.

Copy link
@rokaszygmantas

rokaszygmantas Jul 26, 2019

Contributor

missing docblock

Connection $connection,
$dbPrefix,
DoctrineSearchCriteriaApplicatorInterface $searchCriteriaApplicator,
$employeeIdLang

This comment has been minimized.

Copy link
@rokaszygmantas

rokaszygmantas Jul 26, 2019

Contributor

missing type hint

$qb
->select('a.`id_attachment`, al.`name`, a.`file`, a.`file_size`')
->addSelect('CONCAT(COALESCE(virtual_product_attachment.`product_count`, 0), " product(s)") AS products')

This comment has been minimized.

Copy link
@rokaszygmantas

rokaszygmantas Jul 26, 2019

Contributor

product(s) - this part is not translatable 🤔 was it like this in the legacy code?

src/Core/Util/File/FileSizeConverter.php Outdated Show resolved Hide resolved
{% set confirmation_message = action.options.confirm_message %}
{% if action.options.confirm_message_type == 'dynamic' and record[action.options.dynamic_message_field] is defined %}
{% set dynamic_field = record[action.options.dynamic_message_field] %}
{% set confirmation_message = dynamic_field is defined ? dynamic_field : action.options.confirm_message %}

This comment has been minimized.

Copy link
@rokaszygmantas

rokaszygmantas Jul 26, 2019

Contributor
Suggested change
{% set confirmation_message = dynamic_field is defined ? dynamic_field : action.options.confirm_message %}
{% set confirmation_message = dynamic_field|default(action.options.confirm_message) %}
$attachment = $this->getAttachment($attachmentId);
if (!$this->deleteAttachment($attachment)) {
throw new DeleteAttachmentException(

This comment has been minimized.

Copy link
@PierreRambaud

PierreRambaud Jul 26, 2019

Contributor

What if only the last one failed?
The response will return an error while everything instead of one id weren't deleted? 🤔

This comment has been minimized.

Copy link
@RaimondasSapola

RaimondasSapola Aug 8, 2019

Author Contributor

I see there is an unresolved discussion about this topic #14179

This comment has been minimized.

Copy link
@matks

matks Aug 16, 2019

Contributor

Indeed 😉 nice finding it !

So we should follow strategy skip it, attempt to perform the action on the remaining items, and at the end display a list of the failed usecases unless we think it's not relevant, in this case we ask Product Team (@marionf for example)

This comment has been minimized.

Copy link
@PierreRambaud

PierreRambaud Oct 9, 2019

Contributor

What was decided for this behavior?

defaults:
_controller: PrestaShopBundle:Admin/Sell/Catalog/Attachment:index
_legacy_controller: AdminAttachments
# _legacy_link: AdminAttachments

This comment has been minimized.

Copy link
@PierreRambaud

PierreRambaud Jul 26, 2019

Contributor

Commented?

This comment has been minimized.

Copy link
@rokaszygmantas

rokaszygmantas Jul 26, 2019

Contributor

probably because the page is not visible yet, so the legacy links are not updated.

$productNamesArray = $this->getProductNames($attachment['id_attachment']);
$productNames = $productNamesArray['product_names'] ?? '';
$attachment['dynamic_message'] = $this->trans(
'This file is associated with the following products, do you really want to delete it?',

This comment has been minimized.

Copy link
@LouiseBonnard

LouiseBonnard Aug 7, 2019

Contributor

It seems that there is a useless space in the original string, but I'm not fond of letting it as-is because it is a mistake. In order to bypass the typo, I would suggest writing the following new wording This file is associated with the following products. Are you sure you want to delete it?, what do you think?

This comment has been minimized.

Copy link
@RaimondasSapola

RaimondasSapola Aug 8, 2019

Author Contributor

Sound good to me

Copy link
Contributor

matks left a comment

@RaimondasSapola Nice job ! 😄 I've got a few feedbacks to handle but the work is overall very good

$attachment = $this->getAttachment($attachmentId);
if (!$this->deleteAttachment($attachment)) {
throw new DeleteAttachmentException(

This comment has been minimized.

Copy link
@matks

matks Aug 16, 2019

Contributor

Indeed 😉 nice finding it !

So we should follow strategy skip it, attempt to perform the action on the remaining items, and at the end display a list of the failed usecases unless we think it's not relevant, in this case we ask Product Team (@marionf for example)

src/Core/Util/File/FileSizeConverter.php Outdated Show resolved Hide resolved
@RaimondasSapola

This comment has been minimized.

Copy link
Contributor Author

RaimondasSapola commented Aug 22, 2019

Fixed CR issues including bulk delete behavior and file_exist check. @matks could you take a second look ? :)

@matks matks referenced this pull request Sep 9, 2019
1 of 32 tasks complete
$path = _PS_DOWNLOAD_DIR_ . $attachment->file;
if (!file_exists($path)) {
throw new AttachmentNotFoundException('Attachment file was not found');

This comment has been minimized.

Copy link
@matks

matks Sep 11, 2019

Contributor
Suggested change
throw new AttachmentNotFoundException('Attachment file was not found');
throw new AttachmentNotFoundException(sprintf('Attachment file was not found at %s', $path));

Exceptions are meant to be read by developers and consequently it's best to provide as much as possible info to them. If file does not exists, developer will need the $path to understand where it's supposed to be. For example if _PS_DOWNLOAD_DIR_ is missing a slash, he'll see it immediately this way 😉

@matks

This comment has been minimized.

Copy link
Contributor

matks commented Sep 11, 2019

@RaimondasSapola sorry for the late answer, only 1 thing to improve and we're done !
=> #14864 (comment)

@RaimondasSapola RaimondasSapola force-pushed the RaimondasSapola:PRES-189 branch from 7b4cc2c to 9917dbf Oct 10, 2019
@matks
matks approved these changes Oct 10, 2019
@matks matks merged commit e06677d into PrestaShop:develop Oct 10, 2019
2 checks passed
2 checks passed
PrettyCI Code formatting
Details
Travis CI - Pull Request Build Passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.