Project

General

Profile

Actions
Copy link

Feature #16647

closed

Responsible person

Added by Daniel Kutyła over 3 years ago. Updated almost 3 years ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Daniel Kutyła
Category:
Workbench2
Target version:
2021-05-26 sprint
Story points:
-

Description

Please show the responsible person value in the "information page". The value should be shown as the name of the person not the UUID

Files

Download all files
Line break between _Responsible_ and _person_.png (171 KB) Line break between _Responsible_ and _person_.png Lucas Di Pentima, 05/21/2021 01:50 PM
Line break between name and copy to clipboard button.png (138 KB) Line break between name and copy to clipboard button.png Lucas Di Pentima, 05/21/2021 01:50 PM

Subtasks 1 (0 open — 1 closed)

Task #17444: Review 16647-Responsible-personClosedDaniel Kutyła05/10/2021Actions
Actions
Copy link
 #1

Updated by Peter Amstutz about 3 years ago

  • Target version set to 2021-03-17 sprint
Actions
Copy link
 #2

Updated by Peter Amstutz about 3 years ago

  • Assigned To set to Daniel Kutyła
Actions
Copy link
 #3

Updated by Peter Amstutz about 3 years ago

  • Target version changed from 2021-03-17 sprint to 2021-03-31 sprint
Actions
Copy link
 #4

Updated by Peter Amstutz about 3 years ago

  • Target version changed from 2021-03-31 sprint to 2021-04-14 sprint
Actions
Copy link
 #5

Updated by Peter Amstutz about 3 years ago

  • Target version changed from 2021-04-14 sprint to 2021-04-28 bughunt sprint
Actions
Copy link
 #6

Updated by Daniel Kutyła about 3 years ago

  • Status changed from New to In Progress
Actions
Copy link
 #7

Updated by Peter Amstutz almost 3 years ago

  • Target version changed from 2021-04-28 bughunt sprint to 2021-05-12 sprint
Actions
Copy link
 #9

Updated by Lucas Di Pentima almost 3 years ago

Some comments:

  • General UI enhancement suggestion: In the current state of affairs, when an uuid has to be rendered with the user's name for example, the first and last names are displayed and their uuid added between parenthesis. The issue I'm seeing (and I think failed to see when first implemented) is that if the user's profile doesn't have any name assigned, only the uuid between parenthesis will be shown. We could just show the uuid without the parenthesis in this case.
  • I'm scanning the updates and not seeing any mention of original_owner or even responsible_person_uuid, and AFAICT the code is displaying the collection's owner uuids as the responsible person, is my understanding correct? This ticket is about displaying the value of the managed property usually (but not always) named responsible_person_uuid as a first and last name. I believe I already sent this, but just in case I didn't, please check this doc page: https://doc.arvados.org/v2.1/admin/collection-managed-properties.html -- you will see that the managed property key could be anything, so I think the correct strategy would be to check the cluster's exported config Collections.ManagedProperties keys and check for anything with Function: original_owner on it, and check which key the current configuration is using.
  • I like your idea of showing the responsible person's name & uuid on the main collection panel along with other data. Not just from the UI point of view but also because doing so on the properties panel could be cumbersome to implement as there's already logic in place to show vocabulary terms. The thing I would correct in this approach is to check if a collection does have the property and only then add the Responsible person row on that panel. Some clusters won't have the feature enabled so it doesn't make sense to add that anyways.
  • I've added the proper cluster config for the cypress tests so you can add some there. It would be interesting to have a collection owned by user A, copied by user B (you can do the copying through API calls to avoid adding test time on that operation) and then check that WB2 correctly show the different uuids & names (owner versus responsible person). I think this config change made a preexisting cypress test fail because it's assuming no property is auto-added - developer-tests-workbench2: #404
  • Right now Cypress tests on Jenkins are failing if you leave master as the arvados_git_hash parameter, you can use b1a2bad as a hash parameter as a workaround until we fix that.
Actions
Copy link
 #10

Updated by Peter Amstutz almost 3 years ago

  • Target version changed from 2021-05-12 sprint to 2021-05-26 sprint
Actions
Copy link
 #12

Updated by Lucas Di Pentima almost 3 years ago

  • File src/common/config.ts: I think Collections.ManagedProperties should be declared as a dictionary of keys being strings with values being dicts with Function or Value members with string values and optional Protected members with boolean values. At least that's how the current docs describe the feature. (https://doc.arvados.org/v2.1/admin/collection-managed-properties.html)
  • There's one cypress test failing because of the config change I made on commit c54ac3e -- you will be able to fix the test by instead of doing a complete comparison of properties, just assert that the one being tested exists no matter what else is set as property.
  • As we talked on chat, the feature shouldn't be hardcoded to display resposible_person_uuid because the cluster could be configured differently
  • On the previous version, the responsible person field was also displayed on the main info panel, but now it seems that it got removed, why is that?
  • My arvbox instance user doesn't have a first and last name set, so the "Responsible person" field on the details panel now appears as "()" -- empty parenthesis. I think it should just show the UUID if the user's first and last names aren't available.
  • Please make tests of all these observations before the next review pass to make the process more dynamic.
Actions
Copy link
 #14

Updated by Lucas Di Pentima almost 3 years ago

The feature seems to work very well now! Some last detail observations:

  • When the responsible person's name isn't available and just the UUID is shown, the layout changes adding a line break between "Responsible" and "person". I think this should be fixable with CSS, please see attached image.
  • The same as above happens with the "Copy to clipboard" button, it should be on the same row as the thing is copying, see 2nd attachment.
  • At file src/views-components/data-explorer/renderers.tsx lines 483-489, I think a super easy optimization would be to break the loop once the managed property key is found, what do you think?
Actions
Copy link
 #16

Updated by Lucas Di Pentima almost 3 years ago

This LGTM, thanks!

Actions
Copy link
 #17

Updated by Daniel Kutyła almost 3 years ago

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

Also available in: Atom PDF