Conversation
a better location
Currently beautiful soup is used to get headlines from articles
…f/kerckhoff-server into cliu/wordpress-integrations-tests
Before, the type hint for content was dict, even though it should actually be list
Currently only has tests that do not require kerckhoff packages, google drive, or Wordpress POST requests. These other tests will be added later
The return data provides data allowing posts to be deleted in tests
String concatenation creates too much unnecessary copying
- Add logger for kerckhoff module - Replace print statements with log statements - Remove unnecessary print statements
This reverts commit 4d59e92. Cover images should not be optional
- Log level - Don't capture unit test stdout - Add coverage reports to gitignore
DumboOctopus
left a comment
DumboOctopus
left a comment
There was a problem hiding this comment.
I forgot to submit this review a long time ago
| @@ -0,0 +1,168 @@ | |||
| import tempfile | |||
There was a problem hiding this comment.
Yes, it's the python tempfile package used in image uploading
There was a problem hiding this comment.
But it's not used in models.py anymore so I'll remove it
| MYSQL_DATABASE: wordpress | ||
| MYSQL_USER: wordpress | ||
| MYSQL_PASSWORD: wordpress | ||
| wordpress: |
There was a problem hiding this comment.
Do you use the wordpress docker container and/or do you think it would be ok to use the docker container instead of the fake mainsite
There was a problem hiding this comment.
I don't use the wordpress docker container. We added this a long time ago so I don't quite remember what exactly we were planning. I think I should be able to remove it
| @@ -0,0 +1,159 @@ | |||
| import tempfile | |||
There was a problem hiding this comment.
This file might have been created accidently
| self.detail = f"Google Drive is not yet configured for {package.slug}." | ||
|
|
||
| class PublishError(APIException): | ||
| status_code = 400 |
There was a problem hiding this comment.
Hmm I feel like it might be more of a 500 error since the server was unable to fulfill a valid request (400 means request was bad)
There was a problem hiding this comment.
I think it's hard to say, because some of them are due to invalid AML, and others are because the server failed to do something. I think I'll add an argument to specify the error code
There was a problem hiding this comment.
ohh I see. Yeah in that case I think an argument would be good
| if(file.file_name == "data.aml"): | ||
| json_data = file.data | ||
| if("jpg" in file.file_name or "jpeg" in file.file_name | ||
| or "png" in file.file_name): |
There was a problem hiding this comment.
would it be ok if we just checked if the file ended with jpg, jpeg or png? (there might be some built in python package that can do that for us). The reason is because this could possibly lead to us uploading files like "png_list.txt" to wordpress' media library and that might fail causing the whole publish operation to fail.
There was a problem hiding this comment.
Makes sense. I'll use the python os package for that. I don't remember if I only allow jpg, jpeg and png because DB only uses those file types, or if I simply forgot to include more. Do you know if kerckhoff should be able to support other image extensions as well?
There was a problem hiding this comment.
One thing they would want to have in the future is gifs sometimes. However, there some changes we need to make to aws uploading code to support gifs so for now we can leave it out
The old file was probably getting it from models.py, and it broke after I removed it
DumboOctopus
left a comment
DumboOctopus
left a comment
There was a problem hiding this comment.
Just the merge conflict but besides that good to go!
| for chunk in res.iter_content(chunk_size=1000): | ||
| f.write(chunk) | ||
| headers = self.basic_auth_header | ||
| headers["Content-Disposition"] = f"form-data; filename={file_name}" |
There was a problem hiding this comment.
This is a small thing but technically modifying headers will modify self.basic_auth_header.
2 participants
This pull request enables publishing to the Daily Bruin main site through WordPress REST API from Kerckhoff. Includes:
Change the .env file to specify the desired URL and credentials for publishing. Currently set to DB test site.