Batch copy: copy permission too by alikon · Pull Request #24736 · joomla/joomla-cms

alikon · 2019-04-27T09:48:08Z

follow up #24730.

Summary of Changes

copy permission when Batch copy items to a new category

Testing Instructions

Create an article.
Go to its permission tab and make a change. (e.g registered->allow creating)
Go to Batch and Copy it somewhere else.

Expected result

the permission are copyied

Actual result

permission are not copied

richard67 · 2019-04-27T10:07:20Z

I have tested this item ✅ successfully on c72d6f2

I've tested with 2 articles which had differenc ACL modifications.
Both articles copied in 1 batch action.
Before patch: Copied articles have ACL like in new articles.
After patch: Each of the article copies has ACL like the particular copy source article.

@HLeithner and @wilsonge Release leads please check as also with PR #24730 if this is a bug fix so it goes into 3.9.next or a new feature for 4.0.

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/24736.}

viocassel · 2019-04-27T11:16:00Z

I have tested this item ✅ successfully on c72d6f2

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/24736.}

ghost · 2019-04-27T11:22:56Z

Status "Ready To Commit".

wilsonge · 2019-04-27T15:36:51Z

I'm not unhappy to call this a bug fix - but what happens if there isn't a entry in the permissions table for the asset - shouldn't we check in JTable that there actually is a asset_id for items

infograf768 · 2019-04-27T17:25:50Z

I agree, for this one, with @wilsonge
I guess we may not need that check for categories and modules.

wilsonge · 2019-04-27T17:54:03Z

Agreed. The stuff that's component specific is fine. But this generic one needs more careful sanity checks

alikon · 2019-04-28T07:41:30Z

not so sure what "sanity check" you are talking about if target and/or source asset don't exists the query simply does nothing

infograf768 · 2019-04-28T13:41:04Z

I guess just making sure there is an existing asset for the original item.

if ($oldAssetId)

query

infograf768 · 2019-06-05T17:48:14Z

@wilsonge
the rtc label has been taken off without a reply to @alikon
Please explain further what you expect to get this in.

libraries/src/MVC/Model/AdminModel.php

HLeithner · 2019-06-05T18:01:41Z

I removed it because George requested more sanity checks, I'm also not sure if this is a bugfix because permission copying is missing or a new feature.

Atm I think it's a bugfix but ymmv.

alikon · 2019-06-05T18:06:08Z

for sanity check i've already answered #24736 (comment)
😕

HLeithner · 2019-06-05T18:10:51Z

@wilsonge could you please elaborate your comment?

richard67 · 2019-08-03T10:14:22Z

@alikon why closing this?

richard67 · 2019-08-03T17:39:14Z

@alikon Did you maybe accidently hit the wrong button, close instead of rebase?

alikon · 2019-08-04T09:17:05Z

i've had 5 minutes of "send all to the hell" feeling...
...a little bit better now

richard67 · 2019-08-04T10:58:53Z

@wilsonge The syntax for the join, does the fix for the "ON" which you have done in J4 here a18322d apply also to J3? If so, it should be also fixed here in this PR. I will then make PR for Nicola's branch but want you to confirm if that would be right.

richard67 · 2019-08-04T11:11:35Z

@wilsonge I just see according to J3 database api doc my previous comment for you is wrong, i.e. this PR here is ok with "... ON ... " in 1 join condition. Is that right?

Regarding your sanity check, Nicola has merged noth PRs from @Quy so now it should be ok. Maybe reason why moving things down did not work is because the $this->table->get('id'); would be before that then and reset stuff somehow?

Fix PHPCS for Joomla CMS PR 24736

libraries/src/MVC/Model/AdminModel.php

HLeithner · 2019-08-05T06:58:14Z

libraries/src/MVC/Model/AdminModel.php

 			// Get the new item ID
 			$newId = $this->table->get('id');

+			if (!empty($oldAssetId))


Suggested change

if (!empty($oldAssetId))

if ($this->table->hasField($this->table->getColumnAlias('asset_id')) && !empty($this->table->asset_id))

HLeithner · 2019-08-05T06:58:55Z

libraries/src/MVC/Model/AdminModel.php


+			if (!empty($oldAssetId))
+			{
+				// Copy rules


Suggested change

// Copy rules

// Copy rules

$oldAssetId = $this->table->asset_id;

alikon · 2019-08-06T17:32:31Z

@HLeithner cause i'm not too much able to read only & fully understand code
i've made a test with your suggestion applyied ...... as a result permission are not copyied

so may I ask you to do the same test?
with your suggestion and without (only this silly pr)
........maybe you'll can confirm or deny my results

Quy · 2019-08-07T05:09:20Z

libraries/src/MVC/Model/AdminModel.php

@@ -487,6 +494,21 @@ protected function batchCopy($value, $pks, $contexts)
 			// Get the new item ID


After this point $this->table->asset_id has incremented. Thus the old value must be stored before and not after this point. The PR works as it.

ok maybe I missed something in the code.

richard67 · 2019-08-07T05:32:48Z

@Quy That was what I supposed, too, but seems that comment in code reviews was too hidden.

richard67 · 2019-08-07T05:36:08Z

Tonight I can test again. But last changes were code style only so my old test should still be ok.

richard67 · 2019-08-07T17:21:48Z

I have tested this item ✅ successfully on 6b24187

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/24736.}

richard67 · 2019-08-07T17:23:05Z

@viocassel or @Quy could you test again? Thanks in advance.

viocassel · 2019-08-07T17:30:12Z

I have tested this item ✅ successfully on 6b24187

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/24736.}

richard67 · 2019-08-07T17:31:30Z

@viocassel Thanks for testing.

ghost · 2019-08-07T17:33:20Z

Status "Ready To Commit".

HLeithner · 2019-08-08T11:21:01Z

Thank you for your engagement in Joomla!

copy permission too

c72d6f2

joomla-cms-bot added the PR-staging label Apr 27, 2019

alikon mentioned this pull request Apr 27, 2019

com_categories: batch copy not copy permission #24730

Merged

alikon changed the title ~~copy permission too~~ Batch copy: copy permission too Apr 27, 2019

richard67 mentioned this pull request Apr 27, 2019

batch copy permission on module #24737

Merged

ghost removed the PR-staging label Apr 27, 2019

joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Apr 27, 2019

richard67 mentioned this pull request Apr 27, 2019

Semver j3 #24728

Closed

Merge branch 'staging' into patch-118

420618c

joomla-cms-bot added the PR-staging label Apr 28, 2019

roland-d and others added 2 commits May 18, 2019 23:27

Merge branch 'staging' into patch-118

8f2b75f

Merge branch 'staging' into patch-118

2e23232

HLeithner removed the RTC This Pull Request is Ready To Commit label Jun 5, 2019

Merge branch 'staging' into patch-118

c8e399d

joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Jun 5, 2019

Quy reviewed Jun 5, 2019

View reviewed changes

libraries/src/MVC/Model/AdminModel.php Outdated Show resolved Hide resolved

cs

e2f6555

Merge branch 'staging' into patch-118

c243230

alikon closed this Aug 3, 2019

joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Aug 3, 2019

alikon reopened this Aug 4, 2019

Merge branch 'staging' into patch-118

639b9b9

richard67 and others added 2 commits August 4, 2019 13:14

Fix PHPCS

7fa45d3

Merge pull request #58 from richard67/patch-2

6b24187

Fix PHPCS for Joomla CMS PR 24736

HLeithner reviewed Aug 5, 2019

View reviewed changes

Quy reviewed Aug 7, 2019

View reviewed changes

joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Aug 7, 2019

infograf768 and others added 2 commits August 8, 2019 09:23

Merge branch 'staging' into patch-118

5404e19

Merge branch 'staging' into patch-118

d6c3f4d

HLeithner merged commit 107ddfe into joomla:staging Aug 8, 2019

joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Aug 8, 2019

HLeithner added this to the Joomla! 3.9.11 milestone Aug 8, 2019

alikon deleted the patch-118 branch August 13, 2019 06:28

alikon mentioned this pull request Oct 10, 2020

[4][SQL] - batch copy #31036

Merged

	if (!empty($oldAssetId))
	if ($this->table->hasField($this->table->getColumnAlias('asset_id')) && !empty($this->table->asset_id))

	// Copy rules
	// Copy rules
	$oldAssetId = $this->table->asset_id;

		@@ -487,6 +494,21 @@ protected function batchCopy($value, $pks, $contexts)
		// Get the new item ID

Uh oh!

Conversation

alikon commented Apr 27, 2019

Summary of Changes

Testing Instructions

Expected result

Actual result

Uh oh!

richard67 commented Apr 27, 2019

Uh oh!

viocassel commented Apr 27, 2019

Uh oh!

ghost commented Apr 27, 2019

Uh oh!

wilsonge commented Apr 27, 2019

Uh oh!

infograf768 commented Apr 27, 2019

Uh oh!

wilsonge commented Apr 27, 2019

Uh oh!

alikon commented Apr 28, 2019

Uh oh!

infograf768 commented Apr 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

infograf768 commented Jun 5, 2019

Uh oh!

Uh oh!

HLeithner commented Jun 5, 2019

Uh oh!

alikon commented Jun 5, 2019

Uh oh!

HLeithner commented Jun 5, 2019

Uh oh!

richard67 commented Aug 3, 2019

Uh oh!

richard67 commented Aug 3, 2019

Uh oh!

alikon commented Aug 4, 2019

Uh oh!

richard67 commented Aug 4, 2019

Uh oh!

richard67 commented Aug 4, 2019

Uh oh!

Uh oh!

Uh oh!

HLeithner Aug 5, 2019

Choose a reason for hiding this comment

Uh oh!

HLeithner Aug 5, 2019

Choose a reason for hiding this comment

Uh oh!

alikon commented Aug 6, 2019

Uh oh!

Quy Aug 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HLeithner Aug 7, 2019

Choose a reason for hiding this comment

Uh oh!

richard67 commented Aug 7, 2019

Uh oh!

richard67 commented Aug 7, 2019

Uh oh!

richard67 commented Aug 7, 2019

Uh oh!

richard67 commented Aug 7, 2019

Uh oh!

viocassel commented Aug 7, 2019

Uh oh!

richard67 commented Aug 7, 2019

Uh oh!

ghost commented Aug 7, 2019

Uh oh!

HLeithner commented Aug 8, 2019

Uh oh!

Reviewers

Assignees

Labels

infograf768 commented Apr 28, 2019 •

edited

Loading

Quy Aug 7, 2019 •

edited

Loading