Story #6057

[Workbench] Provide an attractive index of public projects

Added by Brett Smith about 4 years ago. Updated about 4 years ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Radhika Chippada
Category:
Workbench
Target version:
Start date:
05/25/2015
Due date:
% Done:

100%

Estimated time:
(Total: 0.00 h)
Story points:
2.0

Description

The index should list basic information about each project, so a user can browse the page and determine whether or not they might be interested in the pipeline or data set. For each project, it should show:

  • The project name (which links to the individual project page).
  • Some amount of project description. There should be a limit to this display, but it should also look good. One or two paragraphs seems like a fine starting point.
  • Ideally, there would be a logo or graphic too. We might be able to get this for free by having users include it in the project description, and then making sure to render it. We can punt on this if it's too hard.

Subtasks

Task #6115: Review branch: 6057-public-projects-pageResolvedTom Clegg


Related issues

Precedes (1 day) Arvados - Bug #6173: [Workbench] Make public data crawlableNew05/27/201505/27/2015

Associated revisions

Revision 32038d56
Added by Radhika Chippada about 4 years ago

closes #6057
Merge branch '6057-public-projects-page'

Revision 9bd0cf5b (diff)
Added by Tom Clegg about 4 years ago

Use default word wrap (no mid-word line breaks). refs #6057

History

#1 Updated by Brett Smith about 4 years ago

  • Category set to Workbench

#2 Updated by Brett Smith about 4 years ago

  • Description updated (diff)

#3 Updated by Tom Clegg about 4 years ago

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.
  • "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.
TBD:
  • 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.

#4 Updated by Tom Clegg about 4 years ago

  • Story points set to 2.5

#5 Updated by Brett Smith about 4 years ago

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.

#6 Updated by Tom Clegg about 4 years ago

  • Story points changed from 2.5 to 2.0

Brett Smith wrote:

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.

Yes, that sounds better: render the whole thing and use jQuery to pare it down on the client side. Perhaps something along these lines to invoke the paring-down without too much FOUC:

<div class="abbreviate-description" style="display:none">
  <h1>superfluous heading</h1>
  <p>hopefully the important bit</p>
  <p>a long-winded part</p>
</div>

var $a = $('.abbreviate-description');
$a.show();
$a.children().hide();
$a.children('p:first').show();
  • 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.

Let's hope to squeeze a "Browse public projects..." link into the Projects dropdown without too much awkward.

It seems like /users/welcome (in stock + theme) should have a link too, for users who aren't logged in.

(I'd like to avoid "wait, I really have to hit my "back" button N times, or go back to the email / site where I first learned about this, to find the magic link to these "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.

SGTM. How about: {workbench}/projects/public → use the anonymous token to get the list of projects, regardless of whether user is logged in.

Should we consider any SEO basics right away, or just see what we get for now, and save tweaking for another story/day? (I'm assuming the latter.)

#7 Updated by Radhika Chippada about 4 years ago

  • Assigned To set to Radhika Chippada

#8 Updated by Tom Clegg about 4 years ago

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.

#9 Updated by Tom Clegg about 4 years ago

Something like this should get public projects:

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

#10 Updated by Radhika Chippada about 4 years ago

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).

#11 Updated by Radhika Chippada about 4 years ago

  • Status changed from New to In Progress

#12 Updated by Tom Clegg about 4 years ago

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.

#13 Updated by Radhika Chippada about 4 years ago

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.

#14 Updated by Tom Clegg about 4 years ago

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...

#15 Updated by Radhika Chippada about 4 years ago

  • Status changed from In Progress to Resolved
  • % Done changed from 0 to 100

Applied in changeset arvados|commit:32038d56fbfe5b635b1c53f247aea9abcca2285c.

Also available in: Atom PDF