[MVP1][tk116] Alteração do módulo catalogmanager.persistence #117

patymori · 2018-06-13T17:54:11Z

Fixes #116.

Criação de insert_file nos DBManagers
Criação de ChangesService.register com o novo conteúdo do registro
de mudança
Criação de DatabaseService.add_file para persistir arquivos usando o
DBManager e registrando a mudança com ChangesService.register
Ajustes nos fixtures para executar os testes sem duplicação
Testes unitários e de integração do módulo persistence para o novo
registro de artigos

- Criação de `insert_file` nos DBManagers - Criação de `ChangesService.register` com o novo conteúdo do registro de mudança - Criação de `DatabaseService.add_file` para persistir arquivos usando o DBManager e registrando a mudança com `ChangesService.register` - Ajustes nos fixtures para executar os testes sem duplicação - Testes unitários e de integração do módulo persistence para o novo registro de artigos

…-tk106

robertatakenaka · 2018-06-14T14:05:09Z

persistence/services.py

        return change_record['record_id']

+    def register(self, record_id, change_type):
+        sequencial = str(self.seqnum_generator.new())


@patymori : sendo str, não corre o risco de a ordem válida ser 1, 10, 11 no lugar de 1, 2, 3?

Essa dúvida deveria, idealmente, ser respondida por meio de testes de unidade ;)

robertatakenaka · 2018-06-14T14:10:59Z

persistence/services.py

+        file_id: ID do arquivo
+        content: conteúdo do arquivo
+        """
+        self.db_manager.insert_file(file_id=file_id, content=content)


@patymori : está sendo considerado que tanto os arquivos e os manifestos serão gravados usando o mesmo db_manager. Isso será disvinculado futuramente, correto?

@robertatakenaka, isso foi um problema q eu decidi abordar de outra forma: estou pensando em gerenciar 2 db_managers diferentes, 1 para cada database

@patymori ok, minha dúvida é 2 db_service (1 para cada db_manager) ou 1 db_service com 2 db_managers?

Desculpe-me, @robertatakenaka, 2 db_services.

robertatakenaka · 2018-06-14T14:35:14Z

persistence/tests/conftest.py

-        db_service.db_manager.drop_database()
-        db_service.changes_service.changes_db_manager.drop_database()
+        try:
+            db_service.db_manager.drop_database()


@patymori : db_service deveria ter um drop_database para apagar em cascata.

robertatakenaka · 2018-06-14T14:36:57Z

persistence/tests/test_changes.py



-def test_register_create_change(database_service):
+def test_register_change_create(database_service):


@patymori : sim fica melhor invertido

robertatakenaka · 2018-06-14T14:42:52Z

persistence/tests/test_changes.py

+    )
+    assert change_id is not None
+    check_change = dict(
+        database_service.changes_service.changes_db_manager.database[change_id]


Por esta linha, me parece que deveríamos ter previsto um changes_service.get(change_id)

Penso que não é necessário pois não faz sentido para a solução, visto que a aplicação não oferecerá consulta/leitura de uma mudança pelo ID, certo?

robertatakenaka · 2018-06-14T14:52:51Z

persistence/tests/test_databases.py

+        file_id='href_file1',
+        content=xml_test.encode('utf-8')
+    )
+    file = db_manager_test.database.get(


por que a recuperação do file não é db_manager_test.get_file(file_id)?

Porque eu tava implementando só o incluir. Não existe ainda nessa implementação um recuperar arquivo pelo ID. Podemos refatorar esse teste quando ele existir, o que acha?

robertatakenaka · 2018-06-18T11:52:25Z

persistence/tests/test_changes.py

+        changes_list[i]['change_id'] < changes_list[i + 1]['change_id']
+        for i in range(len(changes_list) - 1)
+    ])
+


@patymori : Referente às linhas 72 a 75:
Pode-se comparar também usando assert sorted(items) == items

Por curiosidade, fiz um teste de desempenho.

assert sorted(_items) == _items é mais eficiente.

Para 10 items, a diferença é irrelevante: 1s.
Para 100 items, a diferença é 66 - 38.

>>> t1 = datetime.now(); _items = [item['x'] for item in items];assert sorted(_items) == _items;t2 = datetime.now(); t = t2-t1;print(t); print(t1);print(t2) 0:00:00.000038 2018-06-18 08:41:37.630788 2018-06-18 08:41:37.630826 >>> t1 = datetime.now(); assert all([items[i]['x'] < items[i+1]['x'] for i in range(len(items)-1)]);t2 = datetime.now(); t = t2-t1;print(t); print(t1);print(t2) 0:00:00.000066 2018-06-18 08:41:51.818453 2018-06-18 08:41:51.818519

- Ajustes em `managers` para as mudanças em `persistence`. - Alteração do ArticleDocument para prever o registro de versões diferentes do artigo. - Ajuste do ArticleManager para as alterações do ArticleDocument, o gerenciamento de um DBManager para os arquivos brutos, e a implementação do método `add_document`. Ajuste do método post na view de artigo. - Criação de teste funcional para a funcionalidade de registro de novo artigo.

gustavofonseca · 2018-06-19T19:09:31Z

@patymori, você poderia resolver os conflitos?

gustavofonseca · 2018-06-19T19:30:50Z

managers/__init__.py

 from managers.article_manager import ArticleManager
 from managers.exceptions import ManagerFileError
-from managers.models.article_model import ArticleDocument
+from managers.models.article_model import ArticleDocument, InvalidXMLContent


InvalidXMLContent é uma exceção. Não deveria estar baixo managers.exceptions?

Criei managers.exceptions para ter somente as exceções que serão externalizadas, com o intensão de manter uma interface do que seria o módulo de persistência da aplicação WEB. Não sei se faz sentido.

gustavofonseca

Estes são comentários de uma revisão parcial. Vou continuar amanhã. Prometo ;)

Em geral eu diria que a implementação está boa. Só gostaria de chamar à atenção para o excesso de responsabilidades de alguns métodos como o ArticleDocument.add_version por exemplo.

gustavofonseca · 2018-06-19T19:36:08Z

managers/__init__.py



 def _get_article_manager(**db_settings):
    database_config = db_settings


aqui database_config não serve para nada, uma vez que as cópias das linhas 36 e 38 poderiam partir diretamente de db_settings

gustavofonseca · 2018-06-19T19:44:29Z

managers/__init__.py

+        codificado em XML
+    :rtype: str
+    """
+    try:


Sugiro envolver na estrutura try..except o mínimo de código possível, i.e., apenas o que de fato tem o potencial de lançar a exceção que será tratada. Nessa função seria algo como:

article_manager = _get_article_manager(**db_settings) article_document = ArticleDocument(article_id) try: article_document.add_version(xml_id, xml_file) except InvalidXMLContent as e: raise ManagerFileError(message=e.message) from e else: return article_manager.add_document(article_document)

gustavofonseca · 2018-06-19T20:14:09Z

managers/__init__.py

    """
    xml_file = File(file_name="xml_file.xml", content=xml_content)
-    article = ArticleDocument(article_id, xml_file)
+    article = ArticleDocument(article_id)


Cheiro de inicialização incompleta de ArticleDocument

ArticleDocument pode ser instanciado sem o conteúdo do XML, como ao recuperar o manifesto. Há a possibilidade de colocá-lo como um parâmetro não obrigatório. Melhor?

gustavofonseca · 2018-06-19T20:39:14Z

managers/__init__.py

+    article = ArticleDocument(article_id)
+    article.xml_file = xml_file
    for name in article.assets:
        if name in assets_filenames:


Não entendi a necessidade do argumento assets_filenames. O nome de identificação dos ativos digitais contigos no XML não podem ser obtidos inspecionando o próprio XML?

Esse método só foi alterado por conta da alteração de ArticleDocument e vem de implementações antigas que, provavelmente, serão alteradas. Podemos criar algum comentário nesse ponto para lembrar de revisá-lo ou criamos uma issue de melhoria?

gustavofonseca · 2018-06-19T20:55:55Z

managers/models/article_model.py

+        self.xml_tree = ArticleXMLTree(xml_content)
+        if self.xml_tree.xml_error:
+            raise InvalidXMLContent
+        checksum = hashlib.sha1(xml_content).hexdigest()


checksum deveria ser um método de ArticleXMLTree

gustavofonseca · 2018-06-19T21:03:23Z

managers/article_manager.py


    def receive_xml_file(self, id, xml_file):
-        article = ArticleDocument(id, xml_file)
+        article = ArticleDocument(id)


Inicialização incompleta de ArticleDocument?

…-tk106

- Ajuste do `managers._get_article_manager` - Ajuste em `post_article` no tratamento de exceção e nos testes - Criada property `ArticleXMLTree.checksum`

Patricia Morimoto added 2 commits June 13, 2018 14:42

Merge branch 'master' of github.com:patymori/catalogmanager into mvp1…

33fb769

…-tk106

patymori requested a review from robertatakenaka June 13, 2018 17:54

Patricia Morimoto added 2 commits June 13, 2018 15:06

Adding Prometheus metric to add_file

61ffb0f

DBManager.insert_file returns absolut file URL

5d3bf66

robertatakenaka reviewed Jun 14, 2018

View reviewed changes

Ajuste do sequencial na inserção do novo registro de mudança

3db3ad2

robertatakenaka reviewed Jun 18, 2018

View reviewed changes

robertatakenaka approved these changes Jun 18, 2018

View reviewed changes

gustavofonseca reviewed Jun 19, 2018

View reviewed changes

Patricia Morimoto added 2 commits June 20, 2018 11:48

Merge branch 'master' of github.com:patymori/catalogmanager into mvp1…

34c3626

…-tk106

Ajustes relacionados aos comentários do PR

76896da

- Ajuste do `managers._get_article_manager` - Ajuste em `post_article` no tratamento de exceção e nos testes - Criada property `ArticleXMLTree.checksum`



		def test_register_create_change(database_service):
		def test_register_change_create(database_service):



		def _get_article_manager(**db_settings):
		database_config = db_settings

[MVP1][tk116] Alteração do módulo catalogmanager.persistence #117

Are you sure you want to change the base?

[MVP1][tk116] Alteração do módulo catalogmanager.persistence #117

Uh oh!

Conversation

patymori commented Jun 13, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gustavofonseca commented Jun 19, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gustavofonseca left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants