The redirect url exposes the Odoo ERP server to attacks from outside.
A solution could be to split the code into two parts:

1. a module on the erp server for the VAT declaration
2. a module running on a DMZ server that terminates the HMRC connections

Immediately write submission log entry on button press at earliest technical opportunity. Preferably immediately after guard clauses.

Set hmrc_request_state {'not_sent', 'sent', 'replied_success', 'replied_failure', 'timed_out'} on response from HMRC (timed-out iff we can detect that).

Set 'complete' flag at very end.

Remember to commit() at those key points.

(Pdb) l
 52  	        if response.ok:
 53  	            return {'url': url, 'type': 'ir.actions.act_url', 'target': 'self', 'res_id': record.id}
 54  	        else:
 55  	            response_token = json.loads(response.text)
 56  	            import pdb ; pdb.set_trace()
 57  ->	            error_message = self.env['mtd.display_message'].construct_error_message_to_display(
 58  	                url=url,
 59  	                code=response.status_code,
 60  	                response_token=response_token
 61  	            )
 62  	            record.response_from_hmrc = error_message
(Pdb) response.text
u'{"error":"invalid_request","error_description":"redirect_uri is invalid"}'

Found in 8.0, possibly in all

This will localise rebase/cherry-pick/merge conflicts caused by different versions to only those places where version-specific stuff has actually changed. Most of the code here is not specific to Odoo version, and so should be kept in sync.

from . import whatever_introduces_the_original_model
def MtdOriginalModel(models.Model):
    _inherit='mtd.original.model'
    # implement the methods that are version-specific here.
    # e.g. how to compute the VAT results, and how to build the action to view the VAT breakdown

Though the user can not delete any endpoints but if there were changes made in the code or in the database then the current sequence will be broken.Therefore we need to add some sort of sequence field and then order i by that field.

In certain state from #12, if HMRC bit is complete but we haven't reconciled, we should be able to continue to do that.

In that state, show a button for it to reconcile them.

Depends on #12

How to reproduce

• Restore a copy of the database where this singleton error would occur
  • We don't know cause yet so can't prescribe how to set this up, but we do have a database that has this issue, but don't want to identify it in a public place such as this.
  • Internally, see T15292 for more context info on which database this refers to.
• Install account_mtd_vat
• Add group MTD/Administrator and MTD/User to admin account
• Refresh browser window
• Click Accounting -> Making Tax Digital -> MTD Set-up -> Accounting Set-up -> Create

What should happen

Should not have an error, so we can fill in fields.

What happens

In the GUI we see Odoo Warning dialogue with body Expected singleton: account.account.type(25, 37)

In the logs, we see this:

openerp.http: Exception during JSON request handling.
Traceback (most recent call last):
  File "/usr/lib/python2.7/dist-packages/openerp/http.py", line 546, in _handle_exception
    return super(JsonRequest, self)._handle_exception(exception)
  File "/usr/lib/python2.7/dist-packages/openerp/http.py", line 583, in dispatch
    result = self._call_function(**self.params)
  File "/usr/lib/python2.7/dist-packages/openerp/http.py", line 319, in _call_function
    return checked_call(self.db, *args, **kwargs)
  File "/usr/lib/python2.7/dist-packages/openerp/service/model.py", line 118, in wrapper
    return f(dbname, *args, **kwargs)
  File "/usr/lib/python2.7/dist-packages/openerp/http.py", line 316, in checked_call
    return self.endpoint(*a, **kw)
  File "/usr/lib/python2.7/dist-packages/openerp/http.py", line 812, in __call__
    return self.method(*args, **kw)
  File "/usr/lib/python2.7/dist-packages/openerp/http.py", line 412, in response_wrap
    response = f(*args, **kw)
  File "/usr/lib/python2.7/dist-packages/openerp/addons/web/controllers/main.py", line 944, in call_kw
    return self._call_kw(model, method, args, kwargs)
  File "/usr/lib/python2.7/dist-packages/openerp/addons/web/controllers/main.py", line 936, in _call_kw
    return getattr(request.registry.get(model), method)(request.cr, request.uid, *args, **kwargs)
  File "/usr/lib/python2.7/dist-packages/openerp/api.py", line 268, in wrapper
    return old_api(self, *args, **kwargs)
  File "/usr/lib/python2.7/dist-packages/openerp/api.py", line 399, in old_api
    result = method(recs, *args, **kwargs)
  File "/usr/lib/python2.7/dist-packages/openerp/models.py", line 5998, in onchange
    newval = record[name]
  File "/usr/lib/python2.7/dist-packages/openerp/models.py", line 5654, in __getitem__
    return self._fields[key].__get__(self, type(self))
  File "/usr/lib/python2.7/dist-packages/openerp/fields.py", line 838, in __get__
    self.determine_draft_value(record)
  File "/usr/lib/python2.7/dist-packages/openerp/fields.py", line 946, in determine_draft_value
    self._compute_value(record)
  File "/usr/lib/python2.7/dist-packages/openerp/fields.py", line 885, in _compute_value
    self.compute(records)
  File "/mnt/extra-addons-bundles/OpusVL_mtd/account_mtd_vat/models/hmrc_posting_configuration.py", line 39, in _compute_company_id
    ('name', '=', 'Output Tax')])
  File "/usr/lib/python2.7/dist-packages/openerp/fields.py", line 851, in __set__
    value = self.convert_to_cache(value, record)
  File "/usr/lib/python2.7/dist-packages/openerp/fields.py", line 1051, in convert_to_cache
    return int(value or 0)
  File "/usr/lib/python2.7/dist-packages/openerp/models.py", line 5624, in __int__
    return self.id
  File "/usr/lib/python2.7/dist-packages/openerp/fields.py", line 1909, in __get__
    return record.ensure_one()._ids[0]
  File "/usr/lib/python2.7/dist-packages/openerp/models.py", line 5320, in ensure_one
    raise except_orm("ValueError", "Expected singleton: %s" % self)
except_orm: ('ValueError', 'Expected singleton: account.account.type(25, 37)')

I've been putting in tests as I've been bugfixing, but they're only very simple and very localised unit tests for newly-written code.

We should at least do one good-case run from end-to-end.

God View, and parallel ifs.

God View means one big view covering the union of all the different fields and layouts needed for the different steps of the submission process, with arcane 'attrs' rules switching fields on and off (see also "parallel ifs" below - this is a specific case of that). There is also a God Object issue but I'm not sure if that will be as easy to fix - we should look and see if it is though, but aggressively applying the Strategy pattern might resolve this.

"Parallel ifs" means we're switching on the same field in multiple methods, and/or have multiple dispatch tables on the same thing.

Replace the parallel ifs with Strategy pattern. Also use that to get rid of all the horrible switching on view name.

Replace the God View with a separate XML view per step (possibly with common stuff inherited from a base one) and switch between them in fields_view_get from the Strategies introduced when removing the parallel ifs.

Do a VAT submission that actually has VAT in it, look at logs:

2019-06-20 11:43:02,743 1 WARNING mtd-vat openerp.models: mtd_vat.vat_detailed_submission_logs.create() with unknown fields: amount_residual, amount_residual_currency, analytic_lines, balance, invoice, mtd_tax_amount, narration, vat, vat_submission_id
2019-06-20 11:43:02,746 1 WARNING mtd-vat openerp.models: mtd_vat.vat_detailed_submission_logs.create() with unknown fields: amount_residual, amount_residual_currency, analytic_lines, balance, invoice, mtd_tax_amount, narration, vat, vat_submission_id

Found in 8.0, possibly in all

We've mostly solved this as part of T15219, but we're not currently syncing. Instead we are flagging when we think it's done and checking before doing submission, for now. Our hand was forced because, at least in the sandbox, we're not getting returned updated status from the obligations endpoint after submission.

This bug would now add a 'synchronise' step beforehand.

This is to allows us to display what HMRC give us back unchanged while still giving accountants info they need about whether we owe or HMRC owe us.

A non-stored computed field {'positive', 'negative', 'zero'} net_due_sign {'reclaim', 'payment', 'nothing'} (name TBD) - dependent on whether we owe HMRC or they owe us or neither.

Get clarification from Stuart or someone in accounts as to which of 'positive' or 'negative' to use in which situation.

Another non-stored computed display field based on that to show 'Payment', 'Reclaim' or '' based on computed state.

I've installed your modules under Community v12.0 but I've got the warnings below.
Any thought?

2019-08-21 18:09:43,137 10551 WARNING Production odoo.fields: Field mtd_vat.vat_detailed_submission_logs.reconcile_partial_id with unknown comodel_name 'account.move.reconcile' 
2019-08-21 18:09:43,142 10551 WARNING Production odoo.fields: Field mtd_vat.vat_detailed_submission_logs.reconcile_id with unknown comodel_name 'account.move.reconcile' 
2019-08-21 18:09:43,143 10551 WARNING Production odoo.fields: Field mtd_vat.vat_detailed_submission_logs.tax_code_id with unknown comodel_name 'account.tax.code' 
2019-08-21 18:09:43,143 10551 WARNING Production odoo.fields: Field mtd_vat.vat_detailed_submission_logs.period_id with unknown comodel_name 'account.period' 
2019-08-21 18:09:43,145 10551 WARNING Production odoo.fields: Field mtd_vat.vat_detailed_submission_logs.product_uom_id with unknown comodel_name 'product.uom' 

Possible suggestions I've thought of so far are endpoint_record or even just endpoint if that wouldn't confuse us with the endpoint URL itself.

Also there are points in the API where record is declared optional but the method would crash without it.

We don't currently display _sum_year (we've removed it from the treeview), but we should look at it again in light of our changes to _sum_period and see whether that override can be removed entirely or needs updating to match new behaviour. Even though we don't display it, does something else use it, or would the system crash trying to compute it if we removed it?

The system is currently working, but if the code is unnecessary we should remove it and if it's needed but it now lies we should update it.

Dead code is worse than useless - it creates a drag on understanding for anyone wanting to fix a bug in the module and it can rear its ugly head at the worst possible moment if it's left in.

Calculate VAT button handles the use previous periods flag in one way (by passing in a different date_from in the context and letting the chart of taxes do things normally), and the Breakdown button does it the other way (it passes the flag to the chart of taxes wizard and then the chart of taxes handles it).

My understanding is this is down to project history where originally we were dealing with periods so the chart of taxes had to deal explicitly with the possibility of including previous periods, but nowadays the chart of taxes report takes a date range, so this can be handled by passing in a different date_from (like the Calculate VAT button does) in the VAT Breakdown button handler.

Nowadays however the VAT Breakdown button could feed the corrected date_from into the Chart of Taxes report, just like the Calculate VAT button does.

As it stands there will be two chunks of code doing the same thing with the same meaning, and this is asking for problems, and I have two code paths to review to ensure they are doing things consistently and ponder "if they were inconsistent, which one would be right?".

As it happens, we get the same results (at least with the O1851 case), but it has already caused me problems trying to unify the way the CoT report is configured for the two; wondering why we're doing different things with the chart of taxes parameters in the two different buttons has made a refactoring that should have taken in the order of 15 minutes and with a very low risk of regression to one that's more complicated with a need to test things I don't yet understand. As a result I have postponed that refactoring until later when we can properly verify that both code paths do the right thing and then lose the chart of taxes version of it.

That refactoring was to make one method, used by the Calculate VAT button handler and the View VAT Breakdown button handler, that gives the appropriate Chart of Taxes parameters, so any changes to that only have to be made in one place and we only have to understand the one code path.

no super on _sum, _sum_year, _sum_period in account_tax_code
no super on active in view_tree_js

This is no issue for my current implementation of the mtd module but may cause problems in multi-company/multi-country environments.

The vat box 4 on the View VAT Returns has the wrong box number this needs to be changed to 4 from 2.

2. VAT due in this period on acquisitions from other EC Member States

This also needs to be checked on other versions this issue is currently present on V10.

In the Chart of Taxes we have managed to get a sensible user error explaining that refreshing the page has lost the parameters from the report run.

If you click a row from the Chart of Taxes, and observe the set of journal items, it's all fine, but if you then press refresh when on that list, what you will get back is a larger non-strict subset of the journal items in the system.

I believe that's because in the javascript code where we ask for the domain we need to explicitly:

• catch the Warning exception raised from the Python code
• display the error message on the screen somewhere

However fixing this is out of the scope of the issue I'm currently working on.