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.

Sign up for GitHub

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

Jump to bottom

Add external billing id on enterprises #12980

Open
wants to merge 25 commits into
base: master
Choose a base branch
from
Open

Add external billing id on enterprises #12980

wants to merge 25 commits into from
+84 −23

Conversation

pacodelaluna
Copy link

@pacodelaluna pacodelaluna commented Nov 16, 2024 •
edited
Loading

What? Why?

The objective is to improve the links between OFN enterprises and external billing tools, through the admin reports.

What should we test?

  • Check you can add an ID on an enterprise profile
  • This ID can be found on the revenue by hub report
  • This ID is available on v0 API / report section, as well as the enterprise database
  • Super admin can change the ID as many times as they want
  • Super admin can't add spaces to this ID
  • Several enterprises can have the same ID
  • Enterprise owner / managers don't see this field

Release notes

Changelog Category (reviewers may add a label for the release notes):

  • User facing changes
  • API changes (V0, V1, DFC or Webhook)

Add external billing id on enterprises

The title of the pull request will be included in the release notes.

Documentation updates

No. The tip section should explain the objective of the new field with enough details.

@pacodelaluna pacodelaluna marked this pull request as draft November 16, 2024 15:48
@pacodelaluna pacodelaluna removed their assignment Nov 16, 2024
@pacodelaluna pacodelaluna marked this pull request as ready for review November 28, 2024 21:25
@pacodelaluna
Copy link
Author

I think that I have completed the objective of the ticket.

You can add the External Billing Id information on the enterprise:
image

And it is displayed on the Revenues By Hub report:
image

Please tell me if I missed something.

@pacodelaluna pacodelaluna force-pushed the add-external-billing-id-on-enterprises branch 2 times, most recently from ccd284b to 193be72 Compare November 28, 2024 22:55
@pacodelaluna
Copy link
Author

Oops, it seems I have some failing specs, working on it asap.

@RachL
Copy link
Contributor

RachL commented Nov 29, 2024

LGTM! Many thanks @pacodelaluna ❤️

@pacodelaluna
Copy link
Author

pacodelaluna commented Dec 9, 2024 •
edited
Loading

I have tried to update the specs in order to pass the tests, but I am facing issues setting up Cuprite on my local machine.
I am trying to run it on my local Docker env, and I am not able to find a way to run it properly...
Still having this issue:
image

It seems I am not able to force a specific port for Capybara, not sure why as I am updating the configuration to block the port.
image

Maybe I am not looking in the good direction.
Does anybody know if someone else got this error already?

@RachL RachL added pr-staged-fr staging.coopcircuits.fr and removed pr-staged-fr staging.coopcircuits.fr labels Dec 11, 2024
@RachL
Copy link
Contributor

RachL commented Dec 13, 2024

@pacodelaluna FYI the OFN docker image changed recently: #12905

@RachL
Copy link
Contributor

RachL commented Dec 13, 2024 •
edited
Loading

@pacodelaluna sorry me again. Apologies I didn't pay a close attention to the screenshot the first time, and now that i'm playing with it I'm noticing that you've moved fields in the admin section that should remain accessible to enterprise managers/owners:

image

These two need to stay in "primary details tab'.

@RachL RachL added pr-staged-fr staging.coopcircuits.fr and removed pr-staged-fr staging.coopcircuits.fr labels Dec 16, 2024
@dacook
Copy link
Member

dacook commented Dec 17, 2024

Hi @pacodelaluna sorry that you're having issues with Cuprite. I don't have time to help until next year sorry, but you can try posting in Slack #dev channel in case someone else can help.

@RachL RachL added pr-staged-fr staging.coopcircuits.fr and removed pr-staged-fr staging.coopcircuits.fr labels Dec 18, 2024
@filipefurtad0
Copy link
Contributor

Apologies, I see I must have overridden you on the staging-FR server. I hope I did not cause too much trouble 🙏

@RachL
Copy link
Contributor

RachL commented Dec 18, 2024

no don't worry @filipefurtad0 and I see I've forgotten to remove the label 🤦‍♀️ please use it as much as you need on your timezone. FR team is developing a tool to automate invoices to OFN users, and this PR is needed for the tests, so I will stage it again in the next couples of days, but again feel free to use the staging during your time 💪

@RachL RachL removed the pr-staged-fr staging.coopcircuits.fr label Dec 18, 2024
@pacodelaluna pacodelaluna force-pushed the add-external-billing-id-on-enterprises branch 6 times, most recently from 3ddc08c to 3230159 Compare December 26, 2024 19:52
@pacodelaluna pacodelaluna force-pushed the add-external-billing-id-on-enterprises branch from ec546f5 to 3bcca1e Compare January 6, 2025 20:24
@pacodelaluna
Copy link
Author

@RachL It was possible to move the Sells field as @dacook suggested it, without issues regarding the Permalink fields. I also commented on the ticket.
image

@RachL RachL added pr-staged-fr staging.coopcircuits.fr and removed pr-staged-fr staging.coopcircuits.fr labels Jan 6, 2025
@pacodelaluna
Copy link
Author

Maybe we can also move the triggered fields into a new Permalink & UID section, for clarity.
image
It is a 30 sec change, as you wish.

@RachL
Copy link
Contributor

RachL commented Jan 6, 2025

My comment was more about the fact that I find the UX a bit weird: a below field has an impact on another field which is located above it.

But let's keep it that way 👍 I've moved the PR to code review @pacodelaluna many thanks!!!!

@dacook dacook self-requested a review January 8, 2025 00:48
dacook
dacook requested changes Jan 8, 2025
View reviewed changes
Copy link
Member

@dacook dacook left a comment

Choose a reason for hiding this comment

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

Great, this looks like a nice tidy way to solve it in the end!

I have a couple of small suggestions, otherwise it's all good to go.

For the second reviewer: just review the full diff.

db/migrate/20241113185651_add_external_billing_id_on_enterprises.rb
@@ -0,0 +1,5 @@
class AddExternalBillingIdOnEnterprises < ActiveRecord::Migration[7.0]
def change
add_column :enterprises, :external_billing_id, :string, limit: 128
Copy link
Member

Choose a reason for hiding this comment

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

Makes sense, I agree that there shouldn't be more than 128 chars in this field.
Scanning schema.rb, I see that most fields are set to 255 🤷 Probably it doesn't matter.

spec/serializers/api/admin/enterprise_serializer_spec.rb Outdated
Comment on lines 76 to 84

context "when there is a external billing id" do
let(:enterprise) { create(:distributor_enterprise, external_billing_id: 'INV123456') }

it "includes URLs of image versions" do
serializer = Api::Admin::EnterpriseSerializer.new(enterprise)
expect(serializer.as_json[:external_billing_id]).to eq('INV123456')
end
end
Copy link
Member

Choose a reason for hiding this comment

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

Actually I think we don't really need an extra test for this, we could add the expectation to the first test in this file (because we don't need to test "when there is no billing id").
Would it be ok to change that? That would help avoid growing this file too much.

spec/system/admin/reports/revenues_by_hub_spec.rb Show resolved Hide resolved
@pacodelaluna pacodelaluna force-pushed the add-external-billing-id-on-enterprises branch from 7d15401 to 33f7d8d Compare January 8, 2025 22:08
@pacodelaluna
Copy link
Author

@dacook I have just applied your suggestions about the specs, please tell me if it is ok now, thanks for your time.

mkllnk
mkllnk approved these changes Jan 9, 2025
View reviewed changes
Copy link
Member

@mkllnk mkllnk left a comment

Choose a reason for hiding this comment

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

Great, this is looking good now.

dacook
dacook approved these changes Jan 9, 2025
View reviewed changes
Copy link
Member

@dacook dacook left a comment

Choose a reason for hiding this comment

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

Thank you! I've added back your test for the serialiser too.

spec/system/admin/reports/revenues_by_hub_spec.rb Show resolved Hide resolved
spec/serializers/api/admin/enterprise_serializer_spec.rb Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dacook dacook dacook approved these changes

@mkllnk mkllnk mkllnk approved these changes

At least 1 approving review is required to merge this pull request.

Assignees

@pacodelaluna pacodelaluna

Labels
pr-staged-fr staging.coopcircuits.fr
Projects
OFN Delivery board
Status: Test Ready 🧪
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

As a super admin, I should see a field I can use to link enterprise with my invoicing software
5 participants
@pacodelaluna @RachL @dacook @filipefurtad0 @mkllnk