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

Incorrect behaviour after Product update with invalid public base URL #3028

Merged
merged 1 commit into from Aug 5, 2022

Conversation

lvillen
Copy link
Contributor

@lvillen lvillen commented Jul 28, 2022

Which issue(s) this PR fixes

THREESCALE-8557 Incorrect behaviour after Product update with invalid public base URL

Verification steps

  1. Go to Product Settings page (/apiconfig/services/{service_id}/settings)
  2. Write invalid URL to Staging or Production public base URL text field
  3. Click Update Product button at the bottom of the page

@lvillen lvillen self-assigned this Jul 28, 2022
@lvillen lvillen changed the title [WIP] Fix product update with invalid url [WIP] Incorrect behaviour after Product update with invalid public base URL Jul 28, 2022
@lvillen lvillen requested a review from a team July 28, 2022 16:10
@mayorova
Copy link
Contributor

mayorova commented Jul 28, 2022

Just a weird thing I noticed - the error message and the input label are swapped, compared to master.

master:
Screenshot from 2022-07-28 22-41-19

this branch:
Screenshot from 2022-07-28 22-41-30

Not sure if it is critical, better check with @3scale/qe I guess.

@JaurbanRH
Copy link

JaurbanRH commented Jul 29, 2022

@mayorova it would be best if it will be the same as before otherwise it will break our automatic tests

@lvillen
Copy link
Contributor Author

lvillen commented Aug 3, 2022

Apparently, at previous Formtastic versions the default inline_order was:
https://github.com/formtastic/formtastic/blob/8741d76a95b7adf29d39df41f6d4f015cdd74473/lib/formtastic.rb#L36

But they have changed that into this:
https://github.com/formtastic/formtastic/blob/e34baba470d2fda75bf9748cff8898ee0ed29075/lib/formtastic/inputs/base/wrapping.rb#L12

So, I propose to use the default inline_order to avoid more monkey patching @mayorova @JaurbanRH

@@ -3,4 +3,4 @@
= semantic_form_for @service, url: admin_service_path(@service) do |form|
= render partial: 'api/services/forms/integration_settings', locals: { form: form, service: @service }
= form.actions do
= form.commit_button 'Update Product', button_html: { name: 'update_settings' }
= form.commit_button 'Update Product', button_html: { name: 'update_settings', value: 'true' }
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good but what does this do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It ensures that when the error occurs, it doesn't redirect to the edit template, as you can see here:

render action: params[:update_settings].present? ? :settings : :edit # edit page is only page with free form fields. other forms are less probable to have errors

Copy link
Contributor

Choose a reason for hiding this comment

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

So value: 'true' is just to make #present? return true in case there are no fields? Could you explain a little bit?

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, this is another thing related to the Formtastic upgrade. At the previous version, that commit button was returning true by default, but after the upgrade we have to add the value: 'true' so the user stays at :settings, which was part of the issue, the user being redirected to :edit when sending the form with an invalid URL.

@lvillen lvillen changed the title [WIP] Incorrect behaviour after Product update with invalid public base URL Incorrect behaviour after Product update with invalid public base URL Aug 5, 2022
Copy link
Contributor

@akostadinov akostadinov left a comment

Choose a reason for hiding this comment

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

This brings form to the behavior of master so lets merge it. Investigate later the hiccup with validation when type of proxy is changed but invalid URLs set.

@lvillen lvillen merged this pull request into fips Aug 5, 2022
@lvillen lvillen deleted the fix-product-update-with-invalid-URL branch August 5, 2022 14:38
akostadinov pushed a commit that referenced this pull request Aug 5, 2022
Fix Integration > Settings Incorrect behaviour after Product update with invalid public base URL
akostadinov pushed a commit that referenced this pull request Aug 5, 2022
Fix Integration > Settings Incorrect behaviour after Product update with invalid public base URL
akostadinov pushed a commit that referenced this pull request Aug 8, 2022
Fix Integration > Settings Incorrect behaviour after Product update with invalid public base URL
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants