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

Banner IT improvements after resolving issues on rocky. #10565

Merged
merged 37 commits into from
Jun 18, 2024
Merged

Conversation

jp-tosca
Copy link
Contributor

@jp-tosca jp-tosca commented May 15, 2024

What this PR does / why we need it:
After debugging this test and the endpoint to resolve the issues on #10312 I made a few changes:

  1. This test was relying heavily on having any of the specified locales in the JSON but with no reason. This dependency was cause due the inability of the API to return the ID of the created banner and the need to search the id by the display message, see this comment. To avoid this, the endpoint api/admin/bannerMessage has been extended so the ID is returned when created, this will allow to delete a specific banner with ID after creation. This is not a breaking change since is not removing anything from the JSON.
  2. The method UtilIT.getBannerMessageIdFromResponse is not necessary anymore, it also was hardcoded a return to 0 if the string was not found but the id could be some other number.

Which issue(s) this PR closes:

Closes #10312

Is there a release notes update needed for this change?:
Yes.

@coveralls
Copy link

coveralls commented May 15, 2024

Coverage Status

coverage: 20.657% (+0.08%) from 20.574%
when pulling a47329f on rocky_IX
into 5bf6b6d on develop.

This comment has been minimized.

This comment has been minimized.

@@ -79,6 +79,7 @@
import jakarta.ws.rs.core.Response.Status;

import org.apache.commons.io.IOUtils;
import org.apache.commons.lang.exception.ExceptionUtils;

Choose a reason for hiding this comment

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

🚫 [reviewdog] <com.puppycrawl.tools.checkstyle.checks.imports.IllegalImportCheck> reported by reviewdog 🐶
Illegal import - org.apache.commons.lang.exception.ExceptionUtils.

This comment has been minimized.

1 similar comment

This comment has been minimized.

This comment has been minimized.

2 similar comments

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

2 similar comments

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

@jp-tosca
Copy link
Contributor Author

With the last changes I updated the endpoint api/admin/bannerMessage this test will still fail if no locale is defined but this time will be at the API level not at the Test level. This endpoint will return a 500 with No banner messages found for this locale. if there is no banner for this locale over an array with empty elements.

This comment has been minimized.

Copy link
Member

@pdurbin pdurbin left a comment

Choose a reason for hiding this comment

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

I took a quick look.

Copy link
Member

@pdurbin pdurbin left a comment

Choose a reason for hiding this comment

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

The doc change looks good. I left comments on the Makefile.

makefile Outdated Show resolved Hide resolved
makefile Outdated Show resolved Hide resolved

This comment has been minimized.

This comment has been minimized.

Copy link
Member

@pdurbin pdurbin left a comment

Choose a reason for hiding this comment

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

I left another comment on the Makefile.

makefile Outdated Show resolved Hide resolved

This comment has been minimized.

@jp-tosca
Copy link
Contributor Author

I had to change the make docs-all and docs-pdf since I didn't noticed these requires sphinx-latexpdf over sphinx. They work fine now.

This comment has been minimized.

@sekmiller sekmiller self-assigned this Jun 11, 2024
Copy link

📦 Pushed preview images as

ghcr.io/gdcc/dataverse:rocky-IX
ghcr.io/gdcc/configbaker:rocky-IX

🚢 See on GHCR. Use by referencing with full name as printed above, mind the registry name.

@sekmiller sekmiller removed their assignment Jun 14, 2024
@stevenwinship stevenwinship self-assigned this Jun 18, 2024
Copy link

📦 Pushed preview images as

ghcr.io/gdcc/dataverse:rocky-IX
ghcr.io/gdcc/configbaker:rocky-IX

🚢 See on GHCR. Use by referencing with full name as printed above, mind the registry name.

@stevenwinship stevenwinship merged commit 853965e into develop Jun 18, 2024
11 of 12 checks passed
@stevenwinship stevenwinship deleted the rocky_IX branch June 18, 2024 19:19
@stevenwinship stevenwinship removed their assignment Jun 18, 2024
pdurbin added a commit to pdurbin/dataverse that referenced this pull request Jul 1, 2024
These changes were in 853965e as part of PR IQSS#10565 but were lost during
resolution of a merge conflict.
@pdurbin pdurbin added this to the 6.3 milestone Jul 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Size: 30 A percentage of a sprint. 21 hours. (formerly size:33)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AdminIT.testBannerMessages failing on Rocky 9
5 participants