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

[4.0] Fix batch copy for articles #24519

Closed
wants to merge 43 commits into from

Conversation

Arpit-24
Copy link
Contributor

@Arpit-24 Arpit-24 commented Apr 6, 2019

Pull Request for Issue #24478 .

Summary of Changes

Add workflow_assosciations entry for the new articles

Testing Instructions

Install sample data
Select all articles
Select batch
Select a category and copy

Expected result

Articles are copied to the selected category

Arpit-24 and others added 14 commits March 24, 2019 02:41
Hides the header and options in full screen mode to prevent overlap over the editor area in full-screen mode.

Another possible alternative is displaying both the options as well as the editor in full-screen mode but purely a user preference. Because the need for save and other options is not as important in full-screen mode.
@Arpit-24
Copy link
Contributor Author

Arpit-24 commented Apr 6, 2019

Screenshot (1285)

Testing Result

@Quy Quy mentioned this pull request Apr 6, 2019
@ghost
Copy link

ghost commented Apr 7, 2019

I have tested this item 🔴 unsuccessfully on 5528b0e

Article copied successfully, but open a Categorie got Warning: Declaration of Joomla\Component\Categories\Administrator\Model\CategoryModel::batchCopy($value, $pks, $contexts) should be compatible with Joomla\CMS\MVC\Model\AdminModel::batchCopy($value, $workflowid, $pks, $contexts) in /administrator/components/com_categories/Model/CategoryModel.php on line 39

Maybe this Pull Request can not be tested by Patchtester.

System information

Setting Value
PHP Built On Linux lamp122.cloudaccess.net 3.10.0-962.3.2.lve1.5.24.7.el6h.x86_64 nr.1 SMP Mon Dec 17 12:02:35 EST 2018 x86_64
Database Type mysql
Database Version 5.7.24-cll-lve
Database Collation utf8_general_ci
Database Connection Collation utf8_general_ci
PHP Version 7.1.26
Web Server Apache
WebServer to PHP Interface cgi-fcgi
Joomla! Version Latest Nightly Build Update from Joomla! 4.0.0-alpha8-dev Development [ Amani ] 9-March-2019 13:41 GMT
User Agent Mozilla/5.0 (Macintosh; Intel Mac OS X 10.13; rv:66.0) Gecko/20100101 Firefox/66.0

This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/24519.

@infograf768
Copy link
Member

infograf768 commented Apr 7, 2019

Looks like the problem here is that modifying the Joomla\CMS\MVC\Model\AdminModel::batchCopy by adding $workflowid may be used by another component extending AdminModel, while it concerns only articles.
Therefore categories get warning as well as com_fields. We get errors for contacts, etc.

@bembelimen
Copy link
Contributor

If you have the old ID, you don't need this parameter. One can just get the default stage from the workflow (linked in the new category) when copied, so the article is resetted or just don't change anything when moved.

@Arpit-24
Copy link
Contributor Author

Arpit-24 commented Apr 7, 2019

If you have the old ID, you don't need this parameter. One can just get the default stage from the workflow (linked in the new category) when copied, so the article is resetted or just don't change anything when moved.

Thank you for the help, updated the PR. Now it no longer has the additional param of workflow_id in the function definition.

@franz-wohlkoenig @infograf768 Please re-test

->where($db->quoteName('item_id') . ' = ' . $pk);
$db->setQuery($query);

$results = $db->loadObjectList();
Copy link
Contributor

Choose a reason for hiding this comment

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

->loadObject()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made the required changes

$query->clear()
->insert($db->quoteName('#__workflow_associations'))
->columns(array($db->quoteName('item_id'), $db->quoteName('stage_id'), $db->quoteName('extension')))
->values($newId . ', ' . $old_stage_id . ', \'' . $oldExtension . '\'');
Copy link
Contributor

Choose a reason for hiding this comment

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

->quote(...)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changing this actually gives error in my PC

Copy link
Member

Choose a reason for hiding this comment

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

->values($db->quote($newId) . ',' . $db->quote($old_stage_id) . ',' . $db->quote($oldExtension)); does it here. See my zip below.

$query = $db->getQuery(true)
->select($db->quoteName(array('stage_id','extension')))
->from($db->quoteName('#__workflow_associations'))
->where($db->quoteName('item_id') . ' = ' . $pk);
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing context/extension

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 am afraid I don't understand what you mean

}
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to add this changes here.

Copy link
Member

Choose a reason for hiding this comment

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

@bembelimen
In fact, is indeed missing a ending return for this file in core.

@Quy Quy removed the Updates Requested Indicates that this pull request needs an update from the author and should not be tested. label Jul 20, 2019
@Nilesh5995
Copy link

I have tested this item ✅ successfully on 62a78a1

I have tested this item successfully


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/24519.

@Schmidie64
Copy link
Contributor

I have tested this item ✅ successfully on 62a78a1

Work's as described.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/24519.

@conseilgouz
Copy link
Contributor

I have tested this item ✅ successfully on 62a78a1


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/24519.

@pascalelocher
Copy link

I have tested this item ✅ successfully on 62a78a1

PHP Version 7.3.8
Web Server Apache
WebServer to PHP Interface cgi-fcgi
Joomla! Version Joomla! 4.0.0-beta1-dev Development [ Amani ] 17-October-2019 20:21 GMT
User Agent Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:69.0) Gecko/20100101 Firefox/69.0


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/24519.

@adiheutschi
Copy link

I have tested this item ✅ successfully on 62a78a1

Success

Setting Value
PHP Built On Linux server2.adiheutschi.ch 3.10.0-962.3.2.lve1.5.24.9.el7.x86_64 #1 SMP Wed Feb 13 08:24:50 EST 2019 x86_64
Database Type mysql
Database Version 10.2.27-MariaDB
Database Collation latin1_swedish_ci
Database Connection Collation utf8mb4_general_ci
PHP Version 7.3.10
Web Server LiteSpeed
WebServer to PHP Interface litespeed
Joomla! Version Joomla! 4.0.0-beta1-dev Development [ Amani ] 17-October-2019 20:21 GMT
User Agent Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/77.0.3865.120 Safari/537.36


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/24519.

@alikon
Copy link
Contributor

alikon commented Oct 20, 2019

RTC


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/24519.

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Oct 20, 2019
@richard67
Copy link
Member

I've stumpled over the issue which is fixed by this PR and so found this PR and checked in detail what it does. To be honest: I am not really happy with it.

It overrides the batchCopy routine of the base class with (except of a few codestyle enhancements) identical code just to add then the creation of the workflow association record. This produces duplicate code.

Wouldn't it not be easier just to add following code to routine cleanupPostBatchCopy in the article model below line 92, i.e. after the if with the sql insert of the new record into the #__content_frontpage table (which should also be changed bit that's another issue)?

		// Copy workflow association
		$workflow = new Workflow(['extension' => 'com_content']);
		$assoc = $workflow->getAssociation($oldId);
		$workflow->createAssociation($newId, (int) $assoc->stage_id);

These 3 lines should lead to the same result as this PR does.

Folks, please comment, maybe I am wrong or am missing some aspect of it.

@infograf768
Copy link
Member

@bembelimen
Please check this and infograf768#55 as we are not sure about handling workflow for this.

@Quy Quy removed the RTC This Pull Request is Ready To Commit label Nov 3, 2019
@Quy Quy mentioned this pull request Nov 12, 2019
@richard67
Copy link
Member

Please test PR #26835 . Is solves the same problem as this PR here, but in a more elegant way.

@wilsonge
Copy link
Contributor

Closing in favour of #26835 which uses the API and doesn't require full method duplication

@wilsonge wilsonge closed this Nov 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet