Arvados

• ebd8e5c fixes bug.
• 8e7d4e8 adds debugger to bundle.

Review at 8e7d4e83

If we're now ignoring @name_link entirely when producing breadcrumbs, then this code in apps/workbench/app/views/layouts/body.html.erb should be revisited too:

<% if @name_link or @object %>
...
  <% project_breadcrumbs.each do |p| %>

LGTM otherwise, thanks.

Tim Pierce wrote:

Review at 8e7d4e83

If we're now ignoring @name_link entirely when producing breadcrumbs, then this code in apps/workbench/app/views/layouts/body.html.erb should be revisited too:
[...]

LGTM otherwise, thanks.

Indeed, and there's actually a whole lot more name_link cruft still sitting around that isn't needed after #3036. I'd like to resolve the infinite loop bug without blocking on fixing all that, though, and ignoring @name_link isn't necessary to fix this bug, so I've backed out that part.

Now at 2663cf4 -- look OK?

Tom Clegg wrote:

Indeed, and there's actually a whole lot more name_link cruft still sitting around that isn't needed after #3036. I'd like to resolve the infinite loop bug without blocking on fixing all that, though, and ignoring @name_link isn't necessary to fix this bug, so I've backed out that part.

Now at 2663cf4 -- look OK?

Focusing on just fixing the infinite loop makes sense to me. In the new change, I think that if @name_link is set, then current.is_a?(Group) will never be true and no breadcrumbs will be created.

If that's acceptable, then go ahead and merge. I'm just checking that we're aware of that exception.

Review at e4db374

I think I'm getting more confused with each new commit!

current = @name_link.andand.tail_uuid || @object.andand.owner_uuid
while current.is_a?(Group) and current.group_class == 'project'
  ...

After the first line, current will be a string, or nil. It will never pass current.is_a?(Group).

I think this requires running ArvadosBase.find on the uuid that's returned, or does that happen automagically?

Tim Pierce wrote:

Review at e4db374

I think I'm getting more confused with each new commit!

Me too!

I decided to go back to basics: just detect the infinite loop and halt. 23aea08

Now at e34d685 with master merged.

OK, this makes sense. Is there an easy way we could construct a test for this? Could we add test fixtures for a circular name link chain and write a functional test for project_breadcrumbs, or will the model constraints reject the circular name chain? (IIRC this is done in the controller rather than the model, but I may be thinking of circular ownership instead.)

Added test in ed7369d

Tom Clegg wrote:

Added test in ed7369d

LGTM. Thanks very much.