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

Change ui to display max alert description length #8933

Merged
merged 1 commit into from
Oct 13, 2023

Conversation

kbrock
Copy link
Member

@kbrock kbrock commented Oct 10, 2023

Displaying the database length limit in the ui
(Just trying to get pen tests fixed)

I thought the model's values would be displayed in the UI.
Looks like this is manually entered in 2 places

ManageIQ/manageiq#21921

These limits are arbitrary. We changed a limit of 50 to 100 and then to 255
The UI was not fixed. So testers trying 70 (larger than 50) thought that no limit took place

This fixes the UI so testers can ensure the limits are met

Before

alert_profile_length-antes

After

alert_profile_length-despues

@kbrock kbrock added the bug label Oct 10, 2023
@kbrock kbrock requested a review from a team as a code owner October 10, 2023 23:26
@kbrock
Copy link
Member Author

kbrock commented Oct 11, 2023

It is tricky to see the changes.
Here is a before and after.
Sorry it includes the explorer to show the values

@Fryguy Fryguy self-assigned this Oct 11, 2023
Copy link
Member

@Fryguy Fryguy left a comment

Choose a reason for hiding this comment

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

LGTM but tests are failing

@jeffibm
Copy link
Member

jeffibm commented Oct 12, 2023

Hey @kbrock , running the yarn run jest -u will update the snapshot and will fix the problem...
image

Displaying the database length limit in the ui
(Just trying to get pen tests fixed)

ffae74aa2685bfe21
ManageIQ/manageiq#21921
@kbrock kbrock force-pushed the alert_profile_description_length branch from 3ca1d1e to 82f61ed Compare October 12, 2023 13:39
@kbrock
Copy link
Member Author

kbrock commented Oct 12, 2023

update:

  • rebase
  • updated jest test

@miq-bot
Copy link
Member

miq-bot commented Oct 12, 2023

Checked commit kbrock@82f61ed with ruby 2.6.10, rubocop 1.28.2, haml-lint 0.35.0, and yamllint
0 files checked, 0 offenses detected
Everything looks fine. 🍰

@Fryguy Fryguy merged commit b18bc9b into ManageIQ:master Oct 13, 2023
@kbrock kbrock deleted the alert_profile_description_length branch October 16, 2023 19:39
@Fryguy
Copy link
Member

Fryguy commented Oct 27, 2023

Skipping backport to quinteros, because it is already in the branch.

@Fryguy
Copy link
Member

Fryguy commented Nov 20, 2023

@kbrock A conflict occurred during the backport of this pull request to morphy.

If this pull request is based on another pull request that has not been marked for backport, add the appropriate labels to the other pull request. Otherwise, please create a new pull request direct to the morphy branch in order to resolve this.

Conflict details:

* Unmerged path app/javascript/components/miq-alert-set-form/miq-alert-set-form.schema.js
* Unmerged path app/javascript/spec/miq-alert-set-form/__snapshots__/miq-alert-set-form.spec.js.snap

@Fryguy
Copy link
Member

Fryguy commented Nov 20, 2023

@jeffibm We need some help backporting this change to morphy. The morphy code is from before the react conversion from what I can tell, and app/javascript/components/miq-alert-set-form doesn't exist on morphy.

@kbrock
Copy link
Member Author

kbrock commented Nov 20, 2023

The main goal of this change is to change the messaging on the screen.

@kbrock
Copy link
Member Author

kbrock commented Nov 20, 2023

ok, I vote we do not backport this UI change.
I think this is not an ui issue in morphy.

  • We did backport a change to the model so the back end caped at 255
  • In React, we updated the UI to say the cap was 255 (used to say 50)
  • From what I can tell, the old UI said the cap was 255 characters

refs:

@jeffibm Based upon these links, does this sound right to you?

@Fryguy
Copy link
Member

Fryguy commented Nov 20, 2023

Cool - thanks for digging in @kbrock !

@jeffibm
Copy link
Member

jeffibm commented Nov 21, 2023

ok, I vote we do not backport this UI change. I think this is not an ui issue in morphy.

  • We did backport a change to the model so the back end caped at 255
  • In React, we updated the UI to say the cap was 255 (used to say 50)
  • From what I can tell, the old UI said the cap was 255 characters

refs:

@jeffibm Based upon these links, does this sound right to you?

yes, the old UI had 255 characters as the limit for Description.

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

Successfully merging this pull request may close these issues.

4 participants