Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use filename instead of full file path as the document title #251

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

selurvedu
Copy link
Contributor

@selurvedu selurvedu commented Jul 11, 2016

To be merged in conjunction with openprocurement/openprocurement.client.python#33.

Depends on #259.


This change is Reviewable

These may contain file-like objects instead of file paths,
so the argument names 'filepath' and 'path' are not quite accurate.
create_fake_doc() returns a tuple with 2 elements,
first is the full file path, second is the base name of the file.
All document titles are compared to flenames as well.
@selurvedu
Copy link
Contributor Author

Not tested yet.
Please review.
+(review wanted)
+@mykhaly +@leits

@mykhaly
Copy link
Contributor

mykhaly commented Jul 11, 2016

Далеко не готове.

  1. Потрібно допиляти отримання вмісту файлу, який ми завантажували.
    2)Проконтролювати, що переіменування ключів в словниках нічого не ламає (filepath -> document в Змінити документ в ставці ймовірно щось зламає)
    3)Я б перейменував filepath -> document_path, basename -> document_name, document -> document_name, filename -> document_name.
    4)Потребує живого обговорення.

Reviewed 11 of 11 files at r1.
Review status: all files reviewed at latest revision, 3 unresolved discussions.


op_robot_tests/tests_files/base_keywords.robot, line 78 [r1] (raw file):

  ${file_content_loaded}  ${file_name_loaded}=  Run as  ${viewer}  Отримати документ  ${TENDER['TENDER_UAID']}  ${USERS.users['${username}'].tender_data.data.documents[0].url}
  ${file_name_uploaded}=  Set variable  ${USERS.users['${tender_owner}']['documents']['filename']}
  ${document_content_uploaded}=  get_file_contents  ${file_name_uploaded}

Це не буде працювати :(


op_robot_tests/tests_files/cancelation.robot, line 140 [r1] (raw file):

  ...      ${TENDER['LOT_ID']}
  ...      ${cancellation_data['cancellation_reason']}
  ...      ${cancellation_data['filepath']}

Мало бути filename?


op_robot_tests/tests_files/initial_data.py, line 14 [r1] (raw file):

from op_faker import OP_Provider

from .local_time import get_now

А за яким параметром ти сортував?


Comments from Reviewable

@selurvedu
Copy link
Contributor Author

  1. Детальніше?
  2. filepath із bid_doc_upload, на відміну від upload_response, ніде не використовується, тому ні.
$ grep -ir 'bid_doc_upload'
aboveThreshold_keywords.robot:  ${docid}=  Get Variable Value  ${USERS.users['${username}'].bidresponses['bid_doc_upload']['upload_response'].data.id}
aboveThreshold_keywords.robot:  ${bid_doc_upload}=  Run As  ${username}  Завантажити документ в ставку  ${filepath}  ${TENDER['TENDER_UAID']}  ${doc_type}
aboveThreshold_keywords.robot:  Set To Dictionary  ${USERS.users['${username}'].bidresponses}  bid_doc_upload=${bid_doc_upload}
base_keywords.robot:  ${bid_doc_upload}=  Run As  ${username}  Завантажити документ в ставку  ${filepath}  ${TENDER['TENDER_UAID']}
base_keywords.robot:  Set To Dictionary  ${USERS.users['${username}'].bidresponses}  bid_doc_upload=${bid_doc_upload}
base_keywords.robot:  ${docid}=  Get Variable Value  ${USERS.users['${username}'].bidresponses['bid_doc_upload']['upload_response'].data.id}
$ grep -ir 'Змінити документ в ставці'                                                                            
brokers/openprocurement_client.robot:Змінити документ в ставці
base_keywords.robot:  ${bid_doc_modified}=  Run As  ${username}  Змінити документ в ставці  ${filepath}  ${docid}
$ grep -ir bid.doc.modified
aboveThreshold_keywords.robot:  ${bid_doc_modified}=  Run As  ${username}  Змінити документацію в ставці  ${privat_doc}  ${docid}
aboveThreshold_keywords.robot:  Set To Dictionary  ${USERS.users['${username}'].bidresponses}  bid_doc_modified=${bid_doc_modified}
base_keywords.robot:  ${bid_doc_modified}=  Run As  ${username}  Змінити документ в ставці  ${filepath}  ${docid}
base_keywords.robot:  Set To Dictionary  ${USERS.users['${username}'].bidresponses}  bid_doc_modified=${bid_doc_modified}
  1. Там може бути file-like object, а не filepath, тому назва змінної навмисно не є конкретною.

Review status: all files reviewed at latest revision, 3 unresolved discussions.


op_robot_tests/tests_files/base_keywords.robot, line 78 [r1] (raw file):

Previously, mykhaly (Yurii Mykhalchuk) wrote…

Це не буде працювати :(

Тут якийсь лютий брєд відбувається. Спочатку воно читало контент скачаного файлу, а потім з його імені брало шлях до того, який ми upload'или, читало його і порівнювало з ним. Треба перейменувати змінну в ківорді `Отримати документ` з `filename` на `filepath`, а тут – для наступного: `file_{name,content}_loaded` → `downloaded_file_{name,content}` те саме з `file_{name,content}_uploaded` → `local_file_{name,content}`

Крім того, треба поламати API в ще одному місці – очікувадти від Отримати документ не tuple, а лише однну змінну, в якій буде шлях до файлу або file-like object, а вже з нього читати файлнейм і контент буде інший ківорд. Це виключить необхідність брокерам читати файли з своїх ківордів.


op_robot_tests/tests_files/cancelation.robot, line 140 [r1] (raw file):

Previously, mykhaly (Yurii Mykhalchuk) wrote…

Мало бути filename?

Ні, чому? Щоб завантажити файл, треба передати повний шлях до нього.

op_robot_tests/tests_files/initial_data.py, line 14 [r1] (raw file):

Imports are always put at the top of the file, just after any module comments and docstrings, and before module globals and constants.

Imports should be grouped in the following order:

standard library imports
related third party imports
local application/library specific imports

You should put a blank line between each group of imports.

https://www.python.org/dev/peps/pep-0008/#imports


Comments from Reviewable

@selurvedu
Copy link
Contributor Author

Depends on #259.

@openprocurement-ci
Copy link

My first comment.

@leits leits unassigned leits and mykhaly Apr 27, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants