Bug #14738

[Workbench] Tag editor not loading

Added by Peter Amstutz about 1 month ago. Updated 15 days ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
-
Target version:
Start date:
01/29/2019
Due date:
% Done:

100%

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

Description

On the workbench collection page, the "Tags" tab is not loading, it just says "Loading tags..."


Subtasks

Task #14750: Review 14738-tag-editor-fixResolvedEric Biagiotti

Associated revisions

Revision 6fbd4412
Added by Lucas Di Pentima 21 days ago

Merge branch '14738-tag-editor-fix-es5'
Closes #14738

Arvados-DCO-1.1-Signed-off-by: Lucas Di Pentima <>

History

#1 Updated by Peter Amstutz about 1 month ago

  • Description updated (diff)

#2 Updated by Lucas Di Pentima about 1 month ago

  • Assigned To set to Lucas Di Pentima

#3 Updated by Lucas Di Pentima about 1 month ago

  • Target version set to 2019-01-16 Sprint

#4 Updated by Lucas Di Pentima about 1 month ago

  • Target version changed from 2019-01-16 Sprint to 2019-01-30 Sprint

#5 Updated by Lucas Di Pentima about 1 month ago

Tom Morris reported that the error seems to be about not handling correctly the lack of a vocabulary.json file.

Checking qr1hi and 4xphq I'm also seeing errors like:

[Error] Cross-origin redirection to https://4xphq.arvadosapi.com/arvados/v1/collections?filters=%5B%5B%22uuid%22%2C%22%3D%22%2C%224xphq-4zz18-003mqj38ytfl1om%22%5D%5D&select=%5B%22properties%22%5D denied by Cross-Origin Resource Sharing policy: Origin https://workbench.4xphq.arvadosapi.com is not allowed by Access-Control-Allow-Origin.
[Error] XMLHttpRequest cannot load https://4xphq.arvadosapi.com//arvados/v1/collections?filters=%5B%5B%22uuid%22%2C%22%3D%22%2C%224xphq-4zz18-003mqj38ytfl1om%22%5D%5D&select=%5B%22properties%22%5D due to access control checks.

#6 Updated by Lucas Di Pentima about 1 month ago

It seems that the real issue is about CORS, as I've created a vocabulary.json file on an arvbox instance and the problem persisted.

#7 Updated by Lucas Di Pentima 23 days ago

  • Status changed from New to In Progress

#8 Updated by Lucas Di Pentima 22 days ago

Updates at 1c0566366 - branch 14738-tag-editor-fix
Test run: https://ci.curoverse.com/job/developer-run-tests/1046/

Fixed the way request URL are built on the tag editor to avoid double slashes. Running tests just in case, but this editor isn't explicitly tested.

#9 Updated by Lucas Di Pentima 22 days ago

One way to test if this works is pointing your local workbench to 4xphq or qr1hi and try the Tags tab inside a collection.

#11 Updated by Eric Biagiotti 22 days ago

In reference to 1c0566366:

- Nit picky, but I generally like spaces before and after '+' when concatenating. Not sure what our coding convention for that is.
- Is it safe to assume the variables used on line 195 and 199 are OK to use for a URL? Is there a better way to build URLs in javascript or is this the convention for javascript code in arvados?

Otherwise, LGTM.

#12 Updated by Lucas Di Pentima 22 days ago

Eric Biagiotti wrote:

- Nit picky, but I generally like spaces before and after '+' when concatenating. Not sure what our coding convention for that is.
- Is it safe to assume the variables used on line 195 and 199 are OK to use for a URL? Is there a better way to build URLs in javascript or is this the convention for javascript code in arvados?

I thought we didn't have any coding standards regarding Javascript, but after a quick look at https://dev.arvados.org/projects/arvados/wiki/Coding_Standards, it seems that we should follow what's described in https://github.com/airbnb/javascript.

#13 Updated by Lucas Di Pentima 22 days ago

Update at 608588c42

Used templates to build strings whenever possible, following AirBnB JS style guide.

#14 Updated by Eric Biagiotti 22 days ago

Lucas Di Pentima wrote:

Update at 608588c42

Used templates to build strings whenever possible, following AirBnB JS style guide.

At the risk of scope creep, I would update lines 232 and 31 to be consistent in the entire file.

#15 Updated by Lucas Di Pentima 22 days ago

Updates at cf71576a6

  • Use string templates on those 2 cases, didn't know that ${...} were just any expression, thanks!
  • Replaced "a_string" with 'a_string' following the style guide.

#16 Updated by Lucas Di Pentima 22 days ago

It seems that using string templates are not compatible with our testing infrastructure because it causes SyntaxError failures. (ES5 vs ES6 issue?)

Reverted to 163c8f8750193b791eb62f5a8d73dc44a006b69e and branched out to a new one: 14738-tag-editor-fix-es5, fixing the double quotes. (e17cd76f8)
Test run: https://ci.curoverse.com/job/developer-run-tests/1048/
WB integration tests re-run: https://ci.curoverse.com/job/developer-run-tests-apps-workbench-integration/1079/

#17 Updated by Lucas Di Pentima 21 days ago

  • Target version changed from 2019-01-30 Sprint to 2019-02-13 Sprint

#18 Updated by Lucas Di Pentima 21 days ago

  • Status changed from In Progress to Resolved

#19 Updated by Tom Morris 15 days ago

  • Release set to 21

Also available in: Atom PDF