Remove registrar id from invoice grouping key

[jicelhay]

The desired outcome from this changes are:

• The invoicing file will group/count invoice rows ignoring the registrarId field. Right now registrarId is a field in InvoiceGroupingKey, which is used to generate invoice file rows. With this change we're not setting registrarId but keeping ProductAccountKey, which will result in billing events that have = product account key (and rest of fields in a InvoiceGroupingKey) but != registrarId to be groupped under the same row in the invoice file.
• The detailed reports saved in Google Drive will still be created separately, one per accreditation. They might have different filenames from before - that's not a problem.

b/410593421

---

This change is 

[CydeWeys]

The PR descriptions mentions Sav explicitly, but this will change the billing behavior for all multi-accreditation registrars (of which we have many), right?

Reviewable status: 0 of 2 files reviewed, all discussions resolved

[CydeWeys]

Also, in the absence of the registrar ID, what are we grouping on? Just the billing ID? So the billing ID would have to be the same across the various accreditations for a registrar for this to group correctly?

Reviewable status: 0 of 2 files reviewed, all discussions resolved

[jicelhay]

Yes, it changes the rows of the invoice CSV file which is a single file: it removes the value of one column and because of that the grouping/end result of the file is different.

If you look at the GenerateInvoiceRows.expand method, it maps strings to InvoiceGroupingKeys and then counts/groups identical occurrences. It wasn't explicitly grouping on registrarId, but on the whole row. Once removed it will still group on InvoiceGroupingKey, so for this case = ProductAccountKey and !=registrarId will now group these rows together.

Reviewable status: 0 of 2 files reviewed, all discussions resolved

[CydeWeys]

Can you amend the commit description to explain details of how grouping is handled after this commit, and omit specific mention of Sav seeing as how this change affects all multi-accreditation registrars?

Reviewable status: 0 of 2 files reviewed, all discussions resolved

[CydeWeys]

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion

---

core/src/main/java/google/registry/beam/billing/BillingEvent.java line 175 at r1 (raw file):

                .toString(),
        billingId(),
        "",

Does null make more sense here vs empty string?

[jicelhay]

Description updated.

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @CydeWeys)

---

core/src/main/java/google/registry/beam/billing/BillingEvent.java line 175 at r1 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

Does null make more sense here vs empty string?

I put "" because the invoicing pipeline sets "" on other fields that are not present (see poNumber) so I assumed it's the way the team prefered to handle non present values. I don't have a strong preference.

I think the most correct solution would be to remove registrar id from the key/string row and remove the header from the csv file, but I don't want to change the code too much.

[CydeWeys]

Reviewable status: 0 of 2 files reviewed, all discussions resolved