sniner / docuware-client

A Python client library for the REST API of DocuWare's DMS
BSD 3-Clause "New" or "Revised" License
7 stars 6 forks source link

Feat/add create update document data #4

Closed patrick-cichon-gcx closed 1 year ago

patrick-cichon-gcx commented 1 year ago

Hey @sniner,

I've added some more file cabinet features to the library:

If you have any suggestions or improvements let me know!

Best, Patrick

sniner commented 1 year ago

Hi @patrick-cichon-gcx

These are great additions, thank you for your contribution to the project! Have you already tested them extensively?

patrick-cichon-gcx commented 1 year ago

Hey @sniner,

yes I just added some more code and return values for the functions to clarify how to use them. I also added some more documentation in the code and the README. In my tests everything worked well.

If you have any more suggestions let me know!

sniner commented 1 year ago

My problem with this PR is that the new functionality is not really pythonic. That is, it is not organized the way you would expect a Python library to be. For example: Why is get_file_cabinet_document_and_data() a method of Organization? It should be a method of FileCabinet and named get_document(), or even a simple FileCabinet.get() or [] implemented by __getitem__() might be a good solution. This would then look like this, for example (and this is only an example to clarify this, it would not be an accepted solution, because there is already existing code to retrieve a document):

class FileCabinet:
    ...

    def get_document(self, query:list=[]):
        dlg = self.search_dialog()

        document_object = {}
        for document in dlg.search(query):
            field_list = []

            for field in document.fields:
                document_fields = {}

                document_fields["field_id"] = field.id
                document_fields["content_type"] = field.content_type
                document_fields["name"] = field.name
                document_fields["value"] = field.value

                dt_fields = ["Stored on", "Modified on", "Last accessed on"]
                if any(field.name in dt_fields for item in dt_fields):
                    document_fields["value"] = datetime.strftime(field.value, '%Y-%m-%d %H:%M')
                else:
                    document_fields["value"] = field.value

                field_list.append(document_fields)

                document_object[document.document.id] = field_list

        return document_object

    def delete_document(self, query:list=[]):
        fc_id = self.id

        dlg = self.search_dialog()
        fc_search = dlg.search(query)
        if fc_search.count == 1:
            for result in fc_search:
                document_id = result.document.id
        elif fc_search.count < 1:
            log.warning('Delete search query returned no results, delete request will not be executed.')
            return False
        else:
            log.error('Delete search query returned more than 1 result, delete request can only be executed for one document. Please specify your search query.')
            return False

        headers = {
            "Content-Type": "application/json",
            "Accept": "application/json"
        }
        try:
            result = self.organization.conn.delete(f"{self.endpoints['filecabinets']}/{fc_id}/Documents/{document_id}", headers=headers)
        except Exception as e:
            log.error('Error creating document data in file cabinet: %s', e)
            return False
        return result

And then, please, please, please remove the print() calls and replace them with log.warning(), log.error(), or whatever corresponds to the appropriate level.

import logging

log = logging.getLogger(__name__)
...

My next question: in get_document() you search for documents matching a query: for document in dlg.search(query): and create a dict document_object. What happens if multiple documents match that query?

And the final question: What does Organization.get_file_cabinet_document_and_data() different than SearchResultItem.document()? And why does is not return a Document object but a dict? And btw: delete_document_from_file_cabinet() belongs to Document, not Organization, and should be named delete(). That would make the warning "Delete search query returned more than 1 result..." go away completely, because you method is conceptually at the wrong position (Organization instead of Document).

Sorry, I think this PR needs a major refactoring.

patrick-cichon-gcx commented 1 year ago

Hey @sniner,

thanks for the feedback. And of course, sorry I'll adjust the code according to your suggestions. I was kind of unsure regarding the placement of the functions. I'll rework the code and let you know when I'm finished.

patrick-cichon-gcx commented 1 year ago

Hey @sniner,

I did some refactoring. You can review the code and let me know what you think.

Changes made:

patrick-cichon-gcx commented 1 year ago

Hey @sniner,

did you have a chance to review the changes to the PR yet?

patrick-cichon-gcx commented 11 months ago

Hey @sniner,

could you also upload the new version to PyPi?