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

IBX-426: Created additional UDW config for adding Location to Content #1772

Merged
merged 3 commits into from
Jun 16, 2021

Conversation

barw4
Copy link
Member

@barw4 barw4 commented Jun 1, 2021

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

Add location action should not take into account any content/create restrictions, it should allow every content to be used as a destination if the user can content/read it.

Checklist:

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

@barw4 barw4 self-assigned this Jun 1, 2021
@barw4 barw4 requested a review from a team June 1, 2021 19:06
@@ -105,7 +105,7 @@

{% macro table_header_tools(form_add, form_remove, can_add) %}
<button
data-udw-config="{{ ez_udw_config('single', {}) }}"
data-udw-config="{{ ez_udw_config('add_location', {}) }}"
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure about this one. @Nattfarinn correct me if I'm wrong but changing config name here is a BC break in case someone already used custom configuration for single.

Copy link
Member

@Nattfarinn Nattfarinn Jun 15, 2021

Choose a reason for hiding this comment

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

There is a tiny risk of bahavioral BC here, but I wouldn't bother much with it. I don't think anyone ever would change our generic single configuration to be anything else but multiple: false (and even if he did we don't change configuration itself [big BC], but configuration key used just for this use-case).

@adamwojs
Copy link
Member

Could you please rebase PR against current 2.2 branch? This should fix issues reporeted by CI.

@Steveb-p Steveb-p requested a review from a team June 14, 2021 10:14
@barw4 barw4 changed the base branch from 2.2 to 2.3 June 14, 2021 10:23
Copy link
Contributor

@Steveb-p Steveb-p left a comment

Choose a reason for hiding this comment

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

I'm no expert in UDW, but LGTM.

@micszo micszo self-assigned this Jun 16, 2021
Copy link
Member

@micszo micszo left a comment

Choose a reason for hiding this comment

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

QA Approved on Ibexa Content, Experience, Commerce v3.3.4 with diff.

@micszo micszo removed their assignment Jun 16, 2021
@lserwatka lserwatka merged commit 2e3943d into 2.3 Jun 16, 2021
@lserwatka lserwatka deleted the ibx-426-adding-location-no-restricted-cts branch June 16, 2021 13:39
@lserwatka
Copy link
Member

You can merge it up now.

@barw4
Copy link
Member Author

barw4 commented Jun 16, 2021

Merged into master: ed9fdfe

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