-
Notifications
You must be signed in to change notification settings - Fork 6
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
Add public share results feature #20
Merged
michaelhixson
merged 23 commits into
TechEmpower:master
from
smathews-techempower:add-public-share-results-feature
Jan 30, 2020
Merged
Changes from 1 commit
Commits
Show all changes
23 commits
Select commit
Hold shift + click to select a range
2c29db3
Add feature to allow public sharing of results:
smathews-techempower 92b6132
Add share results page:
smathews-techempower 2031a5c
Merge branch 'master' into add-public-share-results-feature
smathews-techempower 96d5204
Fix errors from merge conflicts, add comments
smathews-techempower 4a3f6e8
Add comments and use the fixed testMetadata key from results files
smathews-techempower e16c3ae
Add missing class-level javadoc
smathews-techempower 9f24d97
Use lowerCamelCase for consistency with the other settings
smathews-techempower f212d02
- Revert the local port to 80 rather than 8080
smathews-techempower f97fadc
Imports: use the ordering and spacing rules from Google's Java style …
smathews-techempower 4ce7191
Add final to classes where possible
smathews-techempower 5f15213
Split the ShareResultsHandler into 2 separate handlers for GET/POST,
smathews-techempower 9c94bea
Use Input/Output Streams instead of Readable/Writable Channels
smathews-techempower e283f11
Improve paste bin results sharing:
smathews-techempower 50a98e2
Create dedicated FileUtils, avoid using java.io.File, and use FileSys…
smathews-techempower 7e7ed26
Fix line break in comment at incorrect place
smathews-techempower 1437142
Remove config.yml that was accidentally committed
smathews-techempower 9e6408f
Add tests for new share results features
smathews-techempower aeb48e9
Make new share results test classes final
smathews-techempower e6d1053
Make new classes final, remove unused class
smathews-techempower e4d1c13
Clarify when testMetadata was added to results json
smathews-techempower def434e
Use random uuids for json files during testing to prevent conflicts b…
smathews-techempower 4e3dbfe
Send an email once a day when the share directory is full and someone…
smathews-techempower e9e22ad
Ensure ShareResultsMailerTest the test is not run in parallel and add…
smathews-techempower File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
69 changes: 69 additions & 0 deletions
69
src/main/java/tfb/status/config/ShareResultsMailerConfig.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,69 @@ | ||
package tfb.status.config; | ||
|
||
import com.fasterxml.jackson.annotation.JsonCreator; | ||
import com.fasterxml.jackson.annotation.JsonProperty; | ||
import com.google.errorprone.annotations.Immutable; | ||
import java.time.Duration; | ||
import java.util.Objects; | ||
import javax.inject.Singleton; | ||
import org.checkerframework.checker.nullness.qual.Nullable; | ||
|
||
/** | ||
* The configuration for the service that sends an emails related to sharing. | ||
*/ | ||
@Immutable | ||
@Singleton | ||
public class ShareResultsMailerConfig { | ||
/** | ||
* The minimum number of seconds that must pass after sending one email before | ||
* sending another. This is used to prevent accidental self-spam if there are | ||
* a lot of requests to upload results when the share directory is full. | ||
*/ | ||
public final long minSecondsBetweenDirectoryFullEmails; | ||
|
||
public ShareResultsMailerConfig(long minSecondsBetweenDirectoryFullEmails) { | ||
this.minSecondsBetweenDirectoryFullEmails = | ||
minSecondsBetweenDirectoryFullEmails; | ||
} | ||
|
||
@Override | ||
public boolean equals(@Nullable Object object) { | ||
if (object == this) { | ||
return true; | ||
} else if (!(object instanceof ShareResultsMailerConfig)) { | ||
return false; | ||
} else { | ||
ShareResultsMailerConfig that = (ShareResultsMailerConfig) object; | ||
return this.minSecondsBetweenDirectoryFullEmails == | ||
that.minSecondsBetweenDirectoryFullEmails; | ||
} | ||
} | ||
|
||
@Override | ||
public int hashCode() { | ||
int hash = 1; | ||
hash = 31 * hash + Long.hashCode(minSecondsBetweenDirectoryFullEmails); | ||
return hash; | ||
} | ||
|
||
@JsonCreator | ||
public static ShareResultsMailerConfig create( | ||
@JsonProperty( | ||
value = "minSecondsBetweenDirectoryFullEmails", | ||
required = false) | ||
@Nullable Long minSecondsBetweenDirectoryFullEmails) { | ||
|
||
return new ShareResultsMailerConfig( | ||
/* minSecondsBetweenDirectoryFullEmails= */ | ||
Objects.requireNonNullElse( | ||
minSecondsBetweenDirectoryFullEmails, | ||
DEFAULT_MIN_SECONDS_BETWEEN_DIRECTORY_FULL_EMAILS)); | ||
} | ||
|
||
public static ShareResultsMailerConfig defaultConfig() { | ||
return create(null); | ||
} | ||
|
||
private static final long DEFAULT_MIN_SECONDS_BETWEEN_DIRECTORY_FULL_EMAILS = | ||
Duration.ofDays(1).toSeconds(); | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,97 @@ | ||
package tfb.status.service; | ||
|
||
import com.google.common.annotations.VisibleForTesting; | ||
import com.google.common.collect.ImmutableList; | ||
import com.google.errorprone.annotations.concurrent.GuardedBy; | ||
import java.time.Clock; | ||
import java.time.Instant; | ||
import java.util.Objects; | ||
import javax.inject.Inject; | ||
import javax.inject.Singleton; | ||
import javax.mail.MessagingException; | ||
import org.checkerframework.checker.nullness.qual.Nullable; | ||
import org.slf4j.Logger; | ||
import org.slf4j.LoggerFactory; | ||
import tfb.status.config.ShareResultsMailerConfig; | ||
|
||
@Singleton | ||
public final class ShareResultsMailer { | ||
private final Logger logger = LoggerFactory.getLogger(getClass()); | ||
private final Clock clock; | ||
private final EmailSender emailSender; | ||
private final ShareResultsMailerConfig config; | ||
|
||
@GuardedBy("emailTimeLock") | ||
private volatile @Nullable Instant previousEmailTime; | ||
private final Object emailTimeLock = new Object(); | ||
|
||
@Inject | ||
public ShareResultsMailer(Clock clock, | ||
EmailSender emailSender, | ||
ShareResultsMailerConfig config) { | ||
this.clock = Objects.requireNonNull(clock); | ||
this.emailSender = Objects.requireNonNull(emailSender); | ||
this.config = Objects.requireNonNull(config); | ||
} | ||
|
||
public void onShareDirectoryFull(long capacityBytes, long sizeBytes) { | ||
synchronized (emailTimeLock) { | ||
Instant now = clock.instant(); | ||
Instant previous = this.previousEmailTime; | ||
|
||
if (previous != null) { | ||
Instant nextEmailTime = | ||
previous.plusSeconds(config.minSecondsBetweenDirectoryFullEmails); | ||
|
||
if (now.isBefore(nextEmailTime)) { | ||
logger.warn( | ||
"Suppressing email for full share directory, " | ||
+ "another email was sent for that account too recently, " | ||
+ "previous email time = {}, next possible email time = {}", | ||
previous, | ||
nextEmailTime); | ||
return; | ||
} | ||
} | ||
|
||
this.previousEmailTime = now; | ||
} | ||
|
||
sendShareDirectoryFullEmail(capacityBytes, sizeBytes); | ||
} | ||
|
||
private void sendShareDirectoryFullEmail(long capacityBytes, long sizeBytes) { | ||
try { | ||
emailSender.sendEmail( | ||
/* subject= */ SHARE_DIRECTORY_FULL_SUBJECT, | ||
/* textContent= */ | ||
composeShareDirectoryFullEmail(capacityBytes, sizeBytes), | ||
/* attachments= */ ImmutableList.of()); | ||
} catch (MessagingException e) { | ||
logger.warn("Error sending email for share directory full", e); | ||
} | ||
} | ||
|
||
private String composeShareDirectoryFullEmail(long capacityBytes, | ||
long sizeBytes) { | ||
return "Hello," | ||
+ "\n" | ||
+ "\n" | ||
+ "The share directory used for storing public uploads of " | ||
+ "results files has reached capacity. Please audit the " | ||
+ "directory and delete old uploads or expand the configured " | ||
+ "capacity." | ||
+ "\n" | ||
+ "\n" | ||
+ "Share directory capacity: " + capacityBytes + " bytes" | ||
+ "\n" | ||
+ "Share directory size: " + sizeBytes + " bytes" | ||
+ "\n" | ||
+ "\n" | ||
+ "-a robot"; | ||
} | ||
|
||
@VisibleForTesting | ||
public static final String SHARE_DIRECTORY_FULL_SUBJECT = | ||
"<tfb> <auto> Share directory full"; | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
51 changes: 51 additions & 0 deletions
51
src/test/java/tfb/status/service/ShareResultsMailerTest.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,51 @@ | ||
package tfb.status.service; | ||
|
||
import static org.junit.jupiter.api.Assertions.assertEquals; | ||
|
||
import com.google.common.collect.ImmutableList; | ||
import java.io.IOException; | ||
import java.util.List; | ||
import javax.mail.MessagingException; | ||
import javax.mail.internet.MimeMessage; | ||
import org.junit.jupiter.api.Test; | ||
import org.junit.jupiter.api.extension.ExtendWith; | ||
import tfb.status.testlib.MailServer; | ||
import tfb.status.testlib.TestServicesInjector; | ||
|
||
/** | ||
* Tests for {@link ShareResultsMailer}. | ||
*/ | ||
@ExtendWith(TestServicesInjector.class) | ||
public final class ShareResultsMailerTest { | ||
/** | ||
* Verifies that the mailer sends an email with the expected title when the | ||
* {@link ShareResultsMailer#onShareDirectoryFull(long, long)} method is | ||
* invoked. | ||
*/ | ||
@Test | ||
public void shareResultsMailer_shareDirectoryFullEmail( | ||
MailServer mailServer, | ||
ShareResultsMailer shareResultsMailer) | ||
throws IOException, MessagingException { | ||
|
||
// The mailer is a singleton and may have been called by other tests | ||
// indirectly. We just need to make sure it sent an email as a result of | ||
// this test. | ||
List<MimeMessage> messages =getShareDirectoryFullEmails(mailServer); | ||
int initialMessagesSize = messages.size(); | ||
|
||
shareResultsMailer.onShareDirectoryFull(1234, 5678); | ||
|
||
messages = getShareDirectoryFullEmails(mailServer); | ||
|
||
assertEquals(initialMessagesSize + 1, messages.size()); | ||
} | ||
|
||
private static ImmutableList<MimeMessage> getShareDirectoryFullEmails( | ||
MailServer mailServer) throws IOException, MessagingException { | ||
|
||
return mailServer.getMessages( | ||
m -> m.getSubject().equals( | ||
ShareResultsMailer.SHARE_DIRECTORY_FULL_SUBJECT)); | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works now, but may cause an issue if you make the tests run in parallel. One of the other tests tries to share a results file after filling the share directory to capacity, which also sends this email. I'm curious what the right solution would be. One idea is that since the email contains the capacity and size in the body, I could add code to find a message with the specific numbers passed to
onShareDirectoryFull
above.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's not solve this now, but please annotate the test so that it can't run in parallel and add a
TODO
near the part of the test that's unfriendly to parallelism.