Altera reg javascript do c.upload / c.p.calendar. #474

idgserpro · 2018-05-15T17:11:23Z

Altera ordem de registro de javascript do collective.upload e
collective.portlet.calendar para corrigir alinhamento do título
(refs. plonegovbr/brasil.gov.agenda#82)

Boas práticas para o PR: Checklist

Foi aberta uma issue relativa a esse PR;

Sim
Não
Não se aplica

Foram adicionados testes unitários;

Sim
Não
Não se aplica

Foram adicionados testes robots;

Sim
Não
Não se aplica

Foram adicionados upgradeSteps;

Sim
Não
Não se aplica

Foi adicionada a modificação no CHANGES.rst do pacote, sempre no topo do arquivo (ou seja, na primeira linha do release ainda não lançado), contendo o seu nome de usuário do github e a referência ao issue que você está tratando (não esqueça de colocar no fim do arquivo a url para a issue, use o padrão do próprio arquivo);

Sim
Não
Não se aplica

PR não contém assuntos diferentes. PR's devem ter um objetivo claro para facilitar o review. Ex: não junte, num mesmo PR, alterações de code-analysis e de implementação. Faça um PR de code-analysis e depois o da implementação em si;

Sim
Não
Não se aplica

Evite muitos commits pequenos de um mesmo assunto no PR, sempre que possível efetue rebase;

Sim
Não
Não se aplica

Altera ordem de registro de javascript do collective.upload e collective.portlet.calendar para corrigir alinhamento do título (refs. plonegovbr/brasil.gov.agenda#82)

hvelarde

na minha opinião acho melhor e mais limpo implementar isso aqui usando código Python ao invés de registrando mais um profile; o código só tem que se preocupar por colocar os estilos do calendário como último item. temos que deixar bem indicado que cada vez que se atualize a versão do collective.upload vai ser necessário chamar esse código antes de cozinhar novamente os recursos para ficar na ordem certa sempre.

idgserpro · 2018-05-16T17:15:35Z

No primeiro ponto (usar profile ou não) teremos um impasse pois já acho esse fato mais uma questão de preferência e não como "erro" a ser corrigido, rs. Eu não concordo em fazer código Python se já existe código Python pronto e testado do Plone que processa o xml. Mas entendi uma motivação maior nesse caso, devido a esse fato mencionado por você:

temos que deixar bem indicado que cada vez que se atualize a versão do collective.upload vai ser necessário chamar esse código antes de cozinhar novamente os recursos para ficar na ordem certa sempre.

Ou seja, se eu fizesse esse upgrade via código Python, facilitaria chamar esse código toda vez que uma versão do collective.upload fosse atualizada (colocaríamos em https://github.com/plonegovbr/portal.buildout/blob/master/buildout.d/versions.cfg o aviso # ATENÇÃO: Sempre chamar método XPTO via upgradeStep em brasil.gov.portal se for alterar a versão. Ver https://github.com/plonegovbr/brasil.gov.agenda/issues/82#issuecomment-383599037.

Acontece que nesse caso também acho a preocupação exagerada. Depois que a ordem é consertada tanto em novas instalações > 1.5.1 do zero quanto em atualizações de versões antigas, dificilmente ela será alterada. Acredito que a única situação em que isso poderia de fato acontecer seria alterações no cssregistry tanto de collective.upload quando do collective.portlet.calendar.

Nesse contexto, prefiro adicionar um teste a mais, garantindo que não tem nenhum insert-after ou insert-before no cssregistry.xml de collective.portlet.calendar e collective.upload, ao invés de refazer o profile via código e ainda por cima ter de ficar lembrando de chamar esse método toda vez que atualizar essa dependência do collective.upload. Se um dia tiver alteração de ordem no registro de css desses componentes, nosso teste quebra cobrando uma intervenção humana que verá essa discussão.

hvelarde

sugiro colocar isso para revisão de alguém mais para chegar a um acordo sobre como implementar.

hvelarde · 2018-05-16T17:33:47Z

src/brasil/gov/portal/profiles/default/cssregistry.xml

@@ -6,4 +6,11 @@
 <stylesheet title="" cacheable="True" compression="safe" cookable="True"
    enabled="1" expression="" id="++resource++brasil.gov.portal/css/main-print.css" media="print"
    rel="stylesheet" rendering="link" insert-after="++resource++brasil.gov.portal/css/main.css"/>
+ <stylesheet title="" cacheable="True" compression="none" cookable="True"


um desses itens é redundante e desnecessário: ao colocar o primeiro antes do segundo, já está garantido que o segundo ficou depois do primeiro.

Isso é pra garantir ao máximo não só que esteja atrás, mas imediatamente atrás do do collective.portlet.calendar pois foi assim que foi testado. Parece redundante mas foi pensado assim.

As demais considerações serão implementadas e adicionadas aqui no PR.

mas isso exatamente o que faz o insert-after: ele coloca imediatamente atrás; não precisam do outro.

hvelarde · 2018-05-16T17:34:47Z

src/brasil/gov/portal/tests/test_setup.py

+        portlet_calendar_css_id = '++resource++calendar_styles/calendar.css'
+        upload_pos = portal_css.getResourcePosition(upload_css_id)
+        calendar_pos = portal_css.getResourcePosition(portlet_calendar_css_id)
+        self.assertTrue(upload_pos < calendar_pos)


usar self.assertLess()

hvelarde · 2018-05-16T17:35:26Z

src/brasil/gov/portal/upgrades/v10804/profiles/default/cssregistry.xml

@@ -0,0 +1,10 @@
+<?xml version="1.0"?>
+<object name="portal_css" meta_type="Stylesheets Registry" purge="False">
+ <stylesheet title="" cacheable="True" compression="none" cookable="True"


um desses itens é redundante e desnecessário: ao colocar o primeiro antes do segundo, já está garantido que o segundo ficou depois do primeiro.

hvelarde · 2018-05-16T17:35:52Z

src/brasil/gov/portal/tests/test_upgrades.py

+        self._do_upgrade(step)
+        upload_pos = portal_css.getResourcePosition(upload_css_id)
+        calendar_pos = portal_css.getResourcePosition(portlet_calendar_css_id)
+        self.assertTrue(upload_pos < calendar_pos)


usar self.assertLess()

hvelarde · 2018-05-16T17:36:16Z

src/brasil/gov/portal/tests/test_upgrades.py

+        portal_css.moveResourceToBottom(upload_css_id)
+        upload_pos = portal_css.getResourcePosition(upload_css_id)
+        calendar_pos = portal_css.getResourcePosition(portlet_calendar_css_id)
+        self.assertTrue(upload_pos > calendar_pos)


usar self.assertGreater()

idgserpro · 2018-05-17T15:21:02Z

@agnogueira @claytonc e @rodfersou, estamos num impasse sobre esse pull request, com relação a usar ou não profile no upgradeStep levantada pelo @hvelarde em #474 (review) e respondida por mim em #474 (comment). Poderiam nos dar suas opiniões?

claytonc · 2018-05-17T20:29:19Z

@idgserpro @hvelarde

AS ordens de css e js já me deu muito dor de cabeça e demorei para descobri o problema.

Se erro no estilo só ocorre sempre que é atualizado o collective.upload, seria bom ter isso documentado, poderia até criar uma sessão na documentação de erros mais comuns e como corrigi-los.

Mas se tive alguma forma automatizar para evitar é melhor.

Mas eu deixaria redundante só se existisse alguma possibilidade de não ficar na ordem certa os css.

idgserpro · 2018-05-18T15:50:15Z

Se erro no estilo só ocorre sempre que é atualizado o collective.upload, seria bom ter isso documentado, poderia até criar uma sessão na documentação de erros mais comuns e como corrigi-los.

É imprevisível caso uma pessoa clique em "reinstall". Mas isso já é documentado como não recomendado em http://identidade-digital-de-governo-plone.readthedocs.io/en/latest/atualizacao/#execucao-de-reinstall-em-portal-quickinstaller

A redundância é uma tentativa de automatização para impedir que as ordens incorretas ocorram, mesmo com reinstall. Não consigo imaginar outra automatização.

hvelarde · 2018-05-18T19:25:04Z

não é só redundância, é simplesmente desnecessário :)

idgserpro · 2018-05-24T10:47:15Z

@hvelarde muita da discussão aqui é preferência, o único ponto divergente acabou sendo o uso de "insert-before" e "insert-after" ao mesmo tempo, vou remover então por mais de um ter achado redundante pra você aprovar o PR, precisamos disso aqui mesclado para dar prosseguimento em outros PRs.

Ficar atento às várias opções disponibilizadas em https://docs.python.org/2/library/unittest.html#assert-methods A principal vantagem é a legibilidade: só de ler o assert você já entende a lógica ao invés de tentar interpretar a lógica interna que voltaria True para assertTrue anteriormente. Registros de css também não necessitam de 'insert-after' E 'insert-before' por ser redundante.

Altera reg javascript do c.upload / c.p.calendar.

44f5ead

Altera ordem de registro de javascript do collective.upload e collective.portlet.calendar para corrigir alinhamento do título (refs. plonegovbr/brasil.gov.agenda#82)

idgserpro requested a review from hvelarde May 15, 2018 17:11

idgserpro mentioned this pull request May 15, 2018

Issue 82 erro portlet calendario idgserpro plonegovbr/brasil.gov.agenda#87

Merged

hvelarde requested changes May 16, 2018

View reviewed changes

idgserpro force-pushed the issue-82-brasil-gov-agenda-erro-portlet-calendario-idgserpro branch from 7fd8f94 to a2f22dd Compare May 24, 2018 10:53

hvelarde approved these changes May 24, 2018

View reviewed changes

hvelarde merged commit 1d6a254 into 1.x May 24, 2018

hvelarde deleted the issue-82-brasil-gov-agenda-erro-portlet-calendario-idgserpro branch May 24, 2018 12:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Altera reg javascript do c.upload / c.p.calendar. #474

Altera reg javascript do c.upload / c.p.calendar. #474

idgserpro commented May 15, 2018

hvelarde left a comment

idgserpro commented May 16, 2018

hvelarde left a comment

hvelarde May 16, 2018

idgserpro May 17, 2018

hvelarde May 17, 2018

hvelarde May 16, 2018

hvelarde May 16, 2018

hvelarde May 16, 2018

hvelarde May 16, 2018

idgserpro commented May 17, 2018

claytonc commented May 17, 2018

idgserpro commented May 18, 2018

hvelarde commented May 18, 2018 •

edited

Loading

idgserpro commented May 24, 2018

Altera reg javascript do c.upload / c.p.calendar. #474

Altera reg javascript do c.upload / c.p.calendar. #474

Conversation

idgserpro commented May 15, 2018

Boas práticas para o PR: Checklist

Foi aberta uma issue relativa a esse PR;

Foram adicionados testes unitários;

Foram adicionados testes robots;

Foram adicionados upgradeSteps;

PR não contém assuntos diferentes. PR's devem ter um objetivo claro para facilitar o review. Ex: não junte, num mesmo PR, alterações de code-analysis e de implementação. Faça um PR de code-analysis e depois o da implementação em si;

Evite muitos commits pequenos de um mesmo assunto no PR, sempre que possível efetue rebase;

hvelarde left a comment

Choose a reason for hiding this comment

idgserpro commented May 16, 2018

hvelarde left a comment

Choose a reason for hiding this comment

hvelarde May 16, 2018

Choose a reason for hiding this comment

idgserpro May 17, 2018

Choose a reason for hiding this comment

hvelarde May 17, 2018

Choose a reason for hiding this comment

hvelarde May 16, 2018

Choose a reason for hiding this comment

hvelarde May 16, 2018

Choose a reason for hiding this comment

hvelarde May 16, 2018

Choose a reason for hiding this comment

hvelarde May 16, 2018

Choose a reason for hiding this comment

idgserpro commented May 17, 2018

claytonc commented May 17, 2018

idgserpro commented May 18, 2018

hvelarde commented May 18, 2018 • edited Loading

idgserpro commented May 24, 2018

hvelarde commented May 18, 2018 •

edited

Loading