Arvados

• Use the first non-empty line of the project description as the "short description": i.e., strip leading whitespace, break at the first newline, render as textile. Perhaps strip textile "header" markup, i.e., /^h[0-9]+\./, to avoid messing with the overall "list of projects" formatting.
• "infinite-scroll" and "filterable" (full text search) behaviors seem highly desirable -- you should be able to type "PGP" into a text box and find PGP data -- but not essential for a first merge.

• What is URI of this page?
• What pages/navs link to this page?
• When logged in, does this become "projects you can access", or "projects you can access but don't own"? Or is it always just public projects?

It would be ideal to share code -- and UX -- between this and the "restrict search to a particular project" part of "choose an input to this pipeline" and "choose a pipeline to run". But this might not be the best way to do a first approximation.

Tom Clegg wrote:

Possible first approximation:

• Use the first non-empty line of the project description as the "short description": i.e., strip leading whitespace, break at the first newline, render as textile. Perhaps strip textile "header" markup, i.e., /^h[0-9]+\./, to avoid messing with the overall "list of projects" formatting.

Would it maybe be a little nicer to manipulate the generated HTML directly? That way you don't have to do cheap Textile parsing hacks; you can just transform that to get the first p tag or whatever.

• What pages/navs link to this page?

I think I'd be okay if the answer for right now was "nothing." The expectation is that this page will be publicly linked from elsewhere. But it might be cool if there was a link to it in the projects dropdown too, or maybe one of the topnav pulldowns.

• When logged in, does this become "projects you can access", or "projects you can access but don't own"? Or is it always just public projects?

I think it makes the most sense for it to always be public projects. Once the user has an account, I'm worried the page will get too cluttered with results-holding projects if it lists all projects. "Can access but don't own" sounds attractive but also like a lot of API overhead that isn't necessary right now.

In case it's useful, source:apps/workbench/app/views/projects/_show_featured.html.erb and source:apps/workbench/app/assets/stylesheets/cards.css.scss are unused but still in the tree from previous UI experiments.

Something like this should get public projects:

public_projects = using_specific_api_token Rails.configuration.anonymous_user_token do
  Project.all
end

Discussed this with Tom and some highlights:

• Let's do the basic public projects display first. We will look into adding logo image afterwards

• Infinite scrolling is also optional for the first go around.

• Tom proposed "news feed" like layout rather than our current layout (table of rows with td columns). He asked me to check with Jonathan about this. Checked with Jonathan and Bryan and they preferred the current display layout (td columns for name and description).

Comments @ 1a0ba80

Controller¶

If Workbench is not configured to serve public projects, I don't think "all projects I'm allowed to see" is a very good "found" target. Rather that redirect to a page showing something different, how about "return render_not_found"? (That defaults to a "page not found" routing error, which is surely what we would implement if disabling the route entirely weren't inconvenient to test...)

Please follow the convention of putting the listed objects in @objects. Presumably, whatever view works well for public projects will work well for browsing other sets of projects, and it'll be less awkward to reuse the view code if it refers to @objects instead of @public_projects. It will also let us do stuff like this:

def public
  @objects = { ... get public projects ... }
  find_objects_for_index     # applies infinite scroll paging, additional filters/search, user-specified sorting, etc. to @objects
  ...
end

The view should have the same name as the action ("public.html.erb"). This is the page Rails will render automatically if action method doesn't redirect or render something else, so the action method will reduce to this:

  def public
    return render_not_found if not Rails.configuration.anonymous_user_token
    @objects = using_specific_api_token Rails.configuration.anonymous_user_token do
      Group.where(group_class: 'project').order("updated_at DESC")
    end
  end

View¶

Couple of tab characters in apps/workbench/app/views/projects/public_projects.html.erb

The "arv-public-projects" pseudoclass doesn't seem to do anything. Ditto style="width:100%;" -- doesn't the bootstrap "table" style already do this?

Add tbody tags around the content rows.

Tests¶

I don't think the "public projects are not configured" tests should be yet more inordinately expensive integration tests. It seems like they would work just as well if they were short controller tests. You can search assigns(:objects) to confirm that the appropriate set of projects has been retrieved.

Likewise, a controller test would be a more suitable way to verify that permissions are being applied correctly.

The permission test for the logged-in case should confirm that a non-public project visible to the current user is not shown.

The view is so simple it seems superfluous to have integration tests at all. 99% of the time is going to be spent rendering layouts in Workbench and then rendering layouts with Firefox -- all for the sake of parsing some HTML, which can be done in a controller test.

The "user" and "no user" cases are so different they would be better off as separate tests, rather than looping over "if ... else ..." once for each side of the condition.

The "not logged in" test appears to confirm that an anonymous user arriving at the non-existent route "/public_projects" sees a welcome page with a "browse public projects" link. Instead, shouldn't we be testing that the anonymous user can actually load "/projects/public" and see a list of public projects?

Testing the link on the "welcome" page is good too but that should be a separate test. Perhaps this part should be added to anonymous_access_test.rb.

It looks like we test for an "unrestricted public data" link even if the anonymous token is not configured and the visitor is not logged in. It seems to me this should fail. Is this passing for you only because the test case loop doesn't actually turn off anonymous_user_token when anon_config==false? It looks like it only serves to turn it on if anon_config is truthy, so it doesn't actually test the "anon token not configured" cases at all if you have one configured in your application.yml.

Tom, thanks for the comments. I incorporated all those comments. Also, removed the integration tests and made them controller tests. And I was able to test the "Browse public projects" button by adding a few more assertions to the anonymous integration tests. Thanks.

At a2eb98f

In the new controller tests, the first assertion is superfluous since it's a strict subset of the second one, and the second+third would automatically print more useful failure messages if they were written as assert_[not_]includes list, item:

      assert_operator 0, :<, project_names.length
      assert project_names.include?('Unrestricted public data')
      assert !project_names.include?('A Project')

The use of "raw" seems superfluous in app/views/layouts/body.html.erb:

  link_to raw('Browse public projects'), ...

Instead of this comment, you could explicitly set Rails.configuration.anonymous_user_token = false at the top of the test:

  # anonymous config is not enabled by default

The "visit public projects page when disabled" test cases still seem a bit silly. Why not admit the tests are completely different, something like this:

-  [
-    nil,
-    :active,
-  ].each do |user|
-    test "visit public projects page when anon config is not enabled, as user #{user}, and expect no such page" do
-      if user
-        get :public, {}, session_for(user)
-        assert_response 404
-      else
-        get :public
-        assert_response :redirect
-        assert_match /\/users\/welcome/, @response.redirect_url
-      end
-    end
-  end
+  test "visit /projects/public with anon config disabled, logged in" do
+    get :public, {}, session_for(:active)
+    assert_response 404
+  end
+
+  test "visit /projects/public with anon config disabled, not logged in" do
+    get :public
+    assert_response :redirect
+    assert_match /\/users\/welcome/, @response.redirect_url
+  end

Thanks...