Add delete button to versions #253

Merged
merged 3 commits into from Jan 30, 2013

Conversation

Projects
None yet
3 participants
Contributor

mvriel commented Nov 29, 2012

@Seldaek indicated on IRC that he would very much love to be able to delete
versions using the web interface of packagist. Since I figured it shouldn't
be much work and he just deleted a version for me I decided to throw some time
against the issue and behold! A new button!

The button only appears if the current user is maintainer or has the
ROLE_EDIT_PACKAGES role; the action executing this code is also protected using
same credentials.

This button replicates the behaviour of the clearVersionsCommand as indicated
by Seldaek.

I have tested this in a local setup.

p.s. the button is explicitly disabled on the first version as that is the main version
and it will only show when expanding the foldout. I assumed the interface would
become too cluttered if I placed it in the top.

Add delete button to versions
@seldaek indicated on IRC that he would very much love to be able to delete
versions using the web interface of packagist. Since I figured it shouldn't
be much work and he just deleted a version for me I decided to throw some time
against the issue and behold! A new button!

The button only appears if the current user is maintainer or has the
ROLE_EDIT_PACKAGES role; the action executing this code is also protected using
same credentials.

This button replicates the behaviour of the clearVersionsCommand as indicated
by Seldaek.

I have tested this in a local setup.
$version = $repo->getFullVersion($versionId);
+ $package = $version->getPackage();
+
+ $is_maintainer = $package->getMaintainers()->contains($this->getUser());
@stof

stof Nov 29, 2012

Contributor

Please use camelCased names for variables

+ $is_maintainer = $package->getMaintainers()->contains($this->getUser());
+ $may_edit_package = $this->get('security.context')->isGranted('ROLE_EDIT_PACKAGES');
+
+ if (!$is_maintainer || !$$may_edit_package) {
@stof

stof Nov 29, 2012

Contributor

duplicated $

+ 'PackagistWebBundle:Web:versionDetails.html.twig',
+ array(
+ 'version' => $version,
+ 'may_delete' => $is_maintainer || $may_edit_package,
@stof

stof Nov 29, 2012

Contributor

why doing this in the controller instead of the template (which is where it is done for other places) ?

@mvriel

mvriel Dec 6, 2012

Contributor

If that is preferred then I'll happily move it there. I generally prefer to keep business logic (and thus model handling) as much as possible in the controller and I consider the is_maintainer variable to be business logic (tbh: if I were religious about this I'd add a method isMaintainer to the User entity that receives a Package and use that.).

Contributor

mvriel commented Dec 6, 2012

@stof thanks for reviewing the code; I have provided feedback on why I have chosen for the location of the model interactions. Please let me know if you'd like that changed

Contributor

stof commented Dec 6, 2012

@mvriel The location of these checks should be consistent in the project IMO. Having one place doing it in the controller whereas all others are doing it in the template looks weird

Refactor button visibility business logic to template
During a code review by @Stof he indicated that it is desirable to follow suit
with the rest of the application and have the business logic determining
whether the 'delete' button is visible in the template instead of the Controller.
Contributor

mvriel commented Dec 22, 2012

I have fixed the issues mentioned in the Code reviews. Thank you for reviewing the code and let me know if there are any other issues

Owner

Seldaek commented Jan 30, 2013

Finally found time to merge this, added a csrf token for safety and fixed the styling a bit. Thanks!

@Seldaek Seldaek merged commit 28dd136 into composer:master Jan 30, 2013

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