Skip to content

Asap 171 make document import more flexible - #235

Merged
lkacenja merged 7 commits into
devfrom
asap-171-make-document-import-more-flexible
Aug 5, 2025
Merged

Asap 171 make document import more flexible#235
lkacenja merged 7 commits into
devfrom
asap-171-make-document-import-more-flexible

Conversation

@lkacenja

@lkacenja lkacenja commented Jul 22, 2025

Copy link
Copy Markdown
Contributor

As we get ready to onboard more stakeholders, they will need to be able to import their own documents. Ideally our import process would be more flexible and not directly coupled to site creation.

This PR simplifies the document bootstrap command and adds a new document_import command. This command requires a site id and allows files in a zip archive or elsewhere. I also cleaned up the models to make them more consistent and added the complexity calculation to the document model. This makes one less rake task for stakeholders to run.

  • What additional steps are required to test this branch locally?

Do a rails db:drop ; rails db:migrate; rails db:setup and make sure bootstrap still works for you. Create a fresh empty site and run a few variants of the document_import command.

bin/rake documents:import_documents["<existing site id>", "<path to csv>", <bool whether file is inside archive>]
  • Are there any areas you would like extra review?

The rake commands and complexity calculation.

  • Are there any rake tasks to run on production?

No

@lkacenja lkacenja self-assigned this Jul 22, 2025
@lkacenja
lkacenja changed the base branch from main to dev July 22, 2025 22:06
Comment thread app/models/document.rb
@@ -1,11 +1,54 @@
class Document < ApplicationRecord
DEFAULT_DECISION = "Needs Decision".freeze

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved constants to the top of the model classes. I think this is a fairly standard pattern.

Comment thread app/models/document.rb

has_many :document_inferences

before_save :set_complexity

@lkacenja lkacenja Jul 22, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before the model is saved, we will now calculate complexity. This means if the underlying field values change, the calculation does as well. I think that is ok.

Comment thread lib/tasks/documents.rake
location: "San Rafael, CA",
primary_url: "https://www.cityofsanrafael.org/"
)
puts "Created site: #{san_rafael.name}"

@lkacenja lkacenja Jul 22, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since these sites are already created in production, I wasn't sure if we needed this anymore. It would be nice not to have to maintain a list of sites and/or mapping to files. If we think keeping all the current sites we load into our development environment is useful, I can bring them back.

Comment thread lib/tasks/documents.rake
(document.number_of_images == 0)) ? Document::SIMPLE_STATUS : Document::COMPLEX_STATUS
document.complexity = complexity
document.save
PaperTrail.request(enabled: false) do

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I simplified this rake task, which could still be useful.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what this rake task does anymore? Logically, it seems to be looping over documents where complexity isn't assigned, and then if information on number of tables or images is available, we save the document without any changes?

@lkacenja lkacenja Aug 4, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Complexity calculation happens every time a document is saved as of this PR. In essence this rake task should be calculating complexity for any documents that are missing it by saving them.

@lkacenja
lkacenja requested a review from allisonmorgan July 28, 2025 13:56
@lkacenja
lkacenja marked this pull request as ready for review July 28, 2025 21:53

@allisonmorgan allisonmorgan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new rake task worked perfectly for me! Had two clarifying questions, but nothing is blocking.

I didn't do a super deep dive on how many simple / complex documents there are before and after this change, but looking at the SLC sample on this branch and the full SLC database on production it looks consistent. Great job!

Comment thread lib/tasks/documents.rake
(document.number_of_images == 0)) ? Document::SIMPLE_STATUS : Document::COMPLEX_STATUS
document.complexity = complexity
document.save
PaperTrail.request(enabled: false) do

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what this rake task does anymore? Logically, it seems to be looping over documents where complexity isn't assigned, and then if information on number of tables or images is available, we save the document without any changes?

Comment thread app/models/document.rb Outdated
self.complexity = calculate_complexity
end

def calculate_complexity

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just of a question: what's the benefit of having two separate functions set_complexity & calculate_complexity vs one that does both? Not a blocker.

@lkacenja lkacenja Aug 4, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great call out. I think I must have not noticed how duplicative they became. I changed it to one simpler function in 24f1178. Thanks!

@lkacenja
lkacenja merged commit 9e86d2d into dev Aug 5, 2025
2 checks passed
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.

2 participants