Populate user subjects based on documents #634

frankete333 · 2025-01-22T01:12:25Z

https://github.com/user-attachments/assets/da254887-94c4-45b2-8672-0a44a5f3c73d

app/controllers/academic_histories_controller.rb

app/models/base_student.rb

RenzoMinelli · 2025-01-22T13:14:58Z

app/controllers/academic_histories_controller.rb

+  end
+
+  def index
+    redirect_to new_academic_history_path


Por que definis el index si el boton en el menu lleva al new? Se podria sacar?

lib/academic_history/pdf_processor.rb

RenzoMinelli · 2025-02-14T01:16:06Z

lib/academic_history/pdf_processor.rb

+  class PdfProcessor
+    include Enumerable
+
+    AcademicEntry = Struct.new(:name, :credits, :number_of_failures, :date_of_completion, :grade) do


Creo que deberias mover esto a una clase AcademicHistory::AcademicEntry luego en esa clase podes definir un metodo como save_academic_entry_to_user(user) y moves el codigo que dejaste en el controller para ahi.

Además, asi podemos hacer test sobre esa clase.

config/routes.rb

app/controllers/academic_histories_controller.rb

lib/academic_history/pdf_processor.rb

EmilioCristalli · 2025-04-03T13:34:08Z

lib/academic_history/pdf_processor.rb

+require 'pdf-reader'
+
+module AcademicHistory
+  class PdfProcessor


opiniones sobre renombrar esta clase a PdfParser y hacer otro PdfProcessor que llame al parser y después haga lo que está haciendo el controlelr de iterar y crear las materias aprobadas?

Muy de acuerdo

lib/academic_history/pdf_processor.rb

app/views/transcripts/new.html.erb

spec/lib/transcript/pdf_parser_spec.rb

lib/transcript/academic_entry.rb

Co-authored-by: Renzo Minelli <[email protected]>

config/routes.rb

RenzoMinelli · 2025-05-01T01:36:06Z

app/models/base_student.rb

@@ -11,6 +11,12 @@ def add(approvable)
    end
  end

+  def force_add_subject(subject)


Capaz deberias agregar un test de modelo para esto

RenzoMinelli · 2025-05-01T01:36:45Z

lib/transcript/academic_entry.rb

+      grade != '***'
+    end
+
+    def save_to_user(user)


Para mi deberias poner un test para este metodo

Co-authored-by: Renzo Minelli <[email protected]>

EmilioCristalli · 2025-05-06T23:18:35Z

config/routes.rb

@@ -30,6 +30,8 @@

  resources :current_optional_subjects, only: :index

+  resources :transcripts, only: [:new, :create]


debería ser un resource singular esto?

EmilioCristalli · 2025-05-06T23:27:27Z

lib/transcript/pdf_parser.rb

+require 'pdf-reader'
+require_relative 'academic_entry'


son necesarios estos require?

EmilioCristalli · 2025-05-06T23:32:06Z

lib/transcript/academic_entry.rb

@@ -0,0 +1,39 @@
+module Transcript


por qué ponerlo en lib y no en app/services o algo del estilo?

EmilioCristalli · 2025-05-06T23:33:07Z

lib/transcript/academic_entry.rb

+  class AcademicEntry
+    attr_accessor :name, :credits, :number_of_failures, :date_of_completion, :grade
+
+    def initialize(name: nil, credits: nil, number_of_failures: nil, date_of_completion: nil, grade: nil)


para mi habría que sacar los defaults, ya que siempre se le pasan todas las keys

EmilioCristalli · 2025-05-06T23:34:27Z

lib/transcript/academic_entry.rb

+    def save_to_user(student)
+      subject = subject_from_name(name)
+
+      return false if subject.blank?


return unless subject

siempre va a ser un subject o nil, no?

EmilioCristalli · 2025-05-06T23:42:56Z

lib/transcript/academic_entry.rb

+
+      return subject_match.first if subject_match.length == 1
+
+      active_subjects = subject_match.select { |subject| !subject.inactive? }


capaz queda mejor subject_match.reject(&:inactive?)?

EmilioCristalli · 2025-05-06T23:44:53Z

app/models/base_student.rb

+    ids << subject.exam.id if subject.exam && !ids.include?(subject.exam.id)
+    ids << subject.course.id if subject.course && !ids.include?(subject.course.id)


if subject.exam && ids.exclude?(subject.exam.id)

EmilioCristalli · 2025-05-06T23:48:40Z

lib/transcript/academic_entry.rb

@@ -0,0 +1,39 @@
+module Transcript
+  class AcademicEntry
+    attr_accessor :name, :credits, :number_of_failures, :date_of_completion, :grade


todos estos atributos tienen que ser públicos?
o cuáles se usan desde afuera?

EmilioCristalli · 2025-05-06T23:57:19Z

lib/transcript/academic_entry.rb

+      @number_of_failures = number_of_failures
+      @date_of_completion = date_of_completion


se usan para algo estos atributos?

EmilioCristalli · 2025-05-06T23:59:07Z

app/views/transcripts/new.html.erb

+<div>
+  <% if @successful_subjects.present? %>
+    <p class="text-green-600 mb-2"><%= @successful_subjects.length %> Materias marcadas correctamente</p>
+    <% @successful_subjects.each do |subject| %>
+      <p class="text-gray-700"><%= subject.code %> - <%= subject.name %></p>
+    <% end %>
+  <% end %>
+  <% if @failed_entries.present? %>
+    <p class="text-red-600 my-2"><%= @failed_entries.length %> Materias no encontradas</p>
+    <% @failed_entries.each do |entry| %>
+      <p class="text-gray-700"><%= entry.name %></p>
+    <% end %>
+  <% end %>
+</div>


para mi esto debería ser otra página, o mostrar como un flash message

Para mi está bien como primera versión, pero yo intentaría ahora pensar en cómo mejorarlo, sí. Yo había pensado en un modal o algo así también, aunque una página aparte no me parece mal tampoco.

Agregué un ticket para eso! https://github.com/orgs/cedarcode/projects/3?pane=issue&itemId=109539243

frankete333 added 6 commits December 24, 2024 20:38

Add pdf reader gem

2be7baa

Add new styles

6d018fa

Add force_add

9144e46

Add new routes and button

627d6a5

Add controller and view

4badc00

Merge branch 'master' into populate-user-subjects-based-on-documents

a0ff0de

RenzoMinelli reviewed Jan 22, 2025

View reviewed changes

frankete333 added 3 commits February 13, 2025 19:20

Move pdf parsing logic to lib

2cb0d5a

Add pdf processor test

35f51c0

Rubocop fixes

ebc0c71

frankete333 marked this pull request as ready for review February 13, 2025 23:18

frankete333 requested review from EmilioCristalli, RenzoMinelli and santiagoguridi February 13, 2025 23:25

Change process to be an enumerator

86aadbf

RenzoMinelli reviewed Feb 14, 2025

View reviewed changes

lib/academic_history/pdf_processor.rb Outdated Show resolved Hide resolved

frankete333 added 4 commits February 13, 2025 21:05

Remove scss

ba09deb

Make PdfProcessor an enum

1439e3d

Merge branch 'master' into populate-user-subjects-based-on-documents

6177348

Fix lint

f49b2fc

RenzoMinelli reviewed Feb 14, 2025

View reviewed changes

EmilioCristalli reviewed Apr 3, 2025

View reviewed changes

frankete333 and others added 8 commits April 30, 2025 19:44

Merge branch 'master' into populate-user-subjects-based-on-documents

31ccea6

Small fixes

58fed35

Remove unnecesary css

f7b6f3a

Rename academic history to transcript

1fc25d0

Split Academic Entry

460d5f4

renombre entries a subject y separar codigo

d182e5f

Use tailwind classes

6a446a0

Update comment

5678d4c

frankete333 added 3 commits April 30, 2025 21:55

Use tailwind css

b13108d

Fix tests

d8a9b53

Push fix lint

eda1f34

RenzoMinelli reviewed May 1, 2025

View reviewed changes

app/views/transcripts/new.html.erb Outdated Show resolved Hide resolved

spec/lib/transcript/pdf_parser_spec.rb Outdated Show resolved Hide resolved

lib/transcript/academic_entry.rb Show resolved Hide resolved

frankete333 and others added 4 commits April 30, 2025 22:27

Use factory

2ae9520

Update app/views/transcripts/new.html.erb

fc281b3

Co-authored-by: Renzo Minelli <[email protected]>

Make attribs read only

62027bd

Fix lint

6c432dd

RenzoMinelli reviewed May 1, 2025

View reviewed changes

frankete333 and others added 5 commits April 30, 2025 22:39

Update config/routes.rb

1f0dbfd

Co-authored-by: Renzo Minelli <[email protected]>

Fix tests

9805f47

Merge branch 'master' into populate-user-subjects-based-on-documents

fe451e9

Add tests

aa6a534

sacar linea innecesaria

00afd9b

RenzoMinelli approved these changes May 1, 2025

View reviewed changes

frankete333 merged commit de2dda7 into master May 1, 2025
5 checks passed

frankete333 deleted the populate-user-subjects-based-on-documents branch May 1, 2025 03:06

EmilioCristalli reviewed May 6, 2025

View reviewed changes

santiagorodriguez96 mentioned this pull request May 14, 2025

Mostrar resultado de importación de materias como un modal #735

Merged

		@@ -30,6 +30,8 @@

		resources :current_optional_subjects, only: :index

		resources :transcripts, only: [:new, :create]


		return subject_match.first if subject_match.length == 1

		active_subjects = subject_match.select { \|subject\| !subject.inactive? }

		ids << subject.exam.id if subject.exam && !ids.include?(subject.exam.id)
		ids << subject.course.id if subject.course && !ids.include?(subject.course.id)

		@number_of_failures = number_of_failures
		@date_of_completion = date_of_completion

		require 'pdf-reader'
		require_relative 'academic_entry'

Populate user subjects based on documents #634

Populate user subjects based on documents #634

Uh oh!

Conversation

frankete333 commented Jan 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

Uh oh!

frankete333 commented Jan 22, 2025 •

edited

Loading