Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle special characters in content type group (EZP-30422) #31

Merged

Conversation

bdunogier
Copy link
Member

@bdunogier bdunogier commented Apr 9, 2019

https://jira.ez.no/browse/EZP-30422

Changes the normalization of content types groups identifiers to transform all non alphanumeric characters to _. It is required on content types groups identifiers as they are actually human readable names.

TODO

  • Decide if we want to handle other types of special characters (such as accentuated, or even asian). Should we transliterate those ? If yes, using what ?

@bdunogier bdunogier requested a review from webhdx April 9, 2019 09:59
Copy link
Member

@mnocon mnocon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested with characters like -_!$%^&*()?/~§£=-`, looks ok to me.

Two things:

  1. have you made any decision about the TODO?
  2. If I create two groups: "Marek Nocoń" and "Marek-Nocoń" they are merged into one that has Content Types of both of them (for example: Marek Nocoń has Content Type folder111 and Marek-Nocoń has Content Type folder2222). There is only one group available (example query:

{
  marekNoco_ {
    _info {
      identifier
    }
    folder111s {
      edges {
        node {
          _name
        }
      }
    }
    folder2222s {
      edges {
        node {
          _name
        }
      }
    }
  }
}

which returns

{
  "data": {
    "marekNoco_": {
      "_info": {
        "identifier": "Marek Nocoń"
      },
      "folder111s": {
        "edges": []
      },
      "folder2222s": {
        "edges": [
          {
            "node": {
              "_name": "2"
            }
          }
        ]
      }
    }
  }
}

As I'm not sure if we can avoid this (and whether this could become an issue) I'm just pointing it out, to double check that we're ok with this behaviour.

@bdunogier
Copy link
Member Author

have you made any decision about the TODO?

No, not yet. In any case, the fixed code is an improvement over the previous one, we don't have to decide right now I guess.

If I create two groups: "Marek Nocoń" and "Marek-Nocoń" they are merged into one that has Content Types of both of them

That's what I call QA ! :)
The result is surprising, and I gotta admit, surprisingly good. I'd have expected one of them to be missing...

I'm not sure how we want to address that, and I probably wouldn't block this PR for that. We may have to decide at some point. The way I see it, there'll always be edge cases with the normalization we do, and the only reliable solution will in the end be to allow to customize names.

@lserwatka
Copy link
Member

Let’s call it stage 1.

@lserwatka lserwatka merged commit 480dd89 into master Apr 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants