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

As an Editor, I want to be able to copy the content type in admin interface #1449

Merged
merged 12 commits into from Oct 12, 2020

Conversation

MarioBlazek
Copy link
Contributor

@MarioBlazek MarioBlazek commented Aug 14, 2020

Question Answer
Tickets EZP-31801
Bug fix? no
New feature? yes
BC breaks? no
Tests pass? yes
Doc needed? no
License GPL-2.0

This PR implements ContentType copy feature in the admin.

Checklist:

  • Coding standards ($ composer fix-cs)
  • Ready for Code Review

@MarioBlazek MarioBlazek changed the title Feature: Copy ContentType in admin As an Editor, I want to be able to copy content type Aug 14, 2020
@MarioBlazek MarioBlazek changed the title As an Editor, I want to be able to copy content type As an Editor, I want to be able to copy the content type in admin interface Aug 14, 2020
@MarioBlazek
Copy link
Contributor Author

PR depends on this ezsystems/ezplatform-kernel#97 as persistence cache doesn't get clear after copy() method, so the listing of content types remains the same.

@SylvainGuittard
Copy link
Contributor

Thanks @MarioBlazek for this contribution.
Could you please upload screenshot(s), so we can have a look at the overall design and user experience.

@MarioBlazek
Copy link
Contributor Author

Screenshot 2020-08-17 at 09 55 46

Screenshot 2020-08-17 at 09 56 02

@SylvainGuittard

@SylvainGuittard
Copy link
Contributor

Thanks @MarioBlazek

@inakijv
Copy link
Contributor

inakijv commented Aug 17, 2020

Hi @MarioBlazek thank you for this PR, it is a requested feature that will improve the experience of our users 👍
One thing I am not sure is that if there's a need to create a new column for the "Copy Content Type" button within the table. Other tables in the interface that contain two or more buttons have them within the same column.

@@ -55,6 +55,7 @@
<th class="ez-table__header-cell">{{ 'content_type.view.list.column.id'|trans|desc('ID') }}</th>
<th class="ez-table__header-cell">{{ 'content_type.view.list.column.modification_date'|trans|desc('Modification date') }}</th>
<th class="ez-table__header-cell"></th>
<th class="ez-table__header-cell"></th>
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if we need to add a new column

@MarioBlazek
Copy link
Contributor Author

@inakijv no problem, I can adjust that.

@@ -83,6 +84,11 @@
<td class="ez-table__cell">{{ content_type.identifier }}</td>
<td class="ez-table__cell">{{ content_type.id }}</td>
<td class="ez-table__cell">{{ content_type.modificationDate|ez_full_datetime }}</td>
<td class="ez-table__cell ez-table__cell--has-action-btns text-right">
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't be possible to have the new button within the same column? That way it would look more consitent with other tables in the app

href="{{ copy_url }}"
{% if class_name %}class="{{ class_name }}"{% endif %}>
<svg class="ez-icon ez-icon-copy">
Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest to add also ez-icon--secondary, given that it is not a primary action. That way it would look more consistent with other tables in the app.

@MarioBlazek
Copy link
Contributor Author

Screenshot 2020-08-17 at 11 10 43

@inakijv @SylvainGuittard adjusted.

@adamwojs adamwojs requested a review from a team Aug 17, 2020
*
* @throws \eZ\Publish\API\Repository\Exceptions\UnauthorizedException
*/
public function copyAction(ContentTypeGroup $group, ContentType $contentType): Response
Copy link
Member

Choose a reason for hiding this comment

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

Hmmm I'm missing here CSRF protection....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@adamwojs do you have an example of handling the CSRF in the ezplatform-admin-ui?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can create a simple form in controller to create and validate a CSRF.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yup, that's what we do too. You could also drop GET action in routing then.

@adamwojs adamwojs requested a review from a team Aug 17, 2020
@SylvainGuittard
Copy link
Contributor

@MarioBlazek Thanks for adjusting the design. 👍

@lserwatka
Copy link
Member

@MarioBlazek we would like to include this feature in 3.2. Do you have time to finish it and address changes after code review? Rebase is needed as well.

@MarioBlazek
Copy link
Contributor Author

Sorry, the last few days I was in a rush. Will update this PR tomorrow, the latest.

@lserwatka
Copy link
Member

@MarioBlazek thank you, this is great contribution, we should use 3.2 window and release it.

@lserwatka
Copy link
Member

You don't need to fix design, we will take over, as we are in the middle of changing look & feel.

@MarioBlazek MarioBlazek force-pushed the feature_content_type_copy branch from 2edd51e to bdf3c59 Compare Sep 11, 2020
@MarioBlazek
Copy link
Contributor Author

@lserwatka @ViniTou PR updated.

@dew326
Copy link
Member

dew326 commented Sep 11, 2020

I removed the duplicated table cell.

//cc @lserwatka The new design was already there.
Zrzut ekranu 2020-09-11 o 13 11 43

@MarioBlazek
Copy link
Contributor Author

I have just realized that form handling is not working properly. There is already one form for bulk delete out there, so multiple copy forms are added inside it. What do you guys suggest?

@dew326
Copy link
Member

dew326 commented Sep 11, 2020

But were those forms are added? I can see there is only added the link to the copy route, and in fact, it doesn't work because it redirects to the content types list.

@MarioBlazek MarioBlazek force-pushed the feature_content_type_copy branch from 52b3797 to 1bb5d16 Compare Sep 11, 2020
@MarioBlazek
Copy link
Contributor Author

MarioBlazek commented Sep 11, 2020

@dew326 I have update the PR with forms. Everything should be ok now. Please review.

@@ -86,6 +86,10 @@
<td class="ez-table__cell">{{ content_type.id }}</td>
<td class="ez-table__cell">{{ content_type.modificationDate|ez_full_datetime }}</td>
<td class="ez-table__cell ez-table__cell--has-action-btns">
<td class="ez-table__cell ez-table__cell--has-action-btns text-right">
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<td class="ez-table__cell ez-table__cell--has-action-btns text-right">

kaff
kaff approved these changes Sep 21, 2020
@lserwatka
Copy link
Member

What is the status here? We are 1 week away from 3.2-rc. If we won't merge it this week, we will need it push it back till December.

@dew326 dew326 force-pushed the feature_content_type_copy branch from 5da1ca5 to 8f7a4e2 Compare Oct 7, 2020
@dew326 dew326 requested review from GrabowskiM and lucasOsti Oct 7, 2020
@dew326 dew326 requested a review from a team Oct 7, 2020
['%name%' => $contentType->getName()],
'content_type'
);
} catch (UnauthorizedException $exception) {
Copy link
Contributor

Choose a reason for hiding this comment

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

When this exception can occur? Is $this->denyAccessUnlessGranted(new Attribute('class', 'create')); not enough?

@@ -346,6 +346,16 @@ ezplatform.content_type.edit:
requirements:
contentTypeGroupId: \d+

ezplatform.content_type.copy:
path: /contenttypegroup/{contentTypeGroupId}/contenttype/{contentTypeId}/copy
methods: ['GET', 'POST']
Copy link
Contributor

Choose a reason for hiding this comment

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

I would stick with POST only.

@ViniTou ViniTou requested a review from adamwojs Oct 7, 2020
Copy link
Member

@adamwojs adamwojs left a comment

Choose a reason for hiding this comment

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

+1 besides @ViniTou comments 😉

Copy link
Member

@Nattfarinn Nattfarinn left a comment

Choose a reason for hiding this comment

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

Hmm, user will end up with two Content Types with the very same name. Even if we prefix the name with "Copy" or something, the name will most likely be up to immediate change to something more meaningful. Wouldn't it be better to make it consistent with what we already have in ie. role Copy?

This way would avoid making JS changes to form action and other workarounds for "Form inside Form" issue as well.

Ping: @NataliaBecla @SylvainGuittard

Role copy as reference:
Screenshot from 2020-10-07 16-13-20

@lserwatka lserwatka merged commit e4e2333 into ezsystems:master Oct 12, 2020
1 check passed
@lserwatka
Copy link
Member

@Nattfarinn we will address it after RC.

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