-
Notifications
You must be signed in to change notification settings - Fork 16
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
Ebean refactor portugal #24
Ebean refactor portugal #24
Conversation
return null; | ||
JPAGenericInvoiceEntity invoice = this.queryInvoice(uidBusiness.toString(), number).findOne(); | ||
if (!(invoice instanceof PTGenericInvoiceEntity)) { | ||
return null; |
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.
I think the previous code returned NoResultException and now returns null
Is this what we want?
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.
The previous code queried PTGenericInvoiceEntities, and then checked if the invoice obtained was a PTGenericInvoiceEntity... if not it would throw a RuntimeException. In practice, that should never happen.
Due to the Ebean inheritance limitations, the new code has to query GenericInvoiceEntities (the superclass). So, after querying, the new code needs to check manually if the invoice is of the right class. In practice, it will always be the right class because the only subclass is the PTGenericInvoiceEntity.
I changed the code to throw a RuntimeException in this case, since it's something that should not happen.
if (cnee.getReference().getNumber().compareTo(invoice.getNumber()) == 0) { | ||
return cne; | ||
for (JPAPTCreditNoteEntity creditNote : creditNotes) { | ||
for (PTCreditNoteEntry entry : creditNote.getEntries()) { |
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 pulls all the credit notes from the DB and then finds which is from that invoice.
This is terrible
PS: I know the previous code did the same, but I think it is a mistake not to fix this on ebean version.
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.
I tried to translate this to Ebean query beans, but due to its inheritance limitations, it's even harder to do so than translating to hibernate queries...
This is terrible indeed. It is a good example of how extremely valuable bidirectional relations can be. The root of the problem is that the relation between a CreditNoteEntry and its referenced PTInvoice is unidirectional. If it were bidirectional, one could simply obtain only the CreditNoteEntries referencing a certain PTInvoice by using the invoice object itself; not a single query needed.
Anyway, I believe that it would be time-consuming to transform this code, and at this point in time there are other priorities.
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.
Add a TODO please, in order to address this 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.
The code already had a TODO. I created an issue for this: #25
return null; | ||
public List<PTCreditNoteEntity> getBusinessCreditNotesForSAFTPT(UID businessUid, Date from, Date to) { | ||
List<JPAGenericInvoiceEntity> invoices = | ||
this.queryInvoice().business.uid.eq(businessUid.toString()).date.between(from, to).findList(); |
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 "between" works the same way as the querydsl? Is all inclusive? just the from?
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.
Yes, it works the same. Both are all inclusive.
/*QJPAPTCreditNoteEntity creditNote = QJPAPTCreditNoteEntity.jPAPTCreditNoteEntity; | ||
QJPAPTCreditNoteEntryEntity entry = QJPAPTCreditNoteEntryEntity.jPAPTCreditNoteEntryEntity; | ||
QJPAPTGenericInvoiceEntity invoice = QJPAPTGenericInvoiceEntity.jPAPTGenericInvoiceEntity; | ||
JPAGenericInvoiceEntity invoice = this.queryInvoice().uid.eq(uidInvoice.toString()).findOne(); |
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.
don't you need to be aware of nulls here?
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.
The previous code did not check for nulls because it (incorrectly) tried to obtain a list of invoices from this query, which can actually only return 1 object max. Now, the query only obtains a single object.
Before, finding no results would lead to an empty list being returned. I've changed the new code to do the same; return an empty list if no result is found (the invoice is null).
… which are not used by Ebean. Updated .gitignore
- Renamed project to billy-portugal-ebean - Removed dependencies to hibernate and querydsl - Added dependencies to ebean and query bean generator - Updated dependency to maven-compiler-plugin - Adapted build plugins to ebean
…yManager, which is no longer needed
…ated to Ebean's @transactional later
…TEntity class so that the Ebean query bean generator can generate QAssociationsEntities
…an, that has its own automatic DDL generator.
… class, an exception is thrown (instead of returning null)
…is found, an empty list is returned, instead of a NullPointerException
… to be enchanced.
…tion to all entities, so that they can be overridden in modules that extend billy-core-ebean
… from billy-core-ebean
7f54273
to
e274a48
Compare
…nceDependencyModule which needed hibernate. Removed generic address, contact, and customer objects that cannot be created alone in Ebean.
5c89525
to
91ea678
Compare
…datory(...) to avoid weird linkage error in runtime (ClassCastExceptions)
097e9d5
to
f30e559
Compare
… bidirectional. This avoids ownership limitation problem of Ebean http://ebean-orm.github.io/docs/mapping/jpa/oneToMany
…y bidirectional. This avoids ownership limitation problem of Ebean http://ebean-orm.github.io/docs/mapping/jpa/oneToMany
…relations to aboid problems with Ebean generating random column names
… instead of handling the injector directly
0986139
to
77d656a
Compare
… zero-valued taxes
… to @manytoone. This allows several entries of a credit note to reference the same invoice multiple times.
…ayments is no longer required
ptTaxes.addAll(taxContextResult); | ||
List<JPAPTRegionContextEntity> childContexts = this.getChildContexts(context); | ||
for (JPAPTRegionContextEntity childContext : childContexts) { | ||
ptTaxes.addAll(this.getTaxesForSAFTPT(childContext, validFrom, validTo)); |
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.
Does this new code do the same as the old one? Can you explain what happened here?
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.
The old code was over-engineered and, as a result, had a bug that prevented adding taxes from sub-contexts where a parent context did not have any tax associated. This simpler version solves that bug.
…ented all the tests that need to be refactored later.
Created a new module: billy-portugal-ebean