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

Only allow admins to delete tags #2512

Merged
merged 2 commits into from
Jan 6, 2017
Merged

Only allow admins to delete tags #2512

merged 2 commits into from
Jan 6, 2017

Conversation

nickvergessen
Copy link
Member

Step 5 of #1465

@MorrisJobke @rullzer @ChristophWurst

Btw morris or christoph, can one of you fix the form and the label (admin only now). It seems somewhat cut off and the tooltip is behind it as well.

bildschirmfoto vom 2016-12-05 16-55-01

@nickvergessen nickvergessen added this to the Nextcloud 11.0 milestone Dec 5, 2016
@mention-bot
Copy link

@nickvergessen, thanks for your PR! By analyzing the history of the files in this pull request, we identified @MorrisJobke and @LukasReschke to be potential reviewers.

@MorrisJobke
Copy link
Member

Btw morris or christoph, can one of you fix the form and the label (admin only now). It seems somewhat cut off and the tooltip is behind it as well.

cc @nextcloud/designers - maybe @skjnldsv want to look into this

@LukasReschke
Copy link
Member

@MorrisJobke CI runs amok again – can you check?

@MorrisJobke
Copy link
Member

@MorrisJobke CI runs amok again – can you check?

Yes - tests are failing because of changed behaviour. not-my-business

@LukasReschke
Copy link
Member

@MorrisJobke Thou shall click through all links:

https://drone.nextcloud.com/nextcloud/server/2689/37

@LukasReschke
Copy link
Member

+ git init
0s
2
Initialized empty Git repository in /drone/src/github.com/nextcloud/server/.git/
0s
3
+ git remote add origin https://github.com/nextcloud/server.git
0s
4
+ git fetch --no-tags --depth=1 origin +refs/pull/2512/merge:
0s
5
From https://github.com/nextcloud/server
6s
6
 * branch            refs/pull/2512/merge -> FETCH_HEAD
6s
7
+ git checkout -qf FETCH_HEAD
6s
8
+ git submodule update --init --recursive
6s
9
Submodule '3rdparty' (https://github.com/nextcloud/3rdparty.git) registered for path '3rdparty'
6s
10
Cloning into '/drone/src/github.com/nextcloud/server/3rdparty'...
6s
11
fatal: write error: No space left on device
7s
12
fatal: index-pack failed
7s
13
fatal: clone of 'https://github.com/nextcloud/3rdparty.git' into submodule path '/drone/src/github.com/nextcloud/server/3rdparty' failed
7s
14
exit status 128

@nickvergessen nickvergessen added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Dec 5, 2016
@icewind1991
Copy link
Member

Btw morris or christoph, can one of you fix the form and the label (admin only now). It seems somewhat cut off and the tooltip is behind it as well.

#2515

@MorrisJobke
Copy link
Member

@MorrisJobke Thou shall click through all links:

Fixed, but the tests are still broken ;)

@nickvergessen nickvergessen force-pushed the cleanup-system-tag-usage branch from b118862 to afe16d5 Compare December 6, 2016 13:55
@nickvergessen
Copy link
Member Author

Updated the test

@nickvergessen nickvergessen added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Dec 6, 2016
@MorrisJobke
Copy link
Member

1) OCA\DAV\Tests\unit\SystemTag\SystemTagNodeTest::testDeleteTag with data set #1 (false)
Sabre\DAV\Exception\Forbidden: No permission to delete tag 1
/drone/src/github.com/nextcloud/server/apps/dav/lib/SystemTag/SystemTagNode.php:164
/drone/src/github.com/nextcloud/server/apps/dav/tests/unit/SystemTag/SystemTagNodeTest.php:256

Signed-off-by: Joas Schilling <coding@schilljs.com>
@nickvergessen nickvergessen force-pushed the cleanup-system-tag-usage branch from afe16d5 to b8e9d25 Compare December 6, 2016 15:30
Signed-off-by: Joas Schilling <coding@schilljs.com>
@nickvergessen
Copy link
Member Author

Fixed, then JS was failing, also fixed now. /me hopes for 💚

@LukasReschke
Copy link
Member

@nickvergessen Can we move that to 12? I'd prefer to consider 11 closed for any non-critical merges.

@nickvergessen
Copy link
Member Author

Well since the same issue exist since 9 already I guess we can

@Spartachetto Spartachetto mentioned this pull request Dec 15, 2016
59 tasks
@LukasReschke
Copy link
Member

I'm not a huge fan of this, that said, I'm not even a huge fan of the tagging at all because it is so confusing that everybody can create a tag but now only admins can delete etc…

@nickvergessen
Copy link
Member Author

Well the idea is to not list tags in the dropdown, a user does not have applied to any files himself. In which case "deleting" would mean its not assigned to anything they see.

Copy link
Member

@MorrisJobke MorrisJobke left a comment

Choose a reason for hiding this comment

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

Tested and works 👍

$.ajax('/remote.php/dav/systemtags/2', {'method': 'DELETE'});

This should fail for a normal user 😉

Copy link
Member

@rullzer rullzer left a comment

Choose a reason for hiding this comment

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

Fine by me.

@rullzer rullzer merged commit 6347d97 into master Jan 6, 2017
@rullzer rullzer deleted the cleanup-system-tag-usage branch January 6, 2017 15:17
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.

6 participants