[Spree Upgrade] Delete Accounts and Billing dead feature code until we decide we want to start using it again

[luisramos0]

Please read the discourse thread about this PR: https://community.openfoodnetwork.org/t/ofn-v2-account-invoices/1548

What? Why?

Closed #2684 and #2918.
From spree upgrade phase 2 issue #2918, I realized this feature has not been used in the last 2 years. Last invoices are from 2016.
There's some effort involved in making this work in v2.
I believe this should be external to OFN, not part of the OFN code base.
I recommend we delete this code until we decide to use this feature again, when we decide that this feature should be moved to a separate engine.

This is a significant part of the OFN complexity, 4000 lines of smart code, so deleting this and forcing it out of the code base into an engine is a great step towards making OFN simple.

Alternatives

Releasing v2 with these specs commented out is not an option, I think we either fix it or delete it. This PR can be reverted easily later when/if we decide to make it work (in v2.1 for example).

So, the only alternative to this PR is to fix the account invoices feature in v2.
Releasing OFN v2 with this feature broken and commented specs is not a valid option.

Opinion

From what I read in slack, I think we should delete this and any other feature that is not being used and is not at the top of our priorities.
https://en.wikipedia.org/wiki/Muda_(Japanese_term)
"one of the three types of deviation from optimal allocation of resources"

Release Notes

Change Type: Removed
Business Model configuration, Account Invoices and Trial date control was removed after 3 years of no use. This feature may come back renewed in the near future.
All the preferences and adjustment records (with the invoice values) will be removed from the database when the database is migrated to v2. You should export the account invoices data before you upgrade (DB table spree_orders with distributor_id equals to the accounts distributor ID, the DB table spree_adjustments where source_type equals 'BillablePeriod' and the DB table spree_preferences).

[sauloperez]

😱

[luisramos0]

4447 lines of code...
It's almost ready, the build is looking good, just 2 specs failing in spec/mailers/enterprise_mailer_spec. I dont understand how it can be related... it must be.

[luisramos0]

ah, nice, the build is good now.
so this is ready for review :-O

[luisramos0]

rebased, green build ;-)

[kristinalim]

@luisramos0 I did some grepping for removed helper methods that are not just for a resource, and noticed that free_use? is still used in the code but is no longer defined. It's just in a view template.

In my understanding, we would be keeping the account and billing data in the DB, but we would no longer be able to "use" these features. Is this correct? If yes, some questions:

1. Are we keeping the Spree::Preference records that we no longer use? I think they should be removed and should just be set again if we put back the removed code. If not, we risk complexity when we happen to use the same key with a different data type or purpose in the future.
2. What about DB records for pending or failed DJ jobs? Bringing this up because a project I worked on had many failed jobs for a job type that was no longer defined in the code. I think these should be removed.
3. Some places that used to exclude platform billing Customer records (Customer.of_regular_shops) would now show these Customers. I think this is okay, but have you checked that this would not result in an error? For example, if we presume certain data is present, but it is missing.
4. How will order pages for existing platform billing orders look now? Some adjustments would have source_type of BillablePeriod, but BillablePeriod is no longer be defined.
5. Is there anything users should ensure before they upgrade? For example, that all existing platform billing orders have been processed. I think this should be included in the release notes.

[luisramos0]

Nice, thanks for the review @kristinalim

1. I'll add code to remove these preferences, we can easily add them back if we recover the code
2. I'll add code to remove pending jobs, these are not needed in case we recover the code
3. There are 20 customers in aus and 2 in UK for this. These are some of the enterprises in the system themselves, it will not be harmful to have these listed in customers lists as they will only be seen by super admins or managers of the "accounts enterprise".
4. great point! in AUS, we are talking about 100 adjustments lines from orders from beginning of 2016. I am sure this can be copied to a spreadsheet and deleted. I will add code to do this.
5. this is dead zone @kristinalim 3 years of no usage...

Summary, I'll add code to:

• remove the 11 preferences (from point 1 above)
• remove pending jobs related to the 3 jobs in this PR (from point 2 above)
• remove all adjustments of type BillablePeriod (from point 4 above)

does this sound good @kristinalim ?

[luisramos0]

actually, regarding point 2 above, what are pending jobs? 😊 I looked at table delayed_jobs in both AUS and UK but there's nothing there related to this PR because all jobs just run and are deleted. what do you think?

[luisramos0]

ok, I've added a new commit to delete preferences and adjustments.
I'll wait for your feedback re delayed jobs.

[kristinalim]

actually, regarding point 2 above, what are pending jobs?

@luisramos0 For pending jobs, just the ones that have not been processed yet before the upgrade (e.g. if there is a long queue of jobs). For failed jobs, we've set DJ not to delete failed jobs. It's great if none of these jobs have failed in the past though.

in AUS, we are talking about 100 adjustments lines from orders from beginning of 2016. I am sure this can be copied to a spreadsheet and deleted. I will add code to do this.

Sounds scary, but sounds like it's the best path. Maybe the instances could also be asked to export the order data for this accounts enterprise before the upgrade? Because, for example, callbacks might update the order totals if we destroy the adjustment records.

this is dead zone @kristinalim 3 years of no usage...

OFN is still open source software which is free to be used by all, and some instances we don't know about might be using this feature. Aside from a warning in the release notes that this feature would be removed, actually, I think an additional note that these records would be removed and that they should export order / adjustment data for this accounts enterprise would be sufficient and requires small enough effort. What do you think?

free_use? is still used in the code but is no longer defined.

@luisramos0 This too, by the way. 🙂

[luisramos0]

re delayedjobs, I think we probably want to have a Delayed::Job.destroy_all before the migration? shall I do that instead?

re release notes, good points, I agree, I drafted it in the description of this issue. We need to see what kind of release notes v2 will have!

I am sorry, I have missed your comment about free_use?
Starting with free_use? I did some more serious search and grep and found all the trials code that depends on account invoices. I added one new commit to delete that.

[luisramos0]

this is new, isnt it?

[kristinalim]

this is new, isnt it?

Yes it is! Nice. We now have a complete summary of reviews too:

[kristinalim]

re delayedjobs, I think we probably want to have a Delayed::Job.destroy_all before the migration? shall I do that instead?

This sounds good to me. A separate issue?

[kristinalim]

Looks good to me now, @luisramos0!

@sauloperez I'll request redo of review from you.

[luisramos0]

thanks guys 👍
staged in FR: https://staging.openfoodfrance.org

[RachL]

@luisramos0 other than not seing the menu in configuration, is there something else to test for this particular PR? I've notice that now Invoice settings have an error 500 https://staging.openfoodfrance.org/admin/invoice_settings/edit could this be linked to that PR?

[luisramos0]

I am sorry I completely missed the "what to test" part of the PR.

I think #3551 is relates to this PR.
Invoice settings problem will also be related, I'll have a look.

We need to test different things:

• enterprise registration and changing enterprise type (sells own vs sells any vs sells none)
• users and customers management in the backoffice
• order confirmation emails
• enterprise dashboard

We also have to validate reports, but we will have to do that later when reports are working in v2

But let me have a look first at #3551
I'll let you know when ready to test again.

[luisramos0]

ok, so invoice settings are back (this PR was removing that by mistake).

I confirmed #3551 is not related to this PR :-) I'll explain there what I found.

So, this is ready for testing again 👍

[RachL]

Testing blocked by #3561

Note to myself: testing notes template: https://docs.google.com/document/d/1YkmXg5RYLKmDiTdQrWsYNfHDb-kartNwc84CewDUhNE/edit#

[RachL]

@luisramos0 before FR staging crashed, I couldn't get an order confirmation email. Not sure yet if it was linked to staging or to the PR. Also I couldn't ship an order. I did a last test with Sally's distributor to see if it was linked to my producer.

Need to check this and invoices when staging will be back. The rest seems to look good.
Testing notes: https://docs.google.com/document/d/1YkmXg5RYLKmDiTdQrWsYNfHDb-kartNwc84CewDUhNE/edit#

[RachL]

@luisramos0 order emails are not sent anymore (registering and shipment still good), so this is back to you. I manage to ship an order by placing it on Sally's distributor. I think there might be something missing for my producer, so I need to check that.

[luisramos0]

Hey @RachL delayed job is not working properly in staging FR, that's the reason emails are not going out (I see them in the logs). We need to rebuild staging FR...
I think we will need to do an overall test of emails in v2. Would you minding ignoring emails in this PR?
Thanks!

[luisramos0]

I deployed this v2 branch to katuma staging and the emails are working fine.

So, this is ready for re-testing.

[RachL]

@luisramos0 When I'm editing a product, I cannot update stock info anymore.

But I see this on staging FR as well. So I'm guessing this is a general v2 bug and not linked to this PR. Am I correct?

Also I'm still struggling to ship an order with a new account (same as in staging FR).

I will create new issues for both.

[RachL]

Created #3573 and #3574

[luisramos0]

yeah, thanks. these issues will not be related to this PR.

does that make this PR ready to go?

[RachL]

@luisramos0 yes

[luisramos0]

awesome, thanks!