Espoo Importer #718

voneiden · 2023-09-25T12:37:13Z

Espoo importer is a rather generic LE importer. Handles various relations between objects etc. although in hindsight nobody has specified if this is necessary. Actually, at this point I still don't know what are the parameters that should be used for importing, so that's configurable via an environment variable..

https://helsinkisolutionoffice.atlassian.net/browse/LINK-1375

codecov-commenter · 2023-09-25T13:01:46Z

Codecov Report

Merging #718 (30795b1) into main (2a62f70) will increase coverage by 1.77%.
The diff coverage is 97.68%.

@@            Coverage Diff             @@
##             main     #718      +/-   ##
==========================================
+ Coverage   80.26%   82.03%   +1.77%     
==========================================
  Files         274      275       +1     
  Lines       21056    21090      +34     
==========================================
+ Hits        16901    17302     +401     
+ Misses       4155     3788     -367

Files	Coverage Δ
events/tests/conftest.py	`98.01% <ø> (-0.03%)`	⬇️
linkedevents/settings.py	`71.57% <100.00%> (+0.92%)`	⬆️
events/tests/importers/test_espoo.py	`99.45% <99.45%> (ø)`
events/importer/espoo.py	`96.22% <95.93%> (+96.22%)`	⬆️

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

events/importer/espoo.py

events/tests/importers/test_espoo.py

events/importer/espoo.py

events/tests/importers/test_espoo.py

events/importer/espoo.py

voneiden · 2023-10-02T08:53:22Z

OK, I believe I've addressed all the comments. Requesting re-review and the new changes are in separate commits for now.

danipran

Some of the earlier comments were not addressed.

events/importer/espoo.py

events/tests/importers/test_espoo.py

events/importer/espoo.py

tuomas777 · 2023-10-11T12:55:00Z

events/importer/espoo.py

-        places = Place.objects.filter(**filter_params).order_by(
-            "-data_source", "-n_events"
+        audiences_data = _get_origin_objs(
+            f"{settings.ESPOO_API_URL}v1/keyword/", missing_audience_ids


There are now three places where a URL is built from settings.ESPOO_API_URL, and this particular one is actually implemented a bit differently than the other two (f-string vs urljoin). From DRY viewpoint it would be better to have something like get_espoo_api_url(resource). Also, it would safer for the future, if the setting ESPOO_API_URL would work with and without a trailing /.

tuomas777 · 2023-10-11T12:56:58Z

events/importer/espoo.py

+    instances = []
+    instance_data_map = {}
+    for obj_data in origin_objs:
+        obj_data = dict(obj_data)


If this is here just to get a copy, I'm guessing it would be more robust to use deepcopy?

tuomas777 · 2023-10-11T13:00:44Z

linkedevents/settings.py

@@ -60,6 +60,15 @@ def get_git_revision_hash() -> str:
    ),
    ENABLE_EXTERNAL_USER_EVENTS=(bool, True),
    ENABLE_REGISTRATION_ENDPOINTS=(bool, False),
+    ESPOO_API_URL=(str, "https://api.espoo.fi/events/"),
+    ESPOO_API_QUERY_PARAMS=(


As these are only used with event resource queries, I would rather name this ESPOO_API_EVENT_QUERY_PARAMS (I actually first thought these were used in every query when I saw the setting 🙂)

Could not identify where it was needed anymore.

danipran

Some suggestions for the tests, otherwise LGTM 👍

danipran · 2023-10-17T05:55:31Z

events/tests/importers/test_espoo.py

+    assert [d["id"] for d in origin_data[:1]] == [i.id for i in common_objs]
+    assert origin_data[1:] == origin_objs


What are we asserting here? The first line especially is very cryptic 😅

danipran · 2023-10-17T06:13:09Z

events/tests/importers/test_espoo.py

+@pytest.fixture
+def sleep(monkeypatch):
+    class Sleep:
+        call_count = 0
+
+        def sleep(self, t):
+            self.call_count += 1
+
+    sleep_instance = Sleep()
+
+    monkeypatch.setattr("time.sleep", sleep_instance.sleep)
+    return sleep_instance


Could use Mock instead:

Suggested change

@pytest.fixture

def sleep(monkeypatch):

class Sleep:

call_count = 0

def sleep(self, t):

self.call_count += 1

sleep_instance = Sleep()

monkeypatch.setattr("time.sleep", sleep_instance.sleep)

return sleep_instance

@pytest.fixture

def sleep(monkeypatch):

mock_sleep = Mock() # or a MagicMock

monkeypatch.setattr("time.sleep", mock_sleep)

return mock_sleep

Or if you don't like mixing & matching:

Suggested change

@pytest.fixture

def sleep(monkeypatch):

class Sleep:

call_count = 0

def sleep(self, t):

self.call_count += 1

sleep_instance = Sleep()

monkeypatch.setattr("time.sleep", sleep_instance.sleep)

return sleep_instance

@pytest.fixture

def sleep():

with patch("time.sleep") as mock: # not completely sure on the target; maybe "events.importer.sleep" if this one doesn't work?

yield mock

danipran · 2023-10-17T06:14:24Z

events/tests/importers/test_espoo.py

+
+
+@pytest.fixture
+def sleep(monkeypatch):


I'd prefer the name mock_sleep, but up to you.

Suggested change

def sleep(monkeypatch):

def mock_sleep(monkeypatch):

A generic LE importer that handles relations and imports Organizations, Places, Keywords and Events under a DataSource. Unreferenced relations are removed using ModelSyncher. Co-authored-by: Tuomas Haapala <[email protected]> Co-authored-by: Daniel Prange <[email protected]>

sonarqubecloud · 2023-10-18T07:37:23Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

96.0% Coverage
0.0% Duplication

voneiden force-pushed the link-1375-espoo branch from 647b21d to 4ea1d33 Compare September 25, 2023 12:54

voneiden force-pushed the link-1375-espoo branch 2 times, most recently from 925881e to b4279bf Compare September 26, 2023 04:38

voneiden marked this pull request as ready for review September 26, 2023 04:47

voneiden requested a review from a team September 26, 2023 04:48

tuomas777 reviewed Sep 27, 2023

View reviewed changes

danipran requested changes Sep 28, 2023

View reviewed changes

tuomas777 reviewed Sep 29, 2023

View reviewed changes

events/importer/espoo.py Outdated Show resolved Hide resolved

voneiden force-pushed the link-1375-espoo branch from b1e046d to cbfac1a Compare October 2, 2023 08:51

voneiden requested review from tuomas777 and danipran October 2, 2023 08:53

danipran requested changes Oct 2, 2023

View reviewed changes

charn reviewed Oct 10, 2023

View reviewed changes

events/importer/espoo.py Outdated Show resolved Hide resolved

tuomas777 reviewed Oct 11, 2023

View reviewed changes

Drop create_initial_license autouse fixture

13a6e2f

Could not identify where it was needed anymore.

voneiden force-pushed the link-1375-espoo branch 2 times, most recently from d0ff51b to 30795b1 Compare October 16, 2023 12:01

voneiden requested review from tuomas777 and danipran October 16, 2023 12:04

danipran approved these changes Oct 17, 2023

View reviewed changes

voneiden force-pushed the link-1375-espoo branch from 30795b1 to 66648f1 Compare October 18, 2023 07:27

voneiden merged commit 495b2ff into main Oct 18, 2023
3 checks passed

voneiden deleted the link-1375-espoo branch October 18, 2023 07:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Espoo Importer #718

Espoo Importer #718

voneiden commented Sep 25, 2023 •

edited

Loading

codecov-commenter commented Sep 25, 2023 •

edited

Loading

voneiden commented Oct 2, 2023

danipran left a comment

tuomas777 Oct 11, 2023

tuomas777 Oct 11, 2023

tuomas777 Oct 11, 2023

danipran left a comment

danipran Oct 17, 2023

danipran Oct 17, 2023

danipran Oct 17, 2023

sonarqubecloud bot commented Oct 18, 2023

		assert [d["id"] for d in origin_data[:1]] == [i.id for i in common_objs]
		assert origin_data[1:] == origin_objs

Espoo Importer #718

Espoo Importer #718

Conversation

voneiden commented Sep 25, 2023 • edited Loading

codecov-commenter commented Sep 25, 2023 • edited Loading

Codecov Report

voneiden commented Oct 2, 2023

danipran left a comment

Choose a reason for hiding this comment

tuomas777 Oct 11, 2023

Choose a reason for hiding this comment

tuomas777 Oct 11, 2023

Choose a reason for hiding this comment

tuomas777 Oct 11, 2023

Choose a reason for hiding this comment

danipran left a comment

Choose a reason for hiding this comment

danipran Oct 17, 2023

Choose a reason for hiding this comment

danipran Oct 17, 2023

Choose a reason for hiding this comment

danipran Oct 17, 2023

Choose a reason for hiding this comment

sonarqubecloud bot commented Oct 18, 2023

voneiden commented Sep 25, 2023 •

edited

Loading

codecov-commenter commented Sep 25, 2023 •

edited

Loading