Bug #16811

Ensure that "public favorites" still work

Added by Peter Amstutz about 1 year ago. Updated about 1 year ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
Workbench
Target version:
Start date:
09/15/2020
Due date:
% Done:

100%

Estimated time:
(Total: 0.00 h)
Story points:
-
Release relationship:
Auto

Subtasks

Task #16821: Review 16811-public-favs (both repos)ResolvedPeter Amstutz


Related issues

Related to Arvados - Bug #16007: Permission graph update is slow with large numbers of groupsResolved05/26/2020

Related to Arvados - Bug #16884: API server tests failing with "Didn't match" errorResolved

Associated revisions

Revision 56524d5f
Added by Peter Amstutz about 1 year ago

Merge branch '16811-public-favs' refs #16811

Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <>

Revision d371cc99 (diff)
Added by Peter Amstutz about 1 year ago

Tweak database seeds so the reset-to-fixture API works

refs #16811

Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <>

Revision fd6e59dc
Added by Peter Amstutz about 1 year ago

Merge branch '16811-public-favs' refs #16811

Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <>

Revision 7874e2fb (diff)
Added by Peter Amstutz about 1 year ago

16884: Add public_project permission link to test fixtures

This is the permission link that gives the all users and anonymous
users read access to public favorites. It is added by database seeds,
but it also needs to be present in the fixtures.

refs #16884
refs #16811

Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <>

Revision 76773b0a (diff)
Added by Peter Amstutz about 1 year ago

Tweak database seeds so the reset-to-fixture API works

refs #16811

Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <>

Revision 5ba3cdd2 (diff)
Added by Peter Amstutz about 1 year ago

16884: Add public_project permission link to test fixtures

This is the permission link that gives the all users and anonymous
users read access to public favorites. It is added by database seeds,
but it also needs to be present in the fixtures.

refs #16884
refs #16811

Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <>

History

#1 Updated by Peter Amstutz about 1 year ago

  • Release set to 25

#2 Updated by Peter Amstutz about 1 year ago

  • Category set to Workbench

#3 Updated by Peter Amstutz about 1 year ago

  • Assigned To set to Peter Amstutz

#4 Updated by Peter Amstutz about 1 year ago

  • Status changed from New to In Progress

#5 Updated by Peter Amstutz about 1 year ago

16811-public-favs @ arvados|3c893d29ad7a9acda0c25b47fd0c9ba5a4a5193a

16811-public-favs @ arvados-workbench2|5145f5d248a59bd87869096fddf0365766249575

This works, but

  1. I didn't add any tests yet
  2. We might want to reconsider the representation

The part to reconsider: previously, a public favorite was a "star" link owned by the "all users" group. The "all users" group is now a "role" and "role" groups cannot own things, only have outgoing links. So now adding a public favorite now involves a two step process of creating a "star" link owned by the system user, and the creating a can_read permission link from "all users" to that individual link object.

A less confusing solution would be a create a new special "Public favorites" project. This would have a well known UUID (zzzzz-j7d0g-publicfavorites) and "all users" would have can_read permission. Then, public favorites would be owned by that project. This would be more like it worked before, but it does require a database migration.

#6 Updated by Tom Clegg about 1 year ago

  • Related to Bug #16007: Permission graph update is slow with large numbers of groups added

#7 Updated by Peter Amstutz about 1 year ago

16811-public-favs @ arvados|7e358c01226d55a44ca7b6224f9c33c38510572b

16811-public-favs @ arvados-workbench2|e44d47bdea01e25926f4f3ff750f2d1cb4fd8204

  • DatabaseSeeds creates public favorites project and permission link
  • Migration ensures public favorites project and permission link exists and adjusts links to new public favorites
  • Workbench & Workbench 2 updated to use new publicfavorites project
  • Also added check to disallow system users/groups from being destroyed
  • On workbench2 the "public favorites" project shows up under "shared with me" but it will appear to be empty. I'm not sure if it should be (1) filtered out from "shared with me", (2) special-cased to display the public favorites view, or (3) the standard project panel view should include display of "star" links. I kind of like (3) the best as it would give us the option to simplify the left panel by removing "favorites" as separate panels.
  • Still needs tests

#8 Updated by Lucas Di Pentima about 1 year ago

Some comments:

  • I’m not sure the migration is reversible the way it’s written. I’ve read the documentation and it seems that only the commands listed on https://api.rubyonrails.org/v5.2.1/classes/ActiveRecord/Migration/CommandRecorder.html are auto-reversible.
  • Should the workbenches check the API version so that they can work with older clusters?
  • Regarding the UI question on wb2 fav panels: For the time being I guess the less involved option would be the first one, and then we may revisit this topic after all pending tasks for replacing wb1 are ready, wdyt?

#9 Updated by Peter Amstutz about 1 year ago

  • Subject changed from Check that "public favorites" still work to Ensure that "public favorites" still work

#10 Updated by Peter Amstutz about 1 year ago

Lucas Di Pentima wrote:

Some comments:

Instead of 'change' it runs on "up", and "down" doesn't do anything. I don't think trying to do anything in "down" is useful.

  • Should the workbenches check the API version so that they can work with older clusters?

If we were going to roll out an updated workbench without updating the API server, we would need to check the API version. But we don't support that in general, and I don't think we're going to do that for on the playground or dev clusters, either.

  • Regarding the UI question on wb2 fav panels: For the time being I guess the less involved option would be the first one, and then we may revisit this topic after all pending tasks for replacing wb1 are ready, wdyt?

Yes, that turned out to be pretty easy, I just tweaked the 'shared' query to filter out the publicfavorites project.

I added some tests. These are the first cypress tests I've written. I'm very impressed by the interactive mode, it is a great framework.

16811-public-favs arvados|a13547aec78a75da2174e083f6015280787cd597

16811-public-favs @ arvados-workbench2|91ec75d2d0e388a8818d0d4d2d7c2990cccf7f62

#11 Updated by Lucas Di Pentima about 1 year ago

  • I think we could do a reversible migration (as it seems to me it’s a trivial task) allowing a correct downgrade process in case it's needed for whatever reason, but if you don’t think that’s needed, I suppose we should be adding an upgrade note warning that public favorites may not be visible in case of a downgrade?
  • I ran wb2 tests and got a failure: https://ci.arvados.org/view/Developer/job/developer-tests-workbench2/93/

The rest LGTM.

#12 Updated by Peter Amstutz about 1 year ago

Lucas Di Pentima wrote:

  • I think we could do a reversible migration (as it seems to me it’s a trivial task) allowing a correct downgrade process in case it's needed for whatever reason, but if you don’t think that’s needed, I suppose we should be adding an upgrade note warning that public favorites may not be visible in case of a downgrade?

It actually isn't trivial.

The real problem is that fix_roles_projects migration is non-reversible, and that's the one that actually breaks public favorites (because 'star' links are no longer be owned by the 'all users' group). So I would have to adjust that migration as well to be able to fully revert the links back to the 2.0 scheme.

I don't really want to do that. However, you're right that we should document the change in the upgrade notes.

I believe that is because the arvados repo changes need to be merged first.

The rest LGTM.

#13 Updated by Peter Amstutz about 1 year ago

  • Target version changed from 2020-09-23 Sprint to 2020-10-07 Sprint

#14 Updated by Peter Amstutz about 1 year ago

  • Status changed from In Progress to Resolved

#15 Updated by Peter Amstutz about 1 year ago

  • Related to Bug #16884: API server tests failing with "Didn't match" error added

Also available in: Atom PDF