Support from Acquia helps fund testing for Drupal Acquia logo

Comments

larowlan’s picture

Assigned: Unassigned » larowlan
sandhya.m’s picture

Assigned: larowlan » sandhya.m
Issue tags: +LONDON_2013_APRIL
andypost’s picture

Status: Active » Needs review
Issue tags: +Needs manual testing
FileSize
7.06 KB

Suppose this should be fine!

PS: currently ordering of terms does not work for me

andypost’s picture

andypost’s picture

dawehner’s picture

Status: Needs review » Needs work
+++ b/core/modules/taxonomy/taxonomy.routing.ymlundefined
@@ -5,3 +5,10 @@ taxonomy_vocabulary_list:
+  requirements:
+    _entity_access: 'taxonomy_vocabulary.view'

I think we need something like vocabulary or terms update, as this permission is certainly open for way to many people.

andypost’s picture

@dawehner There was a issue to change list permission so this one should inherit listing access

+++ b/core/modules/taxonomy/taxonomy.routing.ymlundefined
@@ -5,3 +5,10 @@ taxonomy_vocabulary_list:
     _permission: 'administer taxonomy'
...
+    _entity_access: 'taxonomy_vocabulary.view'

This should be the same

jibran’s picture

Status: Needs work » Needs review
Issue tags: -Needs manual testing, -WSCCI, -FormInterface, -WSCCI-conversion, -LONDON_2013_APRIL

#4: tax-alpha-1976296-4.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Needs manual testing, +WSCCI, +FormInterface, +WSCCI-conversion, +LONDON_2013_APRIL

The last submitted patch, tax-alpha-1976296-4.patch, failed testing.

pcambra’s picture

Status: Needs work » Needs review
FileSize
7.08 KB

Just a plain reroll of #4

Status: Needs review » Needs work

The last submitted patch, tax-alpha-1976296-11.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
1.18 KB
7.07 KB

That was easy to fix.

Status: Needs review » Needs work

The last submitted patch, drupal-1976296-13.patch, failed testing.

jibran’s picture

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Form/VocabularyReset.phpundefined
@@ -0,0 +1,113 @@
+use Symfony\Component\DependencyInjection\ContainerInterface;

vendor namespaces should be at the end.

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Form/VocabularyReset.phpundefined
@@ -0,0 +1,113 @@
+  /**
+   * {@inheritdoc}
+   *
+   * @param \Drupal\taxonomy\VocabularyInterface $taxonomy_vocabulary
+   *   The taxonomy vocabulary to reset.

Last I know this doesn't mix.

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Form/VocabularyReset.phpundefined
@@ -0,0 +1,113 @@
+  public function buildForm(array $form, array &$form_state, VocabularyInterface $taxonomy_vocabulary = NULL) {
...
+    return parent::buildForm($form, $form_state);

This also needs $request as param @see ConfirmFormBase::buildForm

jibran’s picture

Status: Needs work » Needs review
FileSize
1.72 KB
7.46 KB

Fixed #15.

Status: Needs review » Needs work
Issue tags: -Needs manual testing, -WSCCI, -FormInterface, -WSCCI-conversion, -LONDON_2013_APRIL

The last submitted patch, 1976296-16.patch, failed testing.

jibran’s picture

Status: Needs work » Needs review
Issue tags: +Needs manual testing, +WSCCI, +FormInterface, +WSCCI-conversion, +LONDON_2013_APRIL

#16: 1976296-16.patch queued for re-testing.

tim.plunkett’s picture

Looking at this code, I think it might make more sense as extending EntityConfirmFormBase.
Those aren't just for add/edit/delete, we use it in views for the "break lock" form.
It shouldn't be too much to switch over.

jibran’s picture

Assigned: sandhya.m » Unassigned
FileSize
5.56 KB
7.4 KB

Now with EntityConfirmFormBase.

andypost’s picture

Status: Needs review » Needs work
+++ b/core/modules/taxonomy/taxonomy.routing.ymlundefined
@@ -25,3 +25,10 @@ taxonomy_vocabulary_delete:
+    _entity_access: 'taxonomy_vocabulary.view'

this access permission still wrong - see #7 & #8

tim.plunkett’s picture

Status: Needs work » Needs review
Issue tags: -Needs manual testing
FileSize
3.59 KB
7.65 KB

Rerolled with some fixes. Manually tested as well.

andypost’s picture

Status: Needs review » Reviewed & tested by the community

Great!

xjm’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs manual testing

#20 demonstrates that this does have test coverage, but let's please test manually at least once and document it on the issue? Thanks!

tim.plunkett’s picture

Issue tags: -Needs manual testing

I manually tested in #22.

When viewing the listing of an empty vocabulary, or one with one term, there was no reset button (as expected)
When viewing the listing with more than one term, while they are both in alphabetical order, the button appears, leads to the confirmation form, submits and shows the correct message, and does not alter the weight of the terms (as expected)
After manually reordering the terms to be out of order, clicking the button and confirming the form resets the terms to be in the correct order (as expected)

xjm’s picture

Status: Needs review » Reviewed & tested by the community

Thanks @tim.plunkett!

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Needs a reroll

git ac https://drupal.org/files/taxo-1976296-22.patch
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  7835  100  7835    0     0   4498      0  0:00:01  0:00:01 --:--:--  5265
error: patch failed: core/modules/taxonomy/taxonomy.admin.inc:396
error: core/modules/taxonomy/taxonomy.admin.inc: patch does not apply
Gaelan’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
7.66 KB

Rerolled.

star-szr’s picture

Issue tags: -Needs reroll

Tags.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 29907b0 and pushed to 8.x. Thanks!

Automatically closed -- issue fixed for 2 weeks with no activity.