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

Make Owner addressable via RFC 2822 standard #2411

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

noomorph
Copy link
Contributor

@noomorph noomorph commented Mar 11, 2024

Context

I find it inconvenient to have owner just as a non-actionable label – especially in environments where a person has different names in real life, email, Github and Slack/Discord.

image

Therefore, I'd like to suggest improving the rendering and making it follow Internet Message Format, e.g.:

John Doe <[email protected]>
John Doe <https://github.com/john.doe>

and also be able to consume plain e-mail and URL addresses:

[email protected]
https://github.com/john.doe

In the case when the owner is not a valid URL or e-mail, the rendering stays the same, e.g.:

johndoe

Checklist

@baev baev added the type:new feature Change that add something new for end users label Mar 12, 2024
@noomorph noomorph marked this pull request as draft March 14, 2024 13:10
@noomorph noomorph marked this pull request as ready for review March 27, 2024 08:25
@noomorph noomorph requested a review from baev March 27, 2024 08:25
@noomorph
Copy link
Contributor Author

noomorph commented Mar 27, 2024

@baev, I had time this morning to work on your suggested changes, and I hope the PR looks good enough now.

I verified my change manually end-to-end on localhost:

image

@noomorph
Copy link
Contributor Author

noomorph commented Apr 2, 2024

@baev hi, just making sure you're aware that I have fixed the code according to your review.

@baev
Copy link
Member

baev commented Apr 2, 2024

thx, I'm aware. Review is WIP, will try to finish it this week

Copy link

@A1exKH A1exKH left a comment

Choose a reason for hiding this comment

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

Code LGTM.

Added files:
allure-generator/src/main/java/io/qameta/allure/owner/OwnerAddress.java
allure-generator/src/main/java/io/qameta/allure/owner/OwnerAddressParser.java
allure-generator/src/test/java/io/qameta/allure/owner/OwnerAddressParserTest.java

Modified files:
allure-generator/src/main/java/io/qameta/allure/owner/OwnerPlugin.java
allure-generator/src/main/javascript/plugins/testresult-owner/OwnerView.hbs

void shouldReturnNullForEmptyInput() {
assertNull(OwnerAddressParser.parseAddress(""));
}

Copy link

Choose a reason for hiding this comment

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

Tests look good.
For better test coverage it would be good to add a test for invalid URL and invalid email.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@A1exKH, I added

    implementation("commons-validator:commons-validator:1.7")

to Allure Generator build.kts to make it more resilient when it comes to validating URLs and emails. The remaining regexp for angle brackets parsing has been made a bit stricter too.

Please review the updated tests and implementation again. 🙏

@@ -32,20 +32,28 @@ void shouldReturnNullForEmptyInput() {

@Test
void shouldParseRFC2822FormattedStringWithEmail() {
String input = "John Doe <[email protected]>";
String input = "John Doe < [email protected] >";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Empty spaces in the angle brackets are considered normal thing by the standard.

}

// Prevent performance degradation for plain text
if (!isLikelyAddress(maybeAddress)) {
Copy link
Contributor Author

@noomorph noomorph May 9, 2024

Choose a reason for hiding this comment

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

This thing is important to have for plain text. On 10,000 "John Doe {{counter}}" addresses, it eats them in 20ms, just like without parsing (16-18ms).

Without this optimization, it takes up to 200ms on my machine to parse 10,000 addresses.

I measured the performance between my previous simplistic regexp implementation and the actual implementation with Apache validators, and the impact was negligible — ±20-30ms (sometimes faster, sometimes slower), so I hope this is totally okay for you to accept this PR with industry-proven validators for emails and URLs instead of improvised ones by me.

Copy link

@A1exKH A1exKH left a comment

Choose a reason for hiding this comment

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

Code LGTM.

@baev baev removed their request for review October 22, 2024 09:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
theme:build theme:generator theme:ui type:new feature Change that add something new for end users
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants