Improve title validation on new project creation #1115

lorenzo-cavazzi · 2020-11-05T17:45:05Z

The "New Project" page already contains a number of checks to prevent the user from clicking on "Create Project" when the provided data are wrong. Both the title and the project slug were not tested deeply enough and users were getting confusing errors. We are now closer to the GitLab slugify function, and we test unicode chars in the title. Sadly, we still don't allow emoji... yet 😄

This PR solves the issue on the UI side, and it should be deployed together with SwissDataScienceCenter/renku-python#1691 that replicates similar functionalities in the renku-core backend APIs.

Preview: https://lorenzotest.dev.renku.ch/projects/new
How to test: create projects with creative names -- try to insert invalid chars like / or $ as well as other Unicode letters like ê or æ

fix #1107

ciyer

This is looking good, and the code is great. I have a few requests for changes.

More generally, would it be difficult to just strip accents rather than convert those letters into hyphens? E.g., turn é into e rather than -?

ciyer · 2020-11-06T11:40:26Z

client/src/project/new/ProjectNew.state.js

    else if (projects && projectsPaths.includes(`${input.namespace}/${slugFromTitle(input.title, true)}`))
-      errors["title"] = "Title already in use in current namespace.";
+      errors["title"] = `Similar title (${slugFromTitle(input.title, true)}) ${slugExplanation}`;


Change to

Title produces a project identifier (${slugFromTitle(input.title, true)}) that is already taken in the selected namespace. Please select a different title or namespace.

This message is shown when I just enter the slug as a title, in which case no characters were changed, so I don't think we need to mention the transformation.

We substitute all the non [a-zA-Z] letters with - and we strip the extra -. This leads to possibly confusing cases.
E.G: If you have namespace/test, there are a number of title combinations that will produce the same slug, like testü, test-ä, testé, -test-, ...
I will change the message with what you suggested.

ciyer · 2020-11-06T11:43:11Z

client/src/project/new/ProjectNew.state.js

@@ -450,12 +450,17 @@ class NewProjectCoordinator {
      warnings["template"] = "Must get the templates first.";

    // check errors: require user intervention. Skip if there is a warning
+    const slugExplanation = "already in use in current namespace. Non ascii chars were converted or stripped out.";


Change to already taken in the selected namespace.

This message is shown when I just enter the slug as a title, in which case no characters were changed, so I don't think we need to mention that part.

ciyer · 2020-11-06T11:46:22Z

client/src/project/new/ProjectNew.state.js

+    else if (!verifyTitleValidity(input.title))
+      errors["title"] = "Title can contain only letters, digits, '_', '.', '-' or spaces.";
+    else if (input.title && !slugFromTitle(input.title, true))
+      errors["title"] = "Title must contain at least a standard ascii char or a number.";


Title must contain at least one letter (without any accents) or a number.

client/src/utils/HelperFunctions.js

ciyer · 2020-11-06T11:50:30Z

client/src/utils/Utils.test.js


-  it("function slugFromTitle", () => {
+describe("title related functions", () => {


These tests are great, very helpful to understand the behavior.

ciyer · 2020-11-06T11:50:42Z

client/src/utils/Utils.test.js

+  it("function verifyTitleValidity - valid strings", () => {
+    expect(verifyTitleValidity("João-Mario")).toBeTruthy();
+    expect(verifyTitleValidity("здрасти_.и")).toBeTruthy();
+    expect(verifyTitleValidity("")).toBeTruthy();


Why is this one a valid title?

Good point, the name verifyTitleValidity is probably slightly misleading...
It actually checks only that valid chars are used, while other cases are tested separately in the coordinator component to give better feedback to the user (i.e. first letter wrong, title empty, ...).
Should I change the function name to verifyTitleCharacters?

Good idea -- verifyTitleCharacters would make it clearer.

lorenzo-cavazzi · 2020-11-06T13:35:51Z

I also forgot to check for the first letter to be a number or a character. Added now.

More generally, would it be difficult to just strip accents rather than convert those letters into hyphens? E.g., turn é into e rather than -?

It's not particularly difficult, and we could borrow from this convenient function, but we tried to stick to the same rules GitLab is applying on its new project page.
The backend has to implement the very same logic (we don't pass the project part of the namespace slug as a separate argument to the backend) and adopting GitLab standard was preferred over deciding for our own flavor -- even if what you suggested was the first solution we considered 😄

Preview updated in https://lorenzotest.dev.renku.ch/projects/new

ciyer

I think the behavior is good with the improvements. It is very minor, but while we are here, we might as well make the improvements -- could you rename the function to verifyTitleCharacters?

Regarding the conversion of accents. I don't think the GitLab behavior is the best guide, because in GitLab the slug is user editable. I think our solution is better and easier to use, where the slug is just generated from the title, but it need the conversion from title to slug to be good.

For example, if I wanted to create a project with the title �Caffè, it would have the slug caff, which seems weird. I think the slug caffe would be better. In GitLab, I could fix the slug. In RenkuLab, I would need to create the project as Caffe and then rename it to Caffè. This seems needlessly complicated.

But, it's a minor thing. I think if you agree it is worth fixing, we should do it. If not, I will approve after the function rename.

ciyer · 2020-11-06T14:28:39Z

client/src/utils/Utils.test.js

+  it("function verifyTitleValidity - valid strings", () => {
+    expect(verifyTitleValidity("João-Mario")).toBeTruthy();
+    expect(verifyTitleValidity("здрасти_.и")).toBeTruthy();
+    expect(verifyTitleValidity("")).toBeTruthy();


Good idea -- verifyTitleCharacters would make it clearer.

lorenzo-cavazzi · 2020-11-06T16:31:01Z

Well, your argumentation makes sense.
Plus, we have to deviate anyway from the GitLab conversion pattern because it seems to suggest invalid patterns, leading to invalid references format for images in the GitLab registry (example 1, example 2).

The changes are:

Any valid symbol is converted to -
The slugify function tries to convert Unicode accented chars to ascii with the specified rules.

I'll update the preview as soon as the backend counterpart is ready.

lorenzo-cavazzi · 2020-11-09T15:06:58Z

As discussed offline, the final solution won't include the accented letters conversion.
The function is there, I just invoke slugFromTitle without the flag for the conversion.

Preview updated: https://lorenzotest.dev.renku.ch/projects/new

P.S: I'll create a follow-up issue for making the fork modal UX similar to the new project creation. At the moment, the feedback to the user is not real-time (the error comes after trying to fork the project) and the wording for errors is slightly different.

ciyer

Nice fix. Please remember to make sure the issue and PR numbers are in the commit message.

ciyer · 2020-11-09T11:34:07Z

client/src/utils/Utils.test.js

+
+describe("title related functions", () => {
+  // convertUnicodeToAscii
+  it("function convertUnicodeToAscii - valid strings", () => {


lorenzo-cavazzi added 2 commits November 5, 2020 16:31

feat: update slugify function to mirror gitlab code

d2f7656

fix #1107

feat: verify title validity

4944fdf

lorenzo-cavazzi added this to the sprint-2020-10-22 milestone Nov 5, 2020

lorenzo-cavazzi requested a review from a team as a code owner November 5, 2020 17:45

style: lint

f062458

ciyer requested changes Nov 6, 2020

View reviewed changes

refactor: check for first letter and implement review suggestions

c04f8cc

ciyer requested changes Nov 6, 2020

View reviewed changes

lorenzo-cavazzi and others added 4 commits November 6, 2020 16:35

refactor: rename function to improve clarity

91bdbc8

improvement: convert accented chars to ascii and symbols to hyphen

3fdb723

style: lint

d269e32

Merge branch 'master' into 1107-title-validation

d7336de

fix: remove accented chars conversion

cec5bdf

ciyer approved these changes Nov 9, 2020

View reviewed changes

lorenzo-cavazzi mentioned this pull request Nov 9, 2020

Adapt the fork UI/UX to be similar to new project creation #1118

Closed

4 tasks

lorenzo-cavazzi merged commit 51e5ea4 into master Nov 9, 2020

lorenzo-cavazzi deleted the 1107-title-validation branch November 9, 2020 16:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve title validation on new project creation #1115

Improve title validation on new project creation #1115

lorenzo-cavazzi commented Nov 5, 2020 •

edited

Loading

ciyer left a comment

ciyer Nov 6, 2020

lorenzo-cavazzi Nov 6, 2020

ciyer Nov 6, 2020

ciyer Nov 6, 2020

ciyer Nov 6, 2020

ciyer Nov 6, 2020

lorenzo-cavazzi Nov 6, 2020

ciyer Nov 6, 2020

lorenzo-cavazzi commented Nov 6, 2020

ciyer left a comment

ciyer Nov 6, 2020

lorenzo-cavazzi commented Nov 6, 2020

lorenzo-cavazzi commented Nov 9, 2020

ciyer left a comment

ciyer Nov 9, 2020


		it("function slugFromTitle", () => {
		describe("title related functions", () => {

Improve title validation on new project creation #1115

Improve title validation on new project creation #1115

Conversation

lorenzo-cavazzi commented Nov 5, 2020 • edited Loading

ciyer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lorenzo-cavazzi commented Nov 6, 2020

ciyer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lorenzo-cavazzi commented Nov 6, 2020

lorenzo-cavazzi commented Nov 9, 2020

ciyer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lorenzo-cavazzi commented Nov 5, 2020 •

edited

Loading