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

Issue #448 #497

Open
wants to merge 35 commits into
base: main
Choose a base branch
from
Open

Issue #448 #497

wants to merge 35 commits into from

Conversation

IgorMonardez
Copy link
Contributor

No description provided.

# Conflicts:
#	db/schema.rb
# Conflicts:
#	Gemfile
#	Gemfile.lock
#	db/schema.rb
@IgorMonardez IgorMonardez changed the title Issue 448 Issue #448 Sep 2, 2024
@IgorMonardez IgorMonardez linked an issue Sep 2, 2024 that may be closed by this pull request
# Conflicts:
#	Gemfile
app/helpers/enrollments_pdf_helper.rb Outdated Show resolved Hide resolved
app/models/affiliation.rb Outdated Show resolved Hide resolved
app/models/affiliation.rb Outdated Show resolved Hide resolved
app/models/program_level.rb Outdated Show resolved Hide resolved
app/controllers/concerns/shared_pdf_concern.rb Outdated Show resolved Hide resolved
app/helpers/enrollments_pdf_helper.rb Outdated Show resolved Hide resolved
app/models/affiliation.rb Outdated Show resolved Hide resolved
db/migrate/20240524135850_create_affiliation.rb Outdated Show resolved Hide resolved
@IgorMonardez IgorMonardez changed the title Issue #448 WIP: Issue #448 Sep 2, 2024
…rogram level" model scopes, changed tests and application of scopes in pdf and view
@IgorMonardez IgorMonardez changed the title WIP: Issue #448 Issue #448 Sep 2, 2024
@JoaoFelipe
Copy link
Contributor

Mais alguns comentários.

Começando com perguntas que acho que @leomurta pode ajudar a dar opinião:

  1. O que acontece se nenhum program_level estiver cadastrado no momento da defesa? Acho que antes da mudança aparecia o nível do programa em branco. Agora deve dar uma exceção sem indicar exatamente o que foi. Qual será que é o comportamento desejado?

  2. Será que o fato do professor ter sido cadastro com afiliação deveria restringir a remoção do mesmo? Acho que faz sentido restringir nos outros campos (por exemplo, se o professor já deu aula, não deveria ser possível simplesmente remover o registro do sapos), mas não vejo muito sentido no restrict_with_exception para esse caso.

Além disso, tenho algumas sugestões de mudanças:

  1. Quando institution_id de professor é nulo, a migração está criando uma afiliação com instituição nula. Faria mais sentido não criar essa afiliação. Verificar se o mesmo não acontece com program_level

  2. O arquivo spec/features/custom_variables_spec.rb ainda tem a variável referente ao nível do programa (e usa isso em testes). A variável foi removida com a adição da tabela e o teste deveria refletir essa mudança.

  3. O arquivo seeds.rb também está criando program_level como CustomVariable. Altere para criar ProgramLevel

  4. Falta rota/active scaffold para alterar o nível do programa (e testes). Como não é mais custom variable, o local de customizar isso não existe mais e não parece ter nenhuma alternativa.

  5. Em "Professores" foi adicionado um link para "Afiliações":
    image

Acho que seria melhor não ter esse link e as afiliações aparecerem no olhinho.

@IgorMonardez IgorMonardez changed the title Issue #448 WIP: Issue #448 Sep 3, 2024
@IgorMonardez
Copy link
Contributor Author

Mais alguns comentários.

Começando com perguntas que acho que @leomurta pode ajudar a dar opinião:

  1. O que acontece se nenhum program_level estiver cadastrado no momento da defesa? Acho que antes da mudança aparecia o nível do programa em branco. Agora deve dar uma exceção sem indicar exatamente o que foi. Qual será que é o comportamento desejado?
  2. Será que o fato do professor ter sido cadastro com afiliação deveria restringir a remoção do mesmo? Acho que faz sentido restringir nos outros campos (por exemplo, se o professor já deu aula, não deveria ser possível simplesmente remover o registro do sapos), mas não vejo muito sentido no restrict_with_exception para esse caso.

Além disso, tenho algumas sugestões de mudanças:

  1. Quando institution_id de professor é nulo, a migração está criando uma afiliação com instituição nula. Faria mais sentido não criar essa afiliação. Verificar se o mesmo não acontece com program_level
  2. O arquivo spec/features/custom_variables_spec.rb ainda tem a variável referente ao nível do programa (e usa isso em testes). A variável foi removida com a adição da tabela e o teste deveria refletir essa mudança.
  3. O arquivo seeds.rb também está criando program_level como CustomVariable. Altere para criar ProgramLevel
  4. Falta rota/active scaffold para alterar o nível do programa (e testes). Como não é mais custom variable, o local de customizar isso não existe mais e não parece ter nenhuma alternativa.
  5. Em "Professores" foi adicionado um link para "Afiliações":
    image

Acho que seria melhor não ter esse link e as afiliações aparecerem no olhinho.

A rota para o program level estaria na configuration?

@leomurta
Copy link
Member

leomurta commented Sep 4, 2024

  1. O que acontece se nenhum program_level estiver cadastrado no momento da defesa? Acho que antes da mudança aparecia o nível do programa em branco. Agora deve dar uma exceção sem indicar exatamente o que foi. Qual será que é o comportamento desejado?

Eu acho que o ideal seria manter o comportamento atual.

  1. Será que o fato do professor ter sido cadastro com afiliação deveria restringir a remoção do mesmo? Acho que faz sentido restringir nos outros campos (por exemplo, se o professor já deu aula, não deveria ser possível simplesmente remover o registro do sapos), mas não vejo muito sentido no restrict_with_exception para esse caso.

Eu acho que se ele participou de alguma banca com aquela afiliação, não pode ser removido, pois quando tirarmos o histórico do aluno, não conseguiremos saber quem foi a banca dele. Mas o mero fato de ter afiliações sem estar vinculado a bancas, orientações, etc, não me parece relevante para barrar a remoção. Aí seria um cascade, que remove tanto o prof. quanto as afiliações (mantendo as instituições, obviamente).

Além disso, tenho algumas sugestões de mudanças:

  1. Quando institution_id de professor é nulo, a migração está criando uma afiliação com instituição nula. Faria mais sentido não criar essa afiliação. Verificar se o mesmo não acontece com program_level

Concordo.

  1. O arquivo spec/features/custom_variables_spec.rb ainda tem a variável referente ao nível do programa (e usa isso em testes). A variável foi removida com a adição da tabela e o teste deveria refletir essa mudança.

Concordo. Tem que testar se o nível antes da migração (com a variável) é o mesmo depois da migração (com a tabela).

  1. O arquivo seeds.rb também está criando program_level como CustomVariable. Altere para criar ProgramLevel

Concordo.

  1. Falta rota/active scaffold para alterar o nível do programa (e testes). Como não é mais custom variable, o local de customizar isso não existe mais e não parece ter nenhuma alternativa.

Aqui não sei se entendi bem. Não teria uma tela de ProgramLevel em Configurações onde podemos criar/editar/remover ProgramLevels? Na verdade, acho que são dois conceitos distintos: um é com as opções de nível (3 a 7). Talvez o nome seria ProgramLevelType para ficar parecido com o que fazemos no resto do Sapos. Outro com a atribuição de um nível ao programa numa data.

  1. Em "Professores" foi adicionado um link para "Afiliações":
    image

Essa imagem não apareceu.

Acho que seria melhor não ter esse link e as afiliações aparecerem no olhinho.

Talvez na edição do professor seja o melhor local para editar as afiliações.

A rota para o program level estaria na configuration?

Rota = menu, né? Acho que sim.

Leo

@IgorMonardez
Copy link
Contributor Author

Aqui não sei se entendi bem. Não teria uma tela de ProgramLevel em Configurações onde podemos criar/editar/remover ProgramLevels? Na verdade, acho que são dois conceitos distintos: um é com as opções de nível (3 a 7). Talvez o nome seria ProgramLevelType para ficar parecido com o que fazemos no resto do Sapos. Outro com a atribuição de um nível ao programa numa data.

Não entendi, pode ter mais de um ProgramLevel que iriam de 3 a 7? Sobre o nome eu segui o mesmo que estava nos tipos das variáveis, acredito que não tenha por que mudar.

@IgorMonardez
Copy link
Contributor Author

Eu acho que o ideal seria manter o comportamento atual.

Qual seria o comportamento atual?

@leomurta
Copy link
Member

leomurta commented Sep 4, 2024

Aqui não sei se entendi bem. Não teria uma tela de ProgramLevel em Configurações onde podemos criar/editar/remover ProgramLevels? Na verdade, acho que são dois conceitos distintos: um é com as opções de nível (3 a 7). Talvez o nome seria ProgramLevelType para ficar parecido com o que fazemos no resto do Sapos. Outro com a atribuição de um nível ao programa numa data.

Não entendi, pode ter mais de um ProgramLevel que iriam de 3 a 7? Sobre o nome eu segui o mesmo que estava nos tipos das variáveis, acredito que não tenha por que mudar.

Hoje existe esses 5 níveis. Mas a CAPES pode decidir amanhã em trocar isso. O ideal seria isso ser flexível, como fazemos para vários outros dados, como tipo de matrícula, níveis, tipo de bolsa, etc.

@leomurta
Copy link
Member

leomurta commented Sep 4, 2024

Eu acho que o ideal seria manter o comportamento atual.

Qual seria o comportamento atual?

Tente fazer um teste com a versão 6.4.8 para identificar e documente aqui.

@JoaoFelipe
Copy link
Contributor

A rota para o program level estaria na configuration?

A rota em si seria /program_levels
Mas o link para ela em navigation.rb estaria em Configuration sim

Aqui não sei se entendi bem. Não teria uma tela de ProgramLevel em Configurações onde podemos criar/editar/remover ProgramLevels?

Sim, é necessário ter isso. Não tinha quando revisei o PR.

Na verdade, acho que são dois conceitos distintos: um é com as opções de nível (3 a 7). Talvez o nome seria ProgramLevelType ?para ficar parecido com o que fazemos no resto do Sapos. Outro com a atribuição de um nível ao programa numa data.

Acho que criar ProgramLevelType seria complicação demais a toa. Não vejo problema em ter só ProgramLevel com datas do nível e um campo numérico (ou mesmo texto, se quiser deixar bem flexível caso a CAPES resolva mudar os níveis para letras no futuro).

Se percebermos que ProgramLevel tem características a mais de acordo com nível, passa a fazer sentido ter o ProgramLevelType

@leomurta
Copy link
Member

leomurta commented Sep 4, 2024

Acho que criar ProgramLevelType seria complicação demais a toa. Não vejo problema em ter só ProgramLevel com datas do nível e um campo numérico (ou mesmo texto, se quiser deixar bem flexível caso a CAPES resolva mudar os níveis para letras no futuro).

Se percebermos que ProgramLevel tem características a mais de acordo com nível, passa a fazer sentido ter o ProgramLevelType

Beleza então. Talvez uma validação importante seja garantir que um nível não esteja com datas sobrepostas de validade, né? Deve ter uma validação parecida em bolsa.

…ration screen, removal of the affiliation nested in teacher, in addition to the end and start dates in the program level show, update and create.
@IgorMonardez IgorMonardez changed the title WIP: Issue #448 Issue #448 Sep 24, 2024
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.

Informações inconsistentes no histórico
3 participants