cylc review: URL encode links

[kinow]

Close #3121

Close #3119

Use Jinja2's urlencode filter instead of replacing characters individually. Tested with Cylc suites "cylc1" and "%63ylc1". Links working as below.

EDIT: added another close #3119 as it appears to fix both issues 👍

[kinow]

Appears to fix the issue for #3121. Added to next release, but we can push to 7.8.3 if it's better. Tried to fix it as I found the bug and it was included for 7.8.x series 👍

[kinow]

Added @sadielbartholomew as one of the reviewers as she should be aware of what was the issue about. Happy to anyone else step in as second reviewer 👍

[kinow]

But looks like I will have to edit this PR to include a fix for slashes, as Jinja2 developers decided against escaping the slash. Which caused issues in Ansible and other tools. One example was automatic generated passwords with slash that were not correctly escaped 😞

pallets/jinja#515

Will include a fix for the slash in a few minutes.

[hjoliver]

Good for 7.8.2. If @sadielbartholomew can review by early next week (we need to get the release out by May 1, I think.

[kinow]

No problem, otherwise I think it would just go to 7.8.3 or maybe postponed to Cylc 8 (not sure how common are suites with these special characters)

[kinow]

Manual tests passed for me.

@sadielbartholomew added another commit with more changes. It may require further time to test, so feel free to push to after 7.8.2 (i.e. remove next-release milestone, set to 7.8.3 or later I think).

I registered suites with names such as:

• %63ylc1
• cylc1 (same as above if %63 is url-decoded)
• %63ylc2 (i.e. if url-decoded by accident anywhere, there would be no cylc2 and a 404 would raise)
• "''.abc.def240"
• broadcast1
• broad&63ast1

And I used two suite.rc files only (meaning many suites shared the same source). The broadcast suites I used to test the broadcast links were using the suite.rc from Cylc's example suite "etc/examples/task-states/". It is a good suite as it also fails a few times, shows retries, different icons, links, etc.

And the other suite I quickly wrote to have a task with some special characters too, which required adding urlencode when showing links with the task name as well:

# file: suite.rc
[scheduling]
[[dependencies]]
graph = "a+b%c%63"

[runtime]
[[a]]
script = "true"

Hope that helps testing and reproducing the issues.

Cheers
Bruno

[sadielbartholomew]

Good for 7.8.2. If @sadielbartholomew can review by early next week (we need to get the release out by May 1, I think.

Sorry @kinow, I have been very busy with user support, but I'm reviewing this now. Thanks for your patience.

[sadielbartholomew]

• cylc review: Links broken when suites names contain URL encoded names #3121 re-recreated & tested as fixed on this branch;
• cylc review: Server 404 error when trying Display Options for suites contain special characters #3119 ditto (including with / characters);
• I agree with the approach & the code changes are logical.

Well spotted for these bugs! FYI, I have now got clarification on the task & suite names we consider valid (which does include the % character), & I will shortly amend the scope of my PR #3117 to:

• make this explicit in the documentation;
• make validate consistent, to report agreed (in)valid names as (in)valid;
• ensure the set of all valid names function properly, with respect to:
  • suite (runtime) behaviour;
  • Cylc Review.

[kinow]

Thanks Sadie! Happy to close this one if #3117 will supersede it.

[hjoliver]

LGTM

[hjoliver]

Thanks Sadie! Happy to close this one if #3117 will supersede it.

I'm interpreting @sadielbartholomew's approval of this PR literally! Merging...

[kinow]

Oh, I hadn't seen her approval, only the comment (replied around 5AM after the fire alarm sounded in my building until near 6AM 😴 )

[hjoliver]

Damn, that sucks!

[sadielbartholomew]

Thanks Sadie! Happy to close this one if #3117 will supersede it.

Yes, sorry for any lack of clarity, I should have been explicit that the extended #3117 would build on top of the good work completed for this PR. As Hilary rightly guessed, I wanted this merged ASAP 🏁