Admin actions #1593
Admin actions #1593
Conversation
|
@squirrelo which should we review next, this one or #1602 ? Thanks! |
|
This one. |
|
Thanks, please let us know when should we proceed. |
|
ready for review. |
| if access_error: | ||
| return access_error | ||
|
|
||
| try: |
There was a problem hiding this comment.
These 3 try/except don't make sense, can you change? Or perhaps I'm missing something?
There was a problem hiding this comment.
BTW they are also missing tests.
There was a problem hiding this comment.
can_be_submitted_to_ebi is a boolean that tells you if you can query the next attribute or not. Please, use an if statement rather than an exception. try/except should be used when something unexpected may happen, not as a substitute of an if:
can_be_submitted_to_ebi = pd.can_be_submitted_to_ebi
ebi_run_accessions = pd.ebi_run_accessions if can_be_submitted_to_ebi else None
squirrelo
force-pushed
the
admin-actions
branch
from
dc7196f to
3e3131a
Compare
|
Merge issues taken care of, so missing tests and docstrings should be in place now. |
| try: | ||
| can_be_submitted_to_ebi = pd.can_be_submitted_to_ebi | ||
| ebi_run_accessions = pd.ebi_run_accessions | ||
| except QiitaDBOperationNotPermittedError: |
There was a problem hiding this comment.
Can you add tests for the except?
|
A few more comments. |
| return access_error | ||
| user = User(str(user_id)) | ||
| # Set the approval to private if needs approval and admin | ||
| if all([qiita_config.require_approval, visibility == 'private', |
There was a problem hiding this comment.
The first three if statements have common checks. Please extract common checks and use nested ifs to avoid evaluating multiple times the same expression.
| fp = join(qiita_config.base_data_dir, 'uploads', | ||
| str(self.new_study.id), 'uploaded_file.txt') | ||
| base_dir = qdb.util.get_mountpoint('uploads')[0][1] | ||
| fp = join(base_dir, str(self.new_study.id), 'uploaded_file.txt') |
There was a problem hiding this comment.
I think it will simplify the handling of the tests if you don't use this test (note code duplication here and in tearDown).
My recommendation would be that you actually create a new file using mkstemp (note that you can indicate the directory in which you want it) and just remove it in the tearDown.
There was a problem hiding this comment.
There is no code duplication. One creates a file for the new study upload folder, the other replaces the file for the existing study.
There was a problem hiding this comment.
Does it means that the new file created for the new study is not deleted in the tearDown?
There was a problem hiding this comment.
@squirrelo ping about this one
|
done with the review. |
|
Comments addressed |
| return qdb.sql_connection.TRN.execute_fetchindex() | ||
|
|
||
|
|
||
| def get_visibilities(): |
|
Few comments |
|
@squirrelo ETA on addressing the comments here? |
|
All comments have been addressed |
| access_error = check_access(pd.study.id, user_id) | ||
| if access_error: | ||
| return access_error | ||
| user = User(str(user_id)) |
There was a problem hiding this comment.
out of curiosity - why the specific cast to str in here? it looks unnecessary as you've already said before that everything is passed as a string.
There was a problem hiding this comment.
Was casting explicitly to int for the other objects so I just followed the same pattern and did an explicit cast here as well.
There was a problem hiding this comment.
Well, the reasoning doesn't apply here but it is ok - no need to change.
|
Still some unresolved comments here. |
3 participants
Requires #1590
Adds the admin approvals buttons to the interface. Can now change the visibility of all the artifacts individually. Also adds the submit to EBI and VAMPS buttons when applicable.