Mtd 11.0 wave 2 #4

ShreeOpusVL · 2019-01-29T16:26:20Z

Note from PA - I have re-done 11.0 branches and the mtd-11.0 branch from a clean 8.0 base, cherry picking all commits which were included in the previous PR. This was to get git out of the messy state it was in.

Once this is merged in and we can be sure that we didn't miss anything - branches origin/mtd-11.0-wave-1 and origin/mtd_w1 can be deleted. As far as we are concerned with PRs - those branches can be ignored for now

This commit contains successful completion of Hello world API and hello application API. There are some changes which are still required but at least we can establish a connection. The hello user step one getting the authorisation code successfully achieved (still need to handle the exceptions) and step two gaining the access code and refresh code also works. Now need to receive the hello user message (step three from the connection.)

Getting hello user message from HMRC completed and some UI changes have also been made.

user groups created added new data files for hello world. partial logging added to hello world.

some more log messages added and changes to some existing log messages

While testing found an error which is now been fixed. and made few minor changes

Changes request by Manish

MTD admin should alsonot be able to create any endpoints.

Mtd user should not be able to make changes to the HMRC configuration and should have create access to api_request_tracker and api_tokens

and why it could have happened

Making it simple to understand the construction of the authorisation url

dedent the parenthases

logger string changes for readability

and some logger message change

and using named parameters for .format to make it easier to read

…ject and then comparing the object rather than formatting date time into string for comperison. Also changed the name of a variable

PeterAlabaster

Much better and cleaner this time round, though please see my comments

PeterAlabaster · 2019-01-30T17:10:28Z

account_mtd/controllers/main.py

+
+ # to determine which API the authorization code has been received for.
+ api_tracker = http.request.env['mtd.api_request_tracker'].search([('closed', '=', False)])
+ if len(api_tracker) != 1:


Should this not read more like:

if not api_tracker: logger.warn("...") raise exceptions.Warning() elif len(api_tracker) > 1: logger.info("...") .... do something ... else: for record in api_tracker: .... do something ....

so that handling for none, one, and more than one is seperate? I'm just suggesting this based on the comment below about user should never be in a state where they found no tracker record

Agreed with @PeterAlabaster . It will keep comments, if any are necessary at all, brief. The log message can be more focussed too.

PeterAlabaster · 2019-01-30T17:15:50Z

account_mtd/models/mtd_api.py

+ _name = 'mtd.api'
+ _description = "list of api table"
+
+ name = fields.Char('Name', required=True, readonly=True)


My previous review still stands on all field declarations:
name = fields.Char() is the same as name = fields.Char('Name') and name = fields.Char(string='Name').
The only times where a string parameter is required in a field is if the string differs from the fieldname.

See https://www.odoo.com/documentation/10.0/reference/orm.html#creating-models

By default, the field's label (user-visible name) is a capitalized version of the field name, this can be overridden with the string parameter:

So I don't think some of these are totally necessary

I'm still in two minds about this. I think @james-opusvl is in the "be explicit" camp, whereas @PeterAlabaster is saying lean on the defaults. I can see benefits to both

I would say if you're labelling them explicitly, use the kwarg string='Label' rather than leaning on the positional arg order, which varies depending on the type of field. No need to go onto a new line if all you're passing in is string but do so if it takes the line over 80 chars.

PeterAlabaster · 2019-01-30T17:17:58Z

account_mtd/models/mtd_api_request_tracker.py

@@ -0,0 +1,27 @@
+from odoo import models, fields, api
+
+################################################################


I would suggest making this a class docstring like so:

class MtdApiRequestTracker(models.Model): """ This model is created to keep track of any pending requests for access token. If one request is in process we can not send a request for a new one. This also contains information regarding where the request was sent from so when redirected back to Odoo we redirect user back to where they started from """ _name = 'mtd.api_request_tracker' _description = "authorisation request table"

PeterAlabaster · 2019-01-30T17:18:23Z

account_mtd/models/mtd_api_tokens.py

@@ -0,0 +1,25 @@
+from odoo import models, fields, api
+
+##############################################################


The same applies for this one. Also suggest changing module to model

PeterAlabaster · 2019-01-30T17:19:42Z

account_mtd/models/mtd_hello_world.py

+ which_button_type_clicked = fields.Char(string="which_button")
+ path = fields.Char(string="sandbox_url")
+ current_record = fields.Char()
+ # need a variable which will keep the record id so that when we


This sentence cuts out half way through

PeterAlabaster · 2019-01-30T17:21:22Z

account_mtd/models/mtd_hello_world.py

+ ('res_id', '=', self.id), ('model', '=', 'mtd.hello_world')
+ ])
+
+ if endpoint_record.name == "mtd_hello_world_endpoint":


I think there is a more pythonic way of doing this, I will sit at your desk to run through it

In fact, is endpoint_record something which a user can edit? in which case it is probably better to have it hard-coded like this.

mapping = { 'mtd_hello_world_endpoint': '_handle_mtd_hello_world_endpoint', 'mtd_hello_application_endpoint': '_handle_mtd_hello_application_endpoint', 'mtd_hello_user_endpoint': '_handle_mtd_hello_user_endpoint' } if not mapping.get(endpoint_record.name): raise exceptions.Warning("Could not connect to HMRC! \nThis is not a valid HMRC service connection") return getattr(self, mapping.get(endpoint_record.name))

I think should work

Yep another step in the right direction (except you've not called the method returned by getattr()), but why not:

request_handlers = { ... } try: handler_name = request_handlers[endpoint_record.name] except KeyError: raise exceptions.Warning(...) else: handle_request = getattr(self, handler_name) return handle_request()

or

handler_name = request_handlers.get(endpoint_record.name) if handler_name: handle_request = getattr(self, handler_name) return handle_request() else: raise exceptions.Warning(...)

It's also possible to define handler_name in the above thus:

handler_name = "_handle_" + endpoint_record.name try: handle_request = getattr(self, handler_name) except AttributeError: raise exceptions.Warning() else: return handle_request()

BUT...

I think this actually fits the strategy pattern [1] but I don't know where the split between classes (or models in Odoo's case) would go without a much better understanding of how all these bits of logic and data are interacting.

[1] https://en.wikipedia.org/wiki/Strategy_pattern

PeterAlabaster · 2019-01-30T17:34:38Z

account_mtd/models/mtd_hello_world.py

+ _name = 'mtd.hello_world'
+ _description = "Hello world to test connection between Odoo and HMRC"
+
+ _logger = logging.getLogger(__name__)


if you make _logger global (put it just under from urllib.parse import urlparse) then you can change every instance of self._logger() to just _logger

Seconded.

Leave a couple of blank lines either side of it for pep8 though.

PeterAlabaster · 2019-01-30T17:36:08Z

account_mtd/models/mtd_hello_world.py

+ else:
+ return self.get_user_authorisation()
+
+ def _json_command(self, command, timeout=3, record_id=None):


These are pretty hefty functions. Is there any way that these could be reduced, like having common shared functionality (it may not be shared yet, but might be in future?) split out into smaller functions?

Doesn't need to be shared functionality to be worthy of being in a method, but common stuff (as long as it's not only common by coincidence) should be. If something is a sequence of things that need to be done.

_this() _that() _the_other()

and each of those functions' bodies explain how to do this, that and the other, it's much easier to read. Just be careful not to mix levels of abstraction.

and using getattr to call the functions.

Nick-OpusVL

Improved, but I have more questions and suggestions. Some comments are in answer to Peter's so I hope it gives you the full conversation...

Nick-OpusVL · 2019-01-31T10:29:31Z

account_mtd/controllers/main.py

+ def get_user_authorization(self, **args):
+
+ # to determine which API the authorization code has been received for.
+ api_tracker = http.request.env['mtd.api_request_tracker'].search([('closed', '=', False)])


Pluralise this name - we can't assert there is only one at this stage.

Nick-OpusVL · 2019-01-31T10:32:58Z

account_mtd/controllers/main.py

+
+ # to determine which API the authorization code has been received for.
+ api_tracker = http.request.env['mtd.api_request_tracker'].search([('closed', '=', False)])
+ if len(api_tracker) != 1:


Agreed with @PeterAlabaster . It will keep comments, if any are necessary at all, brief. The log message can be more focussed too.

Nick-OpusVL · 2019-01-31T10:34:37Z

account_mtd/controllers/main.py

+ "\nWe can have more than one if there was an initial request and authorisation it was never completed."+
+ "\nIf in this state we need to reset the tracker and get user to create a new connection " +
+ "\napi_tracer = {}".format(api_tracker))
+ for record in api_tracker:


Equivalent:

api_trackers.write({'closed': True})

(I assume you now mean closed = True as that's what you're searching on earlier, apologies if not).

The log messages are a bit verbose. What you're about to do or the results of what you've just done are fine, but don't embed pseudocode in them.

Nick-OpusVL · 2019-01-31T10:40:45Z

account_mtd/models/mtd_api_tokens.py

+ _name = 'mtd.api_tokens'
+ _description = "api token table"
+
+ api_id = fields.Char('API Id', required=True)


How does this map onto the HMRC API?

Nick-OpusVL · 2019-01-31T10:41:15Z

account_mtd/models/mtd_api_tokens.py

+ _description = "api token table"
+
+ api_id = fields.Char('API Id', required=True)
+ api_name = fields.Char('API Name', required=True)


How does this map onto the HMRC API? I presume it's different from the api_id.

Nick-OpusVL · 2019-01-31T12:14:25Z

account_mtd/models/mtd_hello_world.py

+ # refresh_token = token_record.refresh_token if token_record else ""
+
+ header_items = {"Accept": "application/vnd.hmrc.1.0+json"}
+ if self.which_button_type_clicked == "application":


This is smelling like strategy based on endpoint again, but not completely sure.

Yes headers are different for different endpoints

I didn't mean that, I was talking about the switching on which_button_type_clicked.

Yes based on the endpoints, headers are different which is why I have used an internal variable to find out which endpoint I was in. Originally we just had one screen with three different buttons which is why I used the variable name "which_button_type_clicked."

What does it mean now then?

which endpoint did the user come from hello world, application or user. I will change the name of the variable.

Nick-OpusVL · 2019-01-31T12:16:33Z

account_mtd/models/mtd_hello_world.py

+ )
+ response = requests.get(authorisation_url, timeout=3)
+ # response_token = json.loads(req.text)
+ self._logger.info(


I wonder if some of these belong at debug rather than info level.

Nick-OpusVL · 2019-01-31T12:17:13Z

account_mtd/models/mtd_hello_world.py

+ else:
+ response_token = json.loads(response.text)
+ error_message = (
+ "Date {date} Time {time} \n".format(date=datetime.utcnow().date(), time=datetime.utcnow().time())


Do you want to display times to users in UTC?

yes in the time zone they are in right? That's what you suggested in the previous pr.

No they're in Europe/London. When BST starts these strings will be 1 hour out.

The ORM's date/datetime/time fields work in UTC (which is the context in which I suggested using utcnow()), but when formatting a string for humans you need to use local time.

Nick-OpusVL · 2019-01-31T12:23:48Z

account_mtd/models/mtd_hello_world.py

+ else:
+ return self.get_user_authorisation()
+
+ def _json_command(self, command, timeout=3, record_id=None):


Doesn't need to be shared functionality to be worthy of being in a method, but common stuff (as long as it's not only common by coincidence) should be. If something is a sequence of things that need to be done.

_this() _that() _the_other()

and each of those functions' bodies explain how to do this, that and the other, it's much easier to read. Just be careful not to mix levels of abstraction.

Nick-OpusVL · 2019-01-31T12:31:16Z

account_mtd/models/mtd_api.py

+ _name = 'mtd.api'
+ _description = "list of api table"
+
+ name = fields.Char('Name', required=True, readonly=True)


I'm still in two minds about this. I think @james-opusvl is in the "be explicit" camp, whereas @PeterAlabaster is saying lean on the defaults. I can see benefits to both

I would say if you're labelling them explicitly, use the kwarg string='Label' rather than leaning on the positional arg order, which varies depending on the type of field. No need to go onto a new line if all you're passing in is string but do so if it takes the line over 80 chars.

adding an extra selection option for the variable

adding an if to make sure we have a value for the error variable only then we need to add a new line

from date and time field

Needed to change the varible name in the data file as the variable name in the module had been changed.

Whether the response is positive or negative we still need to close the request tracker so that user can place a new request if required.

We need to be able to inherit the hello world module for other mtd modules and use the authorisation steps.

ShreeOpusVL added 15 commits January 29, 2019 16:19

[IMP][O1758]- MTD development step 3 complete.

e9c64af

Getting hello user message from HMRC completed and some UI changes have also been made.

[IMP][O1758] Step 4 refresh development complete

2ad72a0

user groups created added new data files for hello world. partial logging added to hello world.

[IMP][O1758] pr changes

9382960

some more log messages added and changes to some existing log messages

[IMP][O1758] Missed few pr changes - now added

52dc6c7

[IMP][O1758] Pep8 changes

22b1c2f

[IMP][O1758] fixing the error on hello user

ac1c7ef

While testing found an error which is now been fixed. and made few minor changes

[IMP][O1758] Fall back for request tracker added

554207b

[IMP][O1758] Changes to the lables

7bdae2b

[IMP][O1758] fixing typo

f0e1992

[IMP][O1758]Adding accidentally removed files

afae63c

[IMP][O1758]Error fixing

e3028de

[IMP][O1758] Changes to variable name.

8c2a952

Changes request by Manish

[IMP][O1758] Security changes for mtd admin

9438d37

MTD admin should alsonot be able to create any endpoints.

[IMP][O1758]security changes for mtd user

793b937

Mtd user should not be able to make changes to the HMRC configuration and should have create access to api_request_tracker and api_tokens

ShreeOpusVL requested a review from PeterAlabaster January 29, 2019 16:26

ShreeOpusVL added 5 commits January 29, 2019 16:58

[IMP][O1758]logging message stating what happend

096fb08

and why it could have happened

[IMP][O1758]Reson for model existence added

d607f4b

[IMP][O1758] Reason for model existence added

17af863

[IMP][O1758] typo and refactoring each if to a single call

4e0e1e0

[IMP][O1758] Variable name change

e5e9bf7

PeterAlabaster removed their request for review January 29, 2019 17:57

ShreeOpusVL added 8 commits January 29, 2019 18:03

[IMP][O1758] authorisation url construction

9831e45

Making it simple to understand the construction of the authorisation url

[IMP][O1758] Variable name change.

98e29db

[IMP][O1758]adding trailing comma

80465f1

dedent the parenthases

[IMP][O1758] Adding explanation for a dict

de8f502

logger string changes for readability

[IMP][O1758] Removed hard coded menu_id (left in by mistake)

cd6aab5

[IMP][O1758] adding missing return

79962a4

and some logger message change

[IMP][O1758] change as per pr

260c73b

[IMP][O1758] changed datetime.now to datetime.utcnow

18d34b8

and using named parameters for .format to make it easier to read

[IMP][O1758] adding missing import and vars for format

64debca

ShreeOpusVL requested review from Nick-OpusVL, james-opusvl and PeterAlabaster January 30, 2019 12:40

[IMP][O1758] Formatting date time field from database to date time ob…

63fbee4

…ject and then comparing the object rather than formatting date time into string for comperison. Also changed the name of a variable

PeterAlabaster suggested changes Jan 30, 2019

View reviewed changes

ShreeOpusVL added 5 commits January 31, 2019 09:55

[IMP][O1758] removing field declaration of string where not required

8c83742

[IMP][O1758]chanign comments to a class docstring

4f21e60

[IMP][O1758] Removal of variable which is no longer in use

33e04bb

[IMP][O1758] changing the _logger to a global variable

d511d1b

[IMP][O1758] change as per pr to remove the if block

151c6e0

and using getattr to call the functions.

Nick-OpusVL suggested changes Jan 31, 2019

View reviewed changes

ShreeOpusVL added 15 commits January 31, 2019 14:53

[IMP][O1758] chanign variable name to new name

5b36d33

adding an extra selection option for the variable

[IMP][O1758] Variable name change and format change

4eb1bef

[IMP][O1758] variable name pluralising

ec7e436

[IMP][O1758] fields has to be relational to api model

df6ab31

[IMP][O1758] Pep8 changes

560e426

[IMP][O1758]Changes as per pr

5eafc11

[IMP][O1758] removing a comment

93122c3

[IMp][O1758] attempting to reduce the function size

c3800e1

[IMP][O1758] variable name change

1cea388

adding an if to make sure we have a value for the error variable only then we need to add a new line

[IMP][O1758]Variable name change in xml and adding string name

2ed8a34

[IMP][O1758] making the function smaller

d60554d

[IMP][O1758] variable name change and removing utc.now

8e6ab42

from date and time field

[IMP][O1758]Variable name change

32f3b49

Needed to change the varible name in the data file as the variable name in the module had been changed.

[FIX][O1758]Request tracker needs to be closed

abcb340

Whether the response is positive or negative we still need to close the request tracker so that user can place a new request if required.

[IMP][O1758] separating hello world and authorisation steps

b7cafa1

We need to be able to inherit the hello world module for other mtd modules and use the authorisation steps.

		@@ -0,0 +1,27 @@
		from odoo import models, fields, api

		################################################################

		@@ -0,0 +1,25 @@
		from odoo import models, fields, api

		##############################################################

Mtd 11.0 wave 2 #4

Are you sure you want to change the base?

Mtd 11.0 wave 2 #4

Conversation

ShreeOpusVL commented Jan 29, 2019 • edited Loading

PeterAlabaster left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nick-OpusVL Jan 31, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nick-OpusVL Jan 31, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nick-OpusVL Jan 31, 2019 • edited Loading

Choose a reason for hiding this comment

Nick-OpusVL left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nick-OpusVL Jan 31, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShreeOpusVL Feb 4, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShreeOpusVL Feb 4, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nick-OpusVL Jan 31, 2019 • edited Loading

Choose a reason for hiding this comment

Nick-OpusVL Jan 31, 2019 • edited Loading

Choose a reason for hiding this comment

ShreeOpusVL commented Jan 29, 2019 •

edited

Loading

Nick-OpusVL Jan 31, 2019 •

edited

Loading

Nick-OpusVL Jan 31, 2019 •

edited

Loading

Nick-OpusVL Jan 31, 2019 •

edited

Loading

Nick-OpusVL Jan 31, 2019 •

edited

Loading

ShreeOpusVL Feb 4, 2019 •

edited

Loading

ShreeOpusVL Feb 4, 2019 •

edited

Loading

Nick-OpusVL Jan 31, 2019 •

edited

Loading

Nick-OpusVL Jan 31, 2019 •

edited

Loading