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

Fix "Save as copy" button of User Group #4798

Closed
wants to merge 3 commits into from
Closed

Fix "Save as copy" button of User Group #4798

wants to merge 3 commits into from

Conversation

joomdonation
Copy link
Contributor

PR summary

This PR fixes the issue #4778 reported by @apurvaduduskar. Basically, when you are on Joomla group edit screen, if you press "Save as Copy" button to create a new group without changing title, the system will throws an error message and doesn't create a copy group as expected.

I also fixed two ridiculous blocks of code at https://github.com/joomla/joomla-cms/blob/staging/administrator/components/com_users/models/group.php#L163 and https://github.com/joomla/joomla-cms/blob/staging/administrator/components/com_users/models/group.php#L201

How to test:

Testing this PR is simple. You just need to login to administrator area of your site, then go to Users -> Groups, click on a group to edit (don't change group title), then press "Save as Copy" button in the toolbar:

  1. Before patch: The system doesn't create the new group and throw an error message
  2. After patch: The new group is created without error message. The title of the new group is created based on title of the original group and append an increment number: Group (2), Group (3)...

@jissues-bot jissues-bot changed the title Fix "Save as copy" button of User Group #4778 Fix "Save as copy" button of User Group Oct 17, 2014
@1apweb
Copy link

1apweb commented Oct 17, 2014

Notice when I click on a group :

Strict standards: Declaration of UsersModelGroup::generateNewTitle() should be compatible with JModelAdmin::generateNewTitle($category_id, $alias, $title) in C:\UwAmp\www\pizzabug\administrator\components\com_users\models\group.php on line 18
Call Stack

Time Memory Function Location

1 0.0000 142976 {main}( ) ..\index.php:0
2 0.0600 2128984 JApplicationCms->execute( ) ..\index.php:42
3 0.0600 2129072 JApplicationAdministrator->doExecute( ) ..\cms.php:251
4 0.1200 3407752 JApplicationAdministrator->dispatch( ) ..\administrator.php:152
5 0.1200 3447592 JComponentHelper::renderComponent( ) ..\administrator.php:98
6 0.1300 3504304 JComponentHelper::executeComponent( ) ..\helper.php:332
7 0.1300 3526152 require_once( 'C:\UwAmp\www\pizzabug\administrator\components\com_users\users.php' ) ..\helper.php:352
8 0.1500 4120008 JControllerLegacy->execute( ) ..\users.php:21
9 0.1500 4120064 UsersController->display( ) ..\legacy.php:730
10 0.1600 4203816 JControllerLegacy->display( ) ..\controller.php:109
11 0.1600 4315328 JControllerLegacy->getModel( ) ..\legacy.php:650
12 0.1600 4315512 JControllerLegacy->createModel( ) ..\legacy.php:756
13 0.1600 4315656 JModelLegacy::getInstance( ) ..\legacy.php:570
14 0.1600 4352192 require_once( 'C:\UwAmp\www\pizzabug\administrator\components\com_users\models\group.php' ) ..\legacy.php:186

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

@joomdonation
Copy link
Contributor Author

Thanks @1apweb for testing. I updated the PR to solve that notice error. Please help testing it again :).

@joomlamarco
Copy link

test was successful after installing patch
using php 5.4.19, mysql 5.5.32 j3.3.6

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

@1apweb
Copy link

1apweb commented Oct 17, 2014

Hi,

Great, is ok now ! Thanks

De : Tuan Pham Ngoc [mailto:notifications@github.com]
Envoyé : vendredi 17 octobre 2014 19:13
À : joomla/joomla-cms
Cc : 1apweb
Objet : Re: [joomla-cms] Fix "Save as copy" button of User Group (#4798)

Thanks @1apweb https://github.com/1apweb for testing. I updated the PR to solve that notice error. Please help testing it again :).


Reply to this email directly or view it on GitHub #4798 (comment) .Image supprimée par l'expéditeur.

@joomdonation
Copy link
Contributor Author

Thanks for both of you for your help :).

@brianteeman
Copy link
Contributor

Setting to RTC - thanks

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

@jissues-bot jissues-bot changed the title Fix "Save as copy" button of User Group Fix "Save as copy" button of User Group Oct 17, 2014
@wingchunneil
Copy link

I get a 400 bad request error when applying this patch.

Browser message:
Bad Request
Your browser sent a request that this server could not understand.

Browser : Chrome

Joomla version:3.3.6

@N6REJ
Copy link
Contributor

N6REJ commented Oct 17, 2014

@test patch works as described ( tested via patch tester )

@jissues-bot jissues-bot added the RTC This Pull Request is Ready To Commit label Oct 17, 2014
@joomdonation
Copy link
Contributor Author

@wingchunneil: That might be something related to patch tester component.

@ALL : Thanks for help testing

@committer : Travis Failed, however it is not related to this PR, so this one can be merged.

}
}
}

if (JFactory::getApplication()->input->get('task') == 'save2copy')
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't like the idea of getting input data from models. This should be done in the controller.

Copy link
Contributor

Choose a reason for hiding this comment

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

Same method is used everywhere. Which doesn't make it correct but...

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 don't like it, too. However, in this method, there is no way for us to getting the task variable. I think this is not correct but it is still used in many places in Joomla core

@rdeutz rdeutz modified the milestone: Joomla! 3.4.0 Feb 3, 2015
@zero-24 zero-24 removed the RTC This Pull Request is Ready To Commit label Oct 14, 2015
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