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

Fix async validation in catalog form #5440

Merged
merged 3 commits into from Apr 10, 2019

Conversation

rvsia
Copy link
Contributor

@rvsia rvsia commented Apr 9, 2019

Services > Catalogs > Catalogs > Configuration > New/Edit

Description

  • When editing the validation checks if the name already exists, but it did not check if the name already exists on the same catalog => now, it checks ids too
  • updates data-driven-forms to latest
  • adds options to debouncePromise to resolve all promises (this prevented submitting the form, because the unresolved promises was stuck and the form was still validating)

@miq-bot add_label hammer/no, services, react

@miq-bot add_reviewer @Hyperkid123

* unresolved promises prevented ending of validation, so all promises all resolved
.then((json) => {
if (json.resources.length > 0) {
if (json.resources.length > 0 && json.resources[0].id !== id) {
Copy link
Contributor

Choose a reason for hiding this comment

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

What if the result is not first in the array? I think we should use Array.prototype.find for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There should not be more than one result, even if a user injects * to the name of the catalog.

Copy link
Contributor

Choose a reason for hiding this comment

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

are you 100% sure?

Copy link
Contributor

Choose a reason for hiding this comment

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

With find that code is almost unbreakable with [0] a lot of things can go wrong in future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tested the * option on API and when you not put the query inside 's it would not use * as a regular expression.

However, if there is more than one result, it means there are two catalogs with the same name and the DB is already broken. 🗡️

But when I am thinking about it, a user could put the whole name into 's but that seems as a hacky way to name a catalog. Should I take care about these cases? 😸

Copy link
Contributor

Choose a reason for hiding this comment

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

I would definitely use find. Rest does not bother me much.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok! I will also investigate how to properly escape special characters... e.g. % in the name breaks it! 😢

Copy link
Contributor

Choose a reason for hiding this comment

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

I think there should be some JS escaping tools. Using symbols like % is used for filtering etc in the MIQ API

Copy link
Contributor Author

@rvsia rvsia Apr 9, 2019

Choose a reason for hiding this comment

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

I changed it to

  • it tries to find a catalog with the same name and different ID (no looking on the length of results)
  • escaping only % to %25

Copy link
Contributor

@Hyperkid123 Hyperkid123 left a comment

Choose a reason for hiding this comment

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

Just small nitpick. Otherwise looks good. Its unfortunate we cannot throw away useless promises but thats worse than not being submit the form 😄 .

* name async validation checks if the name already exists but it did not check if the name exists on the same catalog -> now it checks id of the catalog with the same name and compares to its id
@miq-bot
Copy link
Member

miq-bot commented Apr 9, 2019

Checked commits rvsia/manageiq-ui-classic@c113510~...18cf9a7 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
0 files checked, 0 offenses detected
Everything looks fine. 👍

@rvsia rvsia changed the title Fix catalog form Fix async validation in catalog form Apr 9, 2019
Copy link
Contributor

@Hyperkid123 Hyperkid123 left a comment

Choose a reason for hiding this comment

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

Restarted travis(ruby timed out). LGTM

@martinpovolny martinpovolny merged commit c796449 into ManageIQ:master Apr 10, 2019
@martinpovolny martinpovolny added this to the Sprint 109 Ending Apr 15, 2019 milestone Apr 10, 2019
@martinpovolny martinpovolny self-assigned this Apr 10, 2019
@rvsia rvsia deleted the fix-catalog-form branch April 10, 2019 07:49
@martinpovolny martinpovolny added this to Done in React rewrite Sep 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
React rewrite
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants