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

Validation issues #176

Closed
mbabker opened this issue Nov 10, 2013 · 28 comments
Closed

Validation issues #176

mbabker opened this issue Nov 10, 2013 · 28 comments
Assignees
Milestone

Comments

@mbabker
Copy link
Contributor

mbabker commented Nov 10, 2013

Some things need to get cleaned up with the validation rules IMO. Pull requests aren't merging into the app anymore, these being the common reasons:

  • The title min length is 15 chars.
  • The title max length is 50 chars.
  • A build is required.

Title should match exactly what GitHub allows via the site here in terms of lengths. Build can be handled by defaulting to the destination branch in the hooks.

@b2z
Copy link
Member

b2z commented Nov 10, 2013

Agree about title. Anyone knows allowed values? :)

Build can be handled by defaulting to the destination branch in the hooks.

For example?

@mbabker
Copy link
Contributor Author

mbabker commented Nov 10, 2013

At https://github.com/joomla/jissues/blob/framework/src/App/Tracker/Controller/Hooks/ReceivePullsHook.php#L118 add in $data['build'] = $this->data->base->ref; to that array.

@b2z
Copy link
Member

b2z commented Nov 10, 2013

A little bit confusing to have branch name as a build, but if we do not have any other option then it's ok.

@mbabker
Copy link
Contributor Author

mbabker commented Nov 10, 2013

On Joomlacode, I often put in 'master' for the build to indicate the current master. So it would either be '2.5.x', 'staging', or 'master' when coming through GitHub.

@b2z
Copy link
Member

b2z commented Nov 10, 2013

Did not know that. Then the only problem now how to know min-max length for the title? I googled around and did not found anything...

@elkuku
Copy link
Contributor

elkuku commented Nov 10, 2013

how to know min-max length for the title?

We might abuse a bit and just try out...

jtester/tests#26 - Title length 1
jtester/tests#27 - Title length 100
jtester/tests#28 - Title length 200

I wouldn't go any further here but I don't think there is any "visible" limit on their side...

@b2z
Copy link
Member

b2z commented Nov 10, 2013

Just tested the title submitting #177 - minimal title length allowed is 1 char and the maximum is 244 chars :)

@elkuku
Copy link
Contributor

elkuku commented Nov 10, 2013

255 👅

@b2z
Copy link
Member

b2z commented Nov 10, 2013

Well :) One space was missed by me 👅

@b2z
Copy link
Member

b2z commented Nov 11, 2013

@mbabker could you check this? I've just pushed fb2e77c that should fix it.

@mbabker
Copy link
Contributor Author

mbabker commented Nov 13, 2013

I've deployed the latest changes to the server, we'll see what happens now.

@mbabker
Copy link
Contributor Author

mbabker commented Nov 13, 2013

On joomla/joomla-cms#2508 still got a Some characters are not allowed in the title. message.

@b2z
Copy link
Member

b2z commented Nov 13, 2013

Oh my :) It's because of the exclamation mark. We should add it too of course.

@b2z
Copy link
Member

b2z commented Nov 13, 2013

This 4a1e568 should fix it. I hope 😺

@mbabker
Copy link
Contributor Author

mbabker commented Nov 13, 2013

Cool - I'll deploy it tonight

@elkuku
Copy link
Contributor

elkuku commented Nov 14, 2013

Sounds silly, but the next char that comes to my mind is a ? ....

@mbabker
Copy link
Contributor Author

mbabker commented Nov 14, 2013

I wish there was somewhere that their validator was posted.

FYI latest code is deployed.

@b2z
Copy link
Member

b2z commented Nov 14, 2013

Everything is ok?

@elkuku
Copy link
Contributor

elkuku commented Nov 14, 2013

#Merge #204 😄

@b2z
Copy link
Member

b2z commented Nov 14, 2013

Sounds like we need to make a test issue trying to pass all the keyboard into the title 😆

@elkuku
Copy link
Contributor

elkuku commented Nov 14, 2013

I was talking about the project with the folks of the german http://www.joomla-bugs.de/ throwing around crazy ideas about a multi-lingual tracker.... so, those Ä, ä , Ö, ö... and of course the ß --- not talking about those chinese or even russian characters --- should be included (see: jtester/tests#30)

@b2z
Copy link
Member

b2z commented Nov 14, 2013

Hmm, currently our regex in the table class supports Unicode chars:
'/^[\w\pN\pL\pM\-.,()\[\]\'"\+_@&$#%!: ]+$/u'

But JS is not:
/^[a-z0-9\-.,()\[\]'"+_@&$#%!:\s]+$/i

I am not a prof in regex 😊 But on SO found this question. As far as I understand this could work:
[^\u0000-\u0080]

If not we will need some external library to achieve that.

@elkuku
Copy link
Contributor

elkuku commented Nov 14, 2013

Which brings up the interesting question if it might be possible to have one rule "provider" for both client and server side validation.

@b2z
Copy link
Member

b2z commented Nov 14, 2013

Well I just started #210 and it allowed all the chars... Seems we do not need to filter the title at all?

@elkuku
Copy link
Contributor

elkuku commented Nov 14, 2013

I was about to say that :-)


Sent from my Android =;)
On Nov 14, 2013 2:58 PM, "Dmitry Rekun" notifications@github.com wrote:

Well I just started #210 https://github.com/joomla/jissues/issues/210and it allowed all the chars... Seems we do not need to filter title at all?


Reply to this email directly or view it on GitHubhttps://github.com//issues/176#issuecomment-28517193
.

@b2z
Copy link
Member

b2z commented Nov 14, 2013

Hehe :) Will post a fix tomorrow 🎉

@b2z
Copy link
Member

b2z commented Nov 15, 2013

Should be fixed. If not please leave a note here 😆

@mbabker
Copy link
Contributor Author

mbabker commented Nov 16, 2013

I finally got around to deploying the latest up, so we'll see how it all goes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants