add barebones internal models for reports #146
Conversation
|
|
||
| @Override | ||
| public Optional<Instant> getLastRun() { | ||
| return Optional.empty(); |
There was a problem hiding this comment.
I think, IIUC, this will take in some sort of callback that hides the ReportJobRepository when fetching the execution state but that would be added with the repo (or perhaps even after).
There was a problem hiding this comment.
Not sure I agree. The two choices are:
-
The internal model object is created with the current last run from the database when it is fetch from the repository using
getJobI(UUID). -
The internal model object does not have this field and it must be fetched separately from the repository.
There are pros/cons to each approach. Regardless, the caller needs to understand the staleness/freshness of the internal model data and act accordingly. @speezepearson is probably in the best position to say which works better since he is writing the primary consumer code for this data.
@BrandonArp thoughts?
There was a problem hiding this comment.
After talking to @speezepearson I realized this functionality is already on JobRepository which coincides with option (2). Unless you have any objections I'll remove this and we can commit to the latter.
There was a problem hiding this comment.
Yes, that's fine. I spoke with Spencer about this yesterday afternoon; just make sure you're on the same page.
| private String _name; | ||
| @NotNull | ||
| @MinSize(value = 1) | ||
| private Set<String> _emails; |
There was a problem hiding this comment.
I added the @Email validator, but from some quick testing it seems like it doesn't think that '+' emails are valid (e.g. [email protected]
There was a problem hiding this comment.
Here's the implementation:
Seem to work:
https://www.regexpal.com/?fam=107035
Of course there may be a bug in the checker; can you provide a test case?
Granted, it does not appear to be RFC compliant:
There was a problem hiding this comment.
So it works, it looks like the problem is that the builder validator does not honor the ConstraintTarget annotation. I could take a stab at adding that support myself but I'm not too familiar with all this reflection and code generation happening in there.
There was a problem hiding this comment.
Is this the relevant issue on commons?
I agree, this is not the time to fork more work. I assume you can work around it for now?
Please add a TODO to to the Metrics Portal code here with a link to issue 23 above.
There was a problem hiding this comment.
I created a new issue here: ArpNetworking/commons#79, I'll link to it in the TODO. This is fine, I can just use a method validator for the time being.
Defines the minimal amount necessary to build a report repository. These models are mostly POJOs but additional methods defining behaviour are expected (particularly in the case of ReportSource).
PDF/HTML => pdf/html Use Immutable* collections in all builders/interfaces JavaDoc style
ConstraintTarget.VALUES isn't supported by OvalBuilder, we'll probably have to roll a custom method validator until that is fixed.
Oval 1.90 introduced a fix to the @Email validator.
cwbriones
force-pushed
the
barebones_internal_model
branch
from
5478d57 to
f24a3be
Compare
Defines the minimal amount necessary to build a report repository. These models are mostly POJOs but additional methods defining behavior are expected (particularly in the case of
ReportSourceand aSinkwrapper aroundRecipientGroup).