Closed thealphadollar closed 1 year ago
Hello @thealphadollar! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
There are currently no PEP 8 issues detected in this Pull Request. Cheers! :beers:
I am not sure if this would be safe to allow normal anonymous users to post links in our database. Posting pdfs also has some level of risk, but if you allow anonymous users to post random links, people will definitely misuse it. I think this power should be allowed only to the admin.
You can use the docker version of the app for testing if MySQL native is causing issues.
Using 2 times paper.save() was intentional actually.
My logic was that unless we save a paper to the db, we don't get a valid primary key.
Now if we try to add keywords to it (which does nothing but link a row of the keywords field to the primary key of the paper), it will not find a valid db record for the paper to do so. The query can fail thereby.
So as a safeguard I committed the paper to the db first, then did all the keyword addition. Then saving again so that these foreign key linkings get committed. I don't remember having specifically tested with just one paper.save() as you have done. But IMHO, it's better to be explicit than to be implicit ( import this
that is :smile: )
Thanks for adding the feature of deleting the file after GDrive upload. I kinda forgot to do that while implementing. :stuck_out_tongue:
I am not sure if this would be safe to allow normal anonymous users to post links in our database. Posting pdfs also has some level of risk, but if you allow anonymous users to post random links, people will definitely misuse it. I think this power should be allowed only to the admin.
What safety concerns? Are you implying spam? That would be similar in both ways, I believe.
Using 2 times paper.save() was intentional actually. My logic was that unless we save a paper to the db, we don't get a valid primary key. Now if we try to add keywords to it (which does nothing but link a row of the keywords field to the primary key of the paper), it will not find a valid db record for the paper to do so. The query can fail thereby. So as a safeguard I committed the paper to the db first, then did all the keyword addition. Then saving again so that these foreign key linkings get committed. I don't remember having specifically tested with just one paper.save() as you have done. But IMHO, it's better to be explicit than to be implicit (
import this
that is )
I don't quite understand this. I'm unable to find where have you referenced the paper in a way that required saving? The below statement, if I understand you correctly, will not work without saving the paper first, is that so? If yes, I wonder why since all you are doing is adding some values to an uncommitted paper which should behave the same as a committed paper unless you are doing a find query or using an attribute which is generated only once the paper is saved.
for key in keys_tmp:
paper.keywords.add(key)
@grapheo12 Can you please make me aware what needs to be done in this PR or are we closing it? I'm not up to date with the developments.
Let's keep it open. We will include it in the next version.
This PR, if merged, will implement the following changes:
os.remove(local_path)
happens in theupload_file
function since it makes more sense that as soon as a file is uploaded, it should be removed.NOTE: You can try to make this command
async
.paper.save()
and save only after adding keywords. (@grapheo12 Was two saves something you intended?)I believe earlier we decided that this should be an option available only to admin but I realized that it could happen sometimes that a user may find a paper on other website such as notehub etc. and would want to link them to mfqp. The same goes in case we are missing a file from library. So, I have made it available to admin and non-admin.
P.S. As with the last PR, I haven't set up a local copy of the application since I'm facing some issues with the database on my Ubuntu version (and I didn't have much time to inspect the issue). Please pardon the same, and test the changes before deploying.
fixes #9