Remove records in Airatable for test #42

yusuke-sforzando · 2020-12-09T09:38:46Z

Describe the PR

To close #25 .
Added the ability to remove items registered in the test.

Screenshots

Detail of the change

airtable_client.py

Create delete_asset method:

def delete_asset_by_title(self, value: str):
    """Delete Airtable asset.

    Deletes the equipment registered in the Airtable with the value of the title field as an argument.

    Args:
        value (str): The values corresponding to the title-field in Airtable.

    Returns:
        Airtable API response (dict): Dictionary of successful or unsuccessful deletions and IDs of items.
    """
       
        ....

test_main.py, test_airtable_client.py

Add the code to remove the registered items.

Anticipated impacts

None.

To reproduce

Steps to check the behavior:

Visit to Airtable
Check the initial state of the table items
Prepare Python environment
python3 -m venv venv
source venv/bin/activate
pytest -vv .
Check Airtable and make sure that the item is registered and deleted, as shown in the screenshot

Additional context

None.

shin-sforzando · 2020-12-09T10:00:21Z

なぜ動名詞？

yusuke-sforzando · 2020-12-09T11:46:01Z

失礼しました。修正しました。

codecov · 2020-12-09T11:46:53Z

Codecov Report

Merging #42 (7ef326f) into main (401930d) will increase coverage by 0.11%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main      #42      +/-   ##
==========================================
+ Coverage   79.88%   80.00%   +0.11%     
==========================================
  Files           5        5              
  Lines         169      175       +6     
  Branches       19       19              
==========================================
+ Hits          135      140       +5     
  Misses         26       26              
- Partials        8        9       +1

Impacted Files	Coverage Δ
airtable_client.py	`87.50% <100.00%> (+4.16%)`	⬆️
main.py	`75.71% <0.00%> (-1.43%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 401930d...7ef326f. Read the comment docs.

tomoya-sforzando

細かいですが、修正をお願いします。

tomoya-sforzando · 2020-12-09T14:22:01Z

tests/test_airtable_client.py

@@ -43,10 +43,18 @@ def test_register(airtable_client):
    """Testing whether a dictionary with the proper field names can be registered correctly."""

    assert airtable_client.register_asset(registerable_asset)
+    assert airtable_client.delete_asset("title", registerable_asset.title)


このテストの名前だけど、test_register_asset_and_delete_asset_success()とか、対応しているメソッドを使ってちゃんとした方が良い

なるほどですね。承知しました。修正します。

tests/test_airtable_client.py

tests/test_main.py

tomoya-sforzando · 2020-12-09T14:31:27Z

airtable_client.py

+        """
+
+        try:
+            return self.airtable_client.delete_by_field(field_name, field_value)


これって何が返ってくるのか。Docstringに追記してほしいです

失礼しました。追記しておきます。

tomoya-sforzando · 2020-12-09T14:33:31Z

tests/test_airtable_client.py

@@ -43,10 +43,18 @@ def test_register(airtable_client):
    """Testing whether a dictionary with the proper field names can be registered correctly."""

    assert airtable_client.register_asset(registerable_asset)
+    assert airtable_client.delete_asset("title", registerable_asset.title)


 def test_register_non_existent_key(airtable_client):
    """Testing an instance of the Airtable data class is an argument."""


この英文、どういう意味？

Airtableデータクラスが引数になっているかのテスト、という意味でしたが間違えているので修正します。

Co-authored-by: Tomoya KASHIMADA <[email protected]>

tomoya-sforzando

LGTM.

shin-sforzando · 2020-12-10T06:28:57Z

airtable_client.py

@@ -36,3 +36,22 @@ def register_asset(self, asset: Asset):
        except TypeError as te:
            app.logger.error(te)
            raise te
+
+    def delete_asset(self, field_name: str, field_value: str):


このメソッドの存在意義がわからない。
引数でもらったものをそのまま self.airtable_client.delete_by_field(field_name, field_value) するだけなら、要らないじゃん。
そもそも Airtable field names. にどんなものがあるか覚えてないと使えないのも意味がわからない。
これなら、 delete_asset_by_title(self, value: str) -> Dict: で return self.airtable_client.delete_by_field("title", value) したほうがいいじゃん。
なんで共同開発者に無駄な負担かけさせるの？

そうですね。
Airtableのfield name を意識させずに書くべきでした。
titleはメソッドに持たせます。
無駄な負担をかけてしまいました。
修正します。

shin-sforzando · 2020-12-10T06:30:55Z

tests/test_airtable_client.py

    """Testing whether a dictionary with the proper field names can be registered correctly."""

    assert airtable_client.register_asset(registerable_asset)
+    assert airtable_client.delete_asset("title", registerable_asset.title)


テストで追加したいものを消したいんだよね？
テストで追加したものと同名のものを消したいんじゃないよね。
なんで airtable_client.register_asset() の返り値からID拾って airtable_client.delete(id) しないの？
そっちの方が楽だし素直じゃん。

そうですね。
こちらもその通りです。
こちらはIDで直接消せるように修正します。

tomoya-sforzando · 2020-12-11T08:28:56Z

tests/test_airtable_client.py

-    assert airtable_client.register_asset(registerable_asset)
+    register_asset = airtable_client.register_asset(registerable_asset)
+    assert register_asset
+    assert airtable_client.airtable_client.delete(register_asset["id"])


delete できたことも assert する？悪いことはないか。

あ、わかった。
このテストの名前は、
test_register_asset_successだけでよくて、
test_delete_asset_by_title_successというテストが別に必要なんじゃない？

わかりました。ちょっとやってみます。

でもここは本当はIDで消したいですよね。
タイトルで削除、というのは仮に今後PlayStation 5 (CFI-1000A01)を登録したいときに困っちゃう可能性がありますね。
やっぱりIDで消したい気もします。

とりあえずは修正しました。

tomoya-sforzando · 2020-12-11T08:30:07Z

tests/test_airtable_client.py


    with pytest.raises(TypeError):
        airtable_client.register_asset({"test": "test"})
+
+
+def test_delete_asset_failure(airtable_client):


で、こちらのテストの名前が、test_delete_asset_by_title_failureになって対になるじゃない？

そうですね、やってみます。

shin-sforzando

ダブルクォートとシングルクォートの混用もいいかげん修正してください。

shin-sforzando · 2020-12-11T16:29:07Z

tests/test_main.py

@@ -87,6 +88,7 @@ def test_POST_register_airtable_success(test_client):
    test_client.post("/registration", data={"asin": "B07XB5WX89"})


たとえば、このファイル中に B07XB5WX89 が3回も書かれている、つまりDRYじゃない。

shin-sforzando · 2020-12-11T16:33:21Z

tests/test_main.py

@@ -87,6 +88,7 @@ def test_POST_register_airtable_success(test_client):
    test_client.post("/registration", data={"asin": "B07XB5WX89"})
    response = test_client.post("/register_airtable", data=imd, follow_redirects=True)
    assert b"Registration completed!" in response.data
+    AirtableClient().delete_asset_by_title("テンマクデザイン サーカス TC DX")


ん？だからなんで登録したテストデータのIDで消さないの？

👍 Add delete_asset method on airtable_client

8e4601c

yusuke-sforzando added the enhancement New feature or request label Dec 9, 2020

yusuke-sforzando added this to the Version 1.1 milestone Dec 9, 2020

yusuke-sforzando requested a review from tomoya-sforzando December 9, 2020 09:38

yusuke-sforzando self-assigned this Dec 9, 2020

yusuke-sforzando changed the title ~~Removing records in Airatable for test~~ Remove records in Airatable for test Dec 9, 2020

👍 Remove the table with field names and values

e229a11

yusuke-sforzando added 2 commits December 9, 2020 20:57

✨ Add test_delete_asset()

bd2f137

🔥 Remove type error for airtable_cilent.py

343aa5e

yusuke-sforzando marked this pull request as ready for review December 9, 2020 12:26

yusuke-sforzando added 3 commits December 9, 2020 22:03

🔥 Remove test_delete_asset_success()

a714b69

👍 Changed the title name to be generic

765e642

👍 Changed the title name to be generic for test_airtable_client

80b50eb

tomoya-sforzando suggested changes Dec 9, 2020

View reviewed changes

yusuke-sforzando and others added 3 commits December 9, 2020 23:38

Update tests/test_airtable_client.py

5c346ce

Co-authored-by: Tomoya KASHIMADA <[email protected]>

Update tests/test_main.py

10c6e44

Co-authored-by: Tomoya KASHIMADA <[email protected]>

📝 Improve comment and test name

9915258

yusuke-sforzando requested a review from tomoya-sforzando December 9, 2020 15:06

tomoya-sforzando approved these changes Dec 9, 2020

View reviewed changes

yusuke-sforzando requested a review from shin-sforzando December 10, 2020 05:20

shin-sforzando suggested changes Dec 10, 2020

View reviewed changes

yusuke-sforzando marked this pull request as draft December 10, 2020 11:15

👍 Change delete_asset to delete_asset_by_title

111771b

yusuke-sforzando requested a review from shin-sforzando December 10, 2020 22:06

yusuke-sforzando marked this pull request as ready for review December 10, 2020 22:06

yusuke-sforzando requested a review from tomoya-sforzando December 11, 2020 08:11

tomoya-sforzando suggested changes Dec 11, 2020

View reviewed changes

🎨 Organized the order of the tests

7ef326f

tomoya-sforzando approved these changes Dec 11, 2020

View reviewed changes

shin-sforzando suggested changes Dec 11, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove records in Airatable for test #42

Remove records in Airatable for test #42

yusuke-sforzando commented Dec 9, 2020 •

edited

Loading

shin-sforzando commented Dec 9, 2020

yusuke-sforzando commented Dec 9, 2020

codecov bot commented Dec 9, 2020 •

edited

Loading

tomoya-sforzando left a comment

tomoya-sforzando Dec 9, 2020

yusuke-sforzando Dec 9, 2020

tomoya-sforzando Dec 9, 2020

yusuke-sforzando Dec 9, 2020

tomoya-sforzando Dec 9, 2020

yusuke-sforzando Dec 9, 2020

tomoya-sforzando left a comment

shin-sforzando Dec 10, 2020

yusuke-sforzando Dec 10, 2020

shin-sforzando Dec 10, 2020

yusuke-sforzando Dec 10, 2020

tomoya-sforzando Dec 11, 2020

yusuke-sforzando Dec 11, 2020

yusuke-sforzando Dec 11, 2020

yusuke-sforzando Dec 11, 2020

tomoya-sforzando Dec 11, 2020

yusuke-sforzando Dec 11, 2020

shin-sforzando left a comment

shin-sforzando Dec 11, 2020

shin-sforzando Dec 11, 2020

		@@ -87,6 +88,7 @@ def test_POST_register_airtable_success(test_client):
		test_client.post("/registration", data={"asin": "B07XB5WX89"})

Remove records in Airatable for test #42

Are you sure you want to change the base?

Remove records in Airatable for test #42

Conversation

yusuke-sforzando commented Dec 9, 2020 • edited Loading

Describe the PR

Screenshots

Detail of the change

airtable_client.py

test_main.py, test_airtable_client.py

Anticipated impacts

To reproduce

Additional context

shin-sforzando commented Dec 9, 2020

yusuke-sforzando commented Dec 9, 2020

codecov bot commented Dec 9, 2020 • edited Loading

Codecov Report

tomoya-sforzando left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomoya-sforzando left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shin-sforzando left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yusuke-sforzando commented Dec 9, 2020 •

edited

Loading

codecov bot commented Dec 9, 2020 •

edited

Loading