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

[com_weblinks] Make ACL core.edit.own work (PR for 11466) #255

Merged
merged 6 commits into from Aug 15, 2016

Conversation

andrepereiradasilva
Copy link
Contributor

Pull Request for Issue joomla/joomla-cms#11466 (com_weblinks part).

Summary of Changes

The ACL core.edit.own is not working in com_weblinks.
This PR makes it work.

Testing Instructions

  1. Use latest staging
  2. Besides the Super User, create a user "test" added to "Administrator" group
  3. Go to com_weblinks Options, Permissions tab and diable "Edit" for "Administrator" group
  4. Now create a new weblink with the "test" user (use another browser or a private window)
  5. Try to edit that item. You can't edit our own. Bug
  6. Now do the same test but with a weblink category Permission. You can't edit our own. Bug
  7. Apply patch, repeat step 5. and 6. and now you can edit our own items.
  8. Code review

Also use the two users to do a general test with the com_weblinks edit permissions to confirm all is fine.

Documentation Changes Required

None.

Notes

This problem was discovered in GsoC 2016 Multilingual project.

This is very similiar with joomla/joomla-cms#11502 (com_newsfeeds) and joomla/joomla-cms#11503 (com_contact).

@andrepereiradasilva
Copy link
Contributor Author

i guess the test errors have nothing to do with this PR ...

$canEditOwn = $user->authorise('core.edit.own', $this->option . '.category.' . (int) $item->catid) && $item->created_by == $user->id;

// Check the category core.edit permissions.
return $canEditOwn || $user->authorise('core.edit', $this->option . '.category.' . (int) $item->catid);
Copy link
Member

Choose a reason for hiding this comment

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

Can you save that in a variable before? That looks really not nice ;-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what variable? you want a $canEdit ? is it really needed .... ?

Copy link
Member

@yvesh yvesh Aug 7, 2016

Choose a reason for hiding this comment

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

Just my opinion :) That return statement is not good readable imo.

Btw. Thank you for your great work!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the idea is to only check authorize to core.edit if user cannot edit own.

Copy link
Member

Choose a reason for hiding this comment

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

That's fine, but you could also save that to a variable ;-)

$canEdit = $canEditOwn || $user->..

return $canEdit; 

But as i said that's just my opinion :-)

Copy link
Contributor

Choose a reason for hiding this comment

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

Personally I'd say don't change it. Unless it's going to improve readability or going to be reused I try to not create variables needlessly. This is a case where it's doing neither IMO. Of course, coding styles and opinions have a lot of things in common 😉

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'm like @mbabker on this. if we don't use a variable i don't like to create it.
but yes is more a matter of coding style.

@yvesh
Copy link
Member

yvesh commented Aug 7, 2016

@andrepereiradasilva i get an SQL syntax error when i apply your patch. System tests also fail because of that

screenshot 2016-08-07 09 43 14

@yvesh
Copy link
Member

yvesh commented Aug 7, 2016

A comma is missing after a.created_by

@@ -132,7 +132,7 @@ protected function getListQuery()
$query->select(
$this->getState(
'list.select',
'a.id, a.title, a.alias, a.checked_out, a.checked_out_time, a.catid,' .
'a.id, a.title, a.alias, a.checked_out, a.checked_out_time, a.catid, a.created_by ' .
Copy link
Member

Choose a reason for hiding this comment

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

Comma missing after a.created_by

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ups sorry will correct

@alikon
Copy link
Contributor

alikon commented Aug 7, 2016

i've tested successfully

@infograf768
Copy link
Contributor

OK here

@yvesh yvesh added the RTC label Aug 7, 2016
@yvesh
Copy link
Member

yvesh commented Aug 8, 2016

@alikon @infograf768 Thanks for testing, please use the issue tracker next time like on core :-)

@yvesh
Copy link
Member

yvesh commented Aug 8, 2016

I have tested this item ✅ successfully on cfd842f


This comment was created with the J!Tracker Application at issues.joomla.org/weblinks/255.

@infograf768
Copy link
Contributor

Didn't even know we have a specific issue tracker for such project.
Found it: https://issues.joomla.org/tracker/weblinks

@andrepereiradasilva
Copy link
Contributor Author

will this be merged?

And BTW when there will be a new weblinks release?

@yvesh
Copy link
Member

yvesh commented Aug 14, 2016

// cc @chrisdavenport @rdeutz

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants