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.

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

Create customer pos #6

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

Conversation

hassaanalansary
Copy link
Collaborator

Let me know if you have any comments

Copy link
Member

@bedilbek bedilbek left a comment

Choose a reason for hiding this comment

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

I am waiting for your response to the comments I have left it here

objects = QBDTaskQuerySet.as_manager()

def get_request(self):
obj_class = import_object_cls(self.qb_resource)
service = obj_class.get_service()()
service.qb_type = self.realm.qb_type # to generate proper XML
Copy link
Member

Choose a reason for hiding this comment

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

I am curious, whether we really have to store qb_type as a property of Service?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

qb_type is used int _prepare_request() in services/base.py, to generate XML
I just needed a way to pass qb_type to Service.
This is the best I could came up with. I don't mind if you have a better solution to pass qb_type.

This :

@property
def qb_type(self):
        raise NotImplementedError("qb_type is a required")

makes sure it is implemented in QBDTaskMixin



class ResponseProcessor:
resource = None
op_type = None
obj_class = None

def __init__(self, response, hresult, message):
def __init__(self, response, hresult, message, realm):
Copy link
Member

Choose a reason for hiding this comment

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

You have to rebase your branch from master because master branch is diverted, and I came to conclusion that I do not have to store realm attribute inside ResponseProcessor, but instead, I have to pass it as an argument while processing ResponseProcessor. What do you think about it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I agree, I found that you _process upon initialization of the object, so I had to pass realm in __init__.
can you show me code on how to achieve that?

BTW I believe this branch is rebased but from master, however I am still learning git and github.


def _process(self):
def _process(self, qb_type):
xml_type = utils.get_xml_type(qb_type)
Copy link
Member

Choose a reason for hiding this comment

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

I actually understand this situation, because after adding POS integration, we will not have static xml type any more. So, maybe refactoring of Processors also needed. Hmm, interesting. The more project gets complex, the more I contemplate that I have not enough time to contribute and enough skills to envision the future path of the project.

module_path, class_name = val.rsplit('.', 1)
module = import_module(module_path)
return getattr(module, class_name)
if val:
Copy link
Member

Choose a reason for hiding this comment

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

why we need check val for existence?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I couldn't understand the reason, but while processing the response, it was throwing an error, because val was empty was empty sometimes

@@ -4,13 +4,14 @@

from django_quickbooks.views import QuickBooksService, Support

app_name = "django_quickbooks"
Copy link
Member

Choose a reason for hiding this comment

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

I believe we do not need to add app_name.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

we don't need it, but it is nice to have it, to avoid confusion when referring to any url.
i.e. when accessing the url from the user project, in case they have similar url confs

@@ -166,6 +166,9 @@ def receiveResponseXML(ctx, ticket, response, hresult, message):
if hresult:
print("hresult=" + hresult)
print("message=" + message)
# FIXME: I know this is a hacky way to get around encoding, needs a better solution
Copy link
Member

Choose a reason for hiding this comment

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

Does the response come explicitly in windows-1252 encoding?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes, please take a look at data/pos/item_inventory_add_response.xml

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants