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.

Sign up for GitHub

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

Jump to bottom

Object ID validation - Custom Fields #319

Closed
wants to merge 23 commits into from
Closed

Object ID validation - Custom Fields #319

wants to merge 23 commits into from

Conversation

kpbacode
Copy link
Contributor

@kpbacode
Copy link
Contributor Author

Removed gon. Hardcoded URL for to check if tests will pass.

@kpbacode
Copy link
Contributor Author

kpbacode commented Sep 30, 2022 •
edited
Loading

Invoice Items search added.

To do:

@kpbacode
Copy link
Contributor Author

kpbacode commented Oct 3, 2022

will add more tests later today

@kpbacode
Copy link
Contributor Author

kpbacode commented Oct 4, 2022

Test done. Ready for review and merge if all good. thanks

@kpbacode
Copy link
Contributor Author

kpbacode commented Oct 4, 2022

Last commit address fallowing issue:
#320
todo: tests

pierre
pierre reviewed Oct 4, 2022
View reviewed changes
config/locales/en.yml Outdated Show resolved Hide resolved
app/controllers/kaui/custom_fields_controller.rb Outdated
rescue StandardError
ensure
if !test_uuid.blank?
msg = { status: '200', message: I18n.translate('custom_field_uuid_exist_in_invoice_item_db') }
Copy link
Member

Choose a reason for hiding this comment

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

Few thoughts:

  1. I don't think we should display anything if the object (e.g. invoice item) exists, just a warning if it doesn't
  2. If the target object for the custom field doesn't exist, we should prevent the user from being able to save it
  3. Can we send the object type to the back-end? That way, we don't have to try all endpoints (invoice item, bundle, etc.). That might take a while 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, good points. Got it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. Done.
  2. Pending.
  3. Done. Way better.
    There will be some improvements.
    11b43ff

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. Done

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. Was already done but response was from KillBill API without I18N and raw data.

@kpbacode
Copy link
Contributor Author

kpbacode commented Oct 5, 2022

To do:

  1. I18N
  2. Tests

@kpbacode
test update
a7e904f 
todo: tests
@kpbacode
update
9087c17 
Fixed tests
I18N added
When Object ID do not exist and you want to add there is redirection to /custom_fields
@kpbacode
Copy link
Contributor Author

kpbacode commented Oct 6, 2022 •
edited
Loading

Fixed tests
I18N added
When Object ID do not exist and you want to add there is redirection to /custom_fields

To do test and review before re-PR

@kpbacode kpbacode mentioned this pull request Oct 6, 2022
Merged
@kpbacode kpbacode closed this Oct 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pierre pierre pierre left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@kpbacode @pierre