search

gitenberg-dev / giten_site

django repo for running the GITenberg website
http://www.gitenberg.org
40 stars 5 forks source link

increase maximum length of cover file names #76

Closed bdr99 closed 6 years ago

bdr99 commented 6 years ago

Some repos such as this one, have very long repo names. The file name being generated for the cover image was 115 characters in length, but the FileField was using the default maximum length which was 100 characters. This PR increases the maximum to 500 characters. It includes a database migration which needs to be applied after the code is deployed. @eshellman Can you please do this when you get a chance?

eshellman commented 6 years ago
  1. migrations are automatically done on deploy
  2. why not just use a hash for the file name?
bdr99 commented 6 years ago
  1. Ok, that's good.
  2. The reason that I didn't do this is because I wanted the new covers to overwrite the old ones when a cover is changed. Otherwise we would have old covers accumulating in Amazon S3. However if you think it should be a hash then I can change it. Let me know what you prefer.
eshellman commented 6 years ago

you could delete the old cover, and just use the gutenberg id with the original file name, if it exists

eshellman commented 6 years ago

because maybe don't depend on the title staying the same to overwrite the cover file?

bdr99 commented 6 years ago

Ok, good point. I'll change it to use the ID as the file name. That way we won't even need the database migration.

bdr99 commented 6 years ago

Ok, the gutenberg id is now used as the file name. I have removed the database migration and model changes from this PR since they are no longer necessary. I also made a few more improvements to the logic in this part of the code.

bdr99 commented 6 years ago

The tests are currently failing because of another XML syntax error in the Standard Ebooks OPDS catalog. It is not related to the changes in this PR and is something Standard Ebooks needs to resolve on their end.

eshellman commented 6 years ago

are you using a strict xml parser?

eshellman commented 6 years ago

maybe mock or skip the test?

just add the decorator @unittest.skip("SE xml issue") to the test

bdr99 commented 6 years ago

I couldn't use the decorator because the tests are all in the same method. I couldn't break them into separate methods because they all rely on the same book object. So I just commented out the Standard Ebooks part of the test. It can be uncommented when the XML issue is resolved.

eshellman commented 6 years ago

will push this to production so as not the delay you.

haven't looked at the code, but maybe the setup method of testcase could create the book object

eshellman commented 6 years ago

deployed