aleksis/AlekSIS-App-CSVImport
8
0
Fork 0
Code Issues 16 Pull requests 5 Projects Releases 2 Packages Activity Actions

WIP: General CSV importer #90

Closed
hansegucker wants to merge 6 commits from feature/general-csv-importer into master
pull from: feature/general-csv-importer
merge into: aleksis:master
Conversation 80 Commits 6 Files changed 5 +22 -6
hansegucker commented 2020-05-12 17:43:16 +02:00 (Migrated from edugit.org)
Copy link

Closes #11
Closes #12

Closes #11 Closes #12
hansegucker commented 2020-05-12 17:43:18 +02:00 (Migrated from edugit.org)
Copy link

changed milestone to %5

changed milestone to %5
hansegucker commented 2020-05-12 17:44:35 +02:00 (Migrated from edugit.org)
Copy link

added 1 commit

Compare with previous version

added 1 commit <ul><li>b7402243 - Fix validators</li></ul> [Compare with previous version](/AlekSIS/Onboarding/AlekSIS-App-SchILD-NRW/-/merge_requests/15/diffs?diff_id=2110&start_sha=86c3f498f3320d05a20450614f1311677ebdca11)
hansegucker commented 2020-05-12 19:59:22 +02:00 (Migrated from edugit.org)
Copy link

added 3 commits

  • e2c9420a - Add models for defining import templates
  • 6e871b0c - Move util code to own module
  • a6cabecb - Add models for defining import templates

Compare with previous version

added 3 commits <ul><li>e2c9420a - Add models for defining import templates</li><li>6e871b0c - Move util code to own module</li><li>a6cabecb - Add models for defining import templates</li></ul> [Compare with previous version](/AlekSIS/Onboarding/AlekSIS-App-SchILD-NRW/-/merge_requests/15/diffs?diff_id=2111&start_sha=b74022432ad37569ca13698e26b90bbe7e17703b)
hansegucker commented 2020-05-12 20:10:46 +02:00 (Migrated from edugit.org)
Copy link

added 1 commit

  • 55be02d6 - Refactor CSV import form and template for general usage

Compare with previous version

added 1 commit <ul><li>55be02d6 - Refactor CSV import form and template for general usage</li></ul> [Compare with previous version](/AlekSIS/Onboarding/AlekSIS-App-SchILD-NRW/-/merge_requests/15/diffs?diff_id=2112&start_sha=a6cabecb9727bf561b3df5b0ebdb5cebfb22f532)
hansegucker commented 2020-05-12 21:08:28 +02:00 (Migrated from edugit.org)
Copy link

added 6 commits

  • 00d86982 - Update processing of CSV files
  • 8d526d0a - Move import code to one function and define some constants
  • 68bcbfa4 - Move parser for phone numbers and sexes to own functions
  • 62bd3c91 - Get converters from template field definition
  • e2dc05ad - Validate field types
  • ef5d8ea6 - Process date_of_birth only if used in current template

Compare with previous version

added 6 commits <ul><li>00d86982 - Update processing of CSV files</li><li>8d526d0a - Move import code to one function and define some constants</li><li>68bcbfa4 - Move parser for phone numbers and sexes to own functions</li><li>62bd3c91 - Get converters from template field definition</li><li>e2dc05ad - Validate field types</li><li>ef5d8ea6 - Process date_of_birth only if used in current template</li></ul> [Compare with previous version](/AlekSIS/Onboarding/AlekSIS-App-SchILD-NRW/-/merge_requests/15/diffs?diff_id=2113&start_sha=55be02d61f36f63f267dab781e4575af5c8f13a5)
hansegucker commented 2020-05-20 12:07:05 +02:00 (Migrated from edugit.org)
Copy link

added 4 commits

  • a5a8b209 - Add field separator to import template
  • d814ebbc - Add some new field types and a getter method for field types
  • 527b2ea9 - Improve basic CSV parsing
  • 4db2d336 - Add teachers template for Pedasos

Compare with previous version

added 4 commits <ul><li>a5a8b209 - Add field separator to import template</li><li>d814ebbc - Add some new field types and a getter method for field types</li><li>527b2ea9 - Improve basic CSV parsing</li><li>4db2d336 - Add teachers template for Pedasos</li></ul> [Compare with previous version](/AlekSIS/official/AlekSIS-App-CSVImport/-/merge_requests/15/diffs?diff_id=2215&start_sha=ef5d8ea67646735c38854ae7510ddfe8f560cf5c)
hansegucker commented 2020-05-20 19:48:57 +02:00 (Migrated from edugit.org)
Copy link

added 4 commits

  • b6f2d05a - Activate advanced logging again and deactivate old objects
  • da05ee54 - Add import_ref_csv field
  • 53956ea4 - Add support for importing the departments of a person (subject-orientated)
  • 9d072df6 - Reformat and lint

Compare with previous version

added 4 commits <ul><li>b6f2d05a - Activate advanced logging again and deactivate old objects</li><li>da05ee54 - Add import_ref_csv field</li><li>53956ea4 - Add support for importing the departments of a person (subject-orientated)</li><li>9d072df6 - Reformat and lint</li></ul> [Compare with previous version](/AlekSIS/official/AlekSIS-App-CSVImport/-/merge_requests/15/diffs?diff_id=2216&start_sha=4db2d3364e8f007bf03c985ff3d6a83253d78182)
hansegucker commented 2020-05-23 17:56:04 +02:00 (Migrated from edugit.org)
Copy link

added 2 commits

Compare with previous version

added 2 commits <ul><li>61b87c3c - Refactor files</li><li>e5b9b2ea - Fix regex for whitespace</li></ul> [Compare with previous version](/AlekSIS/official/AlekSIS-App-CSVImport/-/merge_requests/15/diffs?diff_id=2277&start_sha=9d072df693cec74f8fcd688cdc1f59c94a1c311b)
hansegucker commented 2020-05-25 10:36:46 +02:00 (Migrated from edugit.org)
Copy link

added 1 commit

Compare with previous version

added 1 commit <ul><li>8a805727 - Enable testsuite</li></ul> [Compare with previous version](/AlekSIS/official/AlekSIS-App-CSVImport/-/merge_requests/15/diffs?diff_id=2328&start_sha=e5b9b2ea943d8c9520327cf0920f6a2da1810f4f)
hansegucker commented 2020-05-25 10:43:02 +02:00 (Migrated from edugit.org)
Copy link

added 1 commit

  • 2aa0e556 - Update dependency of AlekSIS and update poetry.lock

Compare with previous version

added 1 commit <ul><li>2aa0e556 - Update dependency of AlekSIS and update poetry.lock</li></ul> [Compare with previous version](/AlekSIS/official/AlekSIS-App-CSVImport/-/merge_requests/15/diffs?diff_id=2329&start_sha=8a805727d0837c899f26ad97215dd5576ec0682e)
hansegucker commented 2020-05-26 13:04:13 +02:00 (Migrated from edugit.org)
Copy link

added 3 commits

Compare with previous version

added 3 commits <ul><li>8bdddb17 - Write tests for converters</li><li>45da0773 - Improve converters</li><li>791c1995 - Reformat and lint</li></ul> [Compare with previous version](/AlekSIS/official/AlekSIS-App-CSVImport/-/merge_requests/15/diffs?diff_id=2346&start_sha=2aa0e556b5197879f71c6fac9b90a4078e13d6f3)
hansegucker commented 2020-05-26 20:05:35 +02:00 (Migrated from edugit.org)
Copy link

added 1 commit

Compare with previous version

added 1 commit <ul><li>4689f642 - Write some more unit tests</li></ul> [Compare with previous version](/AlekSIS/official/AlekSIS-App-CSVImport/-/merge_requests/15/diffs?diff_id=2349&start_sha=791c1995e0fc335a0336ab17778f8a89e85dc7e4)
hansegucker commented 2020-05-27 19:53:48 +02:00 (Migrated from edugit.org)
Copy link

added 3 commits

  • 60387bba - Refactor names of some constants
  • 9792245c - Move creation of departments to helper function
  • 3a630c98 - Add Chronos as a dependency

Compare with previous version

added 3 commits <ul><li>60387bba - Refactor names of some constants</li><li>9792245c - Move creation of departments to helper function</li><li>3a630c98 - Add Chronos as a dependency</li></ul> [Compare with previous version](/AlekSIS/official/AlekSIS-App-CSVImport/-/merge_requests/15/diffs?diff_id=2357&start_sha=4689f642209684e7f5d33b230684753467156645)
hansegucker commented 2020-05-30 20:25:05 +02:00 (Migrated from edugit.org)
Copy link

added 2 commits

  • eef3dbfd - Create groups with all imported persons, if wanted
  • e23339c0 - Fix creation of department groups

Compare with previous version

added 2 commits <ul><li>eef3dbfd - Create groups with all imported persons, if wanted</li><li>e23339c0 - Fix creation of department groups</li></ul> [Compare with previous version](/AlekSIS/official/AlekSIS-App-CSVImport/-/merge_requests/15/diffs?diff_id=2385&start_sha=3a630c98c34b7611d247555a440163ab55c445a7)
hansegucker commented 2020-05-31 21:55:03 +02:00 (Migrated from edugit.org)
Copy link

mentioned in issue #11

mentioned in issue #11
hansegucker commented 2020-06-01 17:05:22 +02:00 (Migrated from edugit.org)
Copy link

added 2 commits

  • a6c2b4b0 - Add field type "NAME"
  • e826c754 - Add is_active to update_dict only if the model has the field "is_active" |...

Compare with previous version

added 2 commits <ul><li>a6c2b4b0 - Add field type &quot;NAME&quot;</li><li>e826c754 - Add is_active to update_dict only if the model has the field &quot;is_active&quot; |...</li></ul> [Compare with previous version](/AlekSIS/official/AlekSIS-App-CSVImport/-/merge_requests/15/diffs?diff_id=2393&start_sha=e23339c02e9600950d41583b84c56f0e88497ce5)
hansegucker commented 2020-06-04 11:35:53 +02:00 (Migrated from edugit.org)
Copy link

added 3 commits

  • afb999f7 - Add support for field types which can be used multiple times
  • 54c3cf17 - Add helper method bulk_get_or_create
  • 42f57320 - Add support for group owners by short name (multiple cols)

Compare with previous version

added 3 commits <ul><li>afb999f7 - Add support for field types which can be used multiple times</li><li>54c3cf17 - Add helper method bulk_get_or_create</li><li>42f57320 - Add support for group owners by short name (multiple cols)</li></ul> [Compare with previous version](/AlekSIS/official/AlekSIS-App-CSVImport/-/merge_requests/15/diffs?diff_id=2428&start_sha=e826c754cdc68443321e4b53eb54872bd481fd67)
hansegucker commented 2020-06-06 17:02:33 +02:00 (Migrated from edugit.org)
Copy link

added 1 commit

  • 0ad1a452 - Add import template for classes from Pedasos

Compare with previous version

added 1 commit <ul><li>0ad1a452 - Add import template for classes from Pedasos</li></ul> [Compare with previous version](/AlekSIS/official/AlekSIS-App-CSVImport/-/merge_requests/15/diffs?diff_id=2469&start_sha=42f573201b00b4036a0c1a1db48659f0e72e32f4)
hansegucker commented 2020-06-06 17:46:15 +02:00 (Migrated from edugit.org)
Copy link

added 1 commit

  • 366edce6 - Add support for setting a group type for all objects

Compare with previous version

added 1 commit <ul><li>366edce6 - Add support for setting a group type for all objects</li></ul> [Compare with previous version](/AlekSIS/official/AlekSIS-App-CSVImport/-/merge_requests/15/diffs?diff_id=2472&start_sha=0ad1a45272bf8b222e8f5742c80d34345a26d582)
hansegucker commented 2020-06-10 15:50:52 +02:00 (Migrated from edugit.org)
Copy link

added 3 commits

  • 747e4f12 - Use progress bar to track import status
  • 700ce085 - Add support for group subjects and Pedasos class ranges
  • 3ed8cf01 - Add template for Pedasos courses

Compare with previous version

added 3 commits <ul><li>747e4f12 - Use progress bar to track import status</li><li>700ce085 - Add support for group subjects and Pedasos class ranges</li><li>3ed8cf01 - Add template for Pedasos courses</li></ul> [Compare with previous version](/AlekSIS/official/AlekSIS-App-CSVImport/-/merge_requests/15/diffs?diff_id=2495&start_sha=366edce66a47ce5d0021ff6afe8c914035815162)
hansegucker commented 2020-06-11 10:15:27 +02:00 (Migrated from edugit.org)
Copy link

added 3 commits

  • d3dda1b1 - Add support for importing group memberships
  • 6076649b - Add support for importing guardians from students list
  • 9476781d - Add template for students from Pedasos

Compare with previous version

added 3 commits <ul><li>d3dda1b1 - Add support for importing group memberships</li><li>6076649b - Add support for importing guardians from students list</li><li>9476781d - Add template for students from Pedasos</li></ul> [Compare with previous version](/AlekSIS/official/AlekSIS-App-CSVImport/-/merge_requests/15/diffs?diff_id=2498&start_sha=3ed8cf01c3bea2dc3d29e5d33e5ec4557b6b66a8)
hansegucker commented 2020-06-11 10:59:14 +02:00 (Migrated from edugit.org)
Copy link

added 1 commit

Compare with previous version

added 1 commit <ul><li>4349d1a0 - Add admin.py</li></ul> [Compare with previous version](/AlekSIS/official/AlekSIS-App-CSVImport/-/merge_requests/15/diffs?diff_id=2500&start_sha=9476781d9fe9a46766b6579b5e18afd130158bc4)
hansegucker commented 2020-06-11 16:27:59 +02:00 (Migrated from edugit.org)
Copy link

added 1 commit

  • 30d0d43a - Fix connection of subjects to department groups (update or create GroupSubject)

Compare with previous version

added 1 commit <ul><li>30d0d43a - Fix connection of subjects to department groups (update or create GroupSubject)</li></ul> [Compare with previous version](/AlekSIS/official/AlekSIS-App-CSVImport/-/merge_requests/15/diffs?diff_id=2501&start_sha=4349d1a00648df95d6a46fcd75a652e8b40bf34b)
hansegucker commented 2020-06-11 16:38:27 +02:00 (Migrated from edugit.org)
Copy link

Ready for review, @nik.

Tested with real data.

Ready for review, @nik. Tested with real data.
hansegucker commented 2020-06-11 16:38:29 +02:00 (Migrated from edugit.org)
Copy link

unmarked as a Work In Progress

unmarked as a **Work In Progress**
hansegucker commented 2020-06-11 16:38:29 +02:00 (Migrated from edugit.org)
Copy link

assigned to @nik

assigned to @nik
nik commented 2020-06-12 09:59:17 +02:00 (Migrated from edugit.org)
Copy link

What is the benefit of this function, compared to just calling strptime? Sure, it suppresses the ValueError, but is that always correct? Even if that is the case, maybe we want a wrapper that allows passing a format string and suppresses the exception, but even then, I would prefer handling the exception everywhere where it can occur instead. Wrapping a silent try/except pass in a function will ultimately lead to misconceptions about the code using it.

What is the benefit of this function, compared to just calling `strptime`? Sure, it suppresses the `ValueError`, but is that always correct? Even if that is the case, maybe we want a wrapper that allows passing a format string and suppresses the exception, but even then, I would prefer handling the exception everywhere where it can occur instead. Wrapping a silent `try/except pass` in a function will ultimately lead to misconceptions about the code using it.
hansegucker commented 2020-06-12 10:02:19 +02:00 (Migrated from edugit.org)
Copy link

If I see it correctly, there is no alternative to silently catch the exception because the parse functions are all directly used by pandas' read_csv function.

If I see it correctly, there is no alternative to silently catch the exception because the parse functions are all directly used by pandas' `read_csv` function.
hansegucker commented 2020-06-12 10:03:02 +02:00 (Migrated from edugit.org)
Copy link

But using a wrapper to provide different format strings seems to be a good idea.

But using a wrapper to provide different format strings seems to be a good idea.
nik commented 2020-06-12 10:18:28 +02:00 (Migrated from edugit.org)
Copy link

This documentation is too unspecific. What exactly is "parsed"? I do, actually, not see any parsing done in this function. If I get the code right, it simply splits the string and suppresses any non-truethy values. Again, while on first glance having a function that hides the list/filter/lambda construct makes teh code calling it more readable, on second glance, it leads to misconceptions.

I'd call it split_comma and use re.split(",+", value) instead of the filter construct, which I consider most readable. However, it seems re.split is really slow:

2.283657485000049       re.split(",+", "a,b,c,,d,e,,,f,g,,h,i,,j,")
0.8653640780000842      list(filter(bool, "a,b,c,,d,e,,,f,g,,h,i,,j,".split(",")))
1.0676831779999247      [v for v in "a,b,c,,d,e,,,f,g,,h,i,,j,".split(",") if v]

(measured on 1,000,000 operations)

In any case, I stick with renaming the function and properly documenting it.

This documentation is too unspecific. What exactly is "parsed"? I do, actually, not see any parsing done in this function. If I get the code right, it simply splits the string and suppresses any non-truethy values. Again, while on first glance having a function that hides the list/filter/lambda construct makes teh code calling it more readable, on second glance, it leads to misconceptions. I'd call it `split_comma` and use `re.split(",+", value)` instead of the filter construct, which I consider most readable. However, it seems `re.split` is really slow: ``` 2.283657485000049 re.split(",+", "a,b,c,,d,e,,,f,g,,h,i,,j,") 0.8653640780000842 list(filter(bool, "a,b,c,,d,e,,,f,g,,h,i,,j,".split(","))) 1.0676831779999247 [v for v in "a,b,c,,d,e,,,f,g,,h,i,,j,".split(",") if v] ``` (measured on 1,000,000 operations) In any case, I stick with renaming the function and properly documenting it.
nik commented 2020-06-12 10:21:57 +02:00 (Migrated from edugit.org)
Copy link

I see a common anti-pattern of creating a function that silences an exception and falsely claiming it were doing some parsing. This should, in all cases in this file, be clearly visible in the name of the function.

I see a common anti-pattern of creating a function that silences an exception and falsely claiming it were doing some parsing. This should, in all cases in this file, be clearly visible in the name of the function.
nik commented 2020-06-12 10:23:19 +02:00 (Migrated from edugit.org)
Copy link

Please use relative imports for imports from the same package.

Please use relative imports for imports from the same package.
hansegucker commented 2020-06-12 10:23:41 +02:00 (Migrated from edugit.org)
Copy link

Okay, should I rename them to convert_xy_or_none or what else?

Okay, should I rename them to `convert_xy_or_none` or what else?
nik commented 2020-06-12 10:27:36 +02:00 (Migrated from edugit.org)
Copy link

This should be an injected method on Subject using the model_extensions mechanism (or even a method on Subject in chronos itself, or on a manager).

Besides being cleaner, that circumvents the model import in util code — it is best practice to not have code in a util module depend on model initialisation, because depending on models being ready in utility code will backfire on you or the developer next door some time soon.

This should be an injected method on `Subject` using the `model_extensions` mechanism (or even a method on `Subject` in chronos itself, or on a manager). Besides being cleaner, that circumvents the model import in `util` code — it is best practice to not have code in a `util` module depend on model initialisation, because depending on models being ready in utility code *will* backfire on you or the developer next door some time soon.
nik commented 2020-06-12 10:28:22 +02:00 (Migrated from edugit.org)
Copy link

Same here as in last comment.

Same here as in last comment.
nik commented 2020-06-12 10:29:04 +02:00 (Migrated from edugit.org)
Copy link

s/list/Sequence, because, what's wrong with a tuple?

`s/list/Sequence`, because, what's wrong with a tuple?
nik commented 2020-06-12 10:32:49 +02:00 (Migrated from edugit.org)
Copy link

Missing docstring.

Missing docstring.
nik commented 2020-06-12 10:34:38 +02:00 (Migrated from edugit.org)
Copy link

These must be explained. Even if this implements a very specific use case for a geographically limited data source, it dhould still be somewhat conceivable for other developers (maybe intending to fork this importer for other use cases). Probaby a good way would be to explain what Pedasos is and what kind of school system it implies in a module docstring.

These must be explained. Even if this implements a very specific use case for a geographically limited data source, it dhould still be somewhat conceivable for other developers (maybe intending to fork this importer for other use cases). Probaby a good way would be to explain what Pedasos is and what kind of school system it implies in a module docstring.
nik commented 2020-06-12 10:38:13 +02:00 (Migrated from edugit.org)
Copy link

The name clash between the programming term class and the school class is still very unfortunate. If you have an idea, please rename it (and we should do that globally in turn). If not, just resolve this thread as wont-fix.

(NB. The British term form, besides sounding stupid, doesn't help much either 😂.)

The name clash between the programming term `class` and the `school class` is still very unfortunate. If you have an idea, please rename it (and we should do that globally in turn). If not, just resolve this thread as wont-fix. (NB. The British term `form`, besides sounding stupid, doesn't help much either 😂.)
nik commented 2020-06-12 10:40:20 +02:00 (Migrated from edugit.org)
Copy link

This does not hold what the docstring promises. It will return at most one object for each short name, not all short names that match, and is more likely to raise an exception on second occurance than it is to magically come up with a list as value ;).

This does not hold what the docstring promises. It will return at most one object for each short name, not all short names that match, and is more likely to raise an exception on second occurance than it is to magically come up with a list as value ;).
nik commented 2020-06-12 10:42:25 +02:00 (Migrated from edugit.org)
Copy link

Missing return type.

Missing return type.
nik commented 2020-06-12 10:42:40 +02:00 (Migrated from edugit.org)
Copy link

Missing return type.

Missing return type.
nik commented 2020-06-12 10:43:06 +02:00 (Migrated from edugit.org)
Copy link

Missing return tyoe (please check this mistake on other functions yourself).

Missing return tyoe (please check this mistake on other functions yourself).
hansegucker commented 2020-06-12 10:44:58 +02:00 (Migrated from edugit.org)
Copy link

and is more likely to raise an exception on second occurance than it is to magically come up with a list as value ;).

Fortunately the short name is unique 😂

> and is more likely to raise an exception on second occurance than it is to magically come up with a list as value ;). Fortunately the short name is unique :joy:
nik commented 2020-06-12 10:45:41 +02:00 (Migrated from edugit.org)
Copy link

You're most welcome here ;).

You're most welcome here ;).
hansegucker commented 2020-06-12 10:46:39 +02:00 (Migrated from edugit.org)
Copy link

Self review: CSVImport is official again.

Self review: CSVImport is official again.
nik commented 2020-06-12 10:46:52 +02:00 (Migrated from edugit.org)
Copy link

Please use contextlib.suppress to make the intention clear.

Please use `contextlib.suppress` to make the intention clear.
nik commented 2020-06-12 10:49:11 +02:00 (Migrated from edugit.org)
Copy link

This (and the following settings) look very much as if they should be defaults inside one importer.

This (and the following settings) look very much as if they should be defaults inside one importer.
nik commented 2020-06-12 10:49:48 +02:00 (Migrated from edugit.org)
Copy link

Uh, that's one opinionated default.

Maybe this should be a global setting as well, or probably even a preference.

Uh, that's one opinionated default. Maybe this should be a global setting as well, or probably even a preference.
nik commented 2020-06-12 10:51:34 +02:00 (Migrated from edugit.org)
Copy link

templates.py is an unfortunate naming. It will at some point in time cause confusion with the templated directory. Although Python made an __init__.py mandotory some time ago to make a directory recognisable as a package, I would not count on every tool out there having realised that by now. Probably jsut move this to util/.

`templates.py` is an unfortunate naming. It will at some point in time cause confusion with the `templated` directory. Although Python made an `__init__.py` mandotory some time ago to make a directory recognisable as a package, I would not count on every tool out there having realised that by now. Probably jsut move this to `util/`.
nik commented 2020-06-12 10:51:53 +02:00 (Migrated from edugit.org)
Copy link

Please use relative imports.

Please use relative imports.
nik commented 2020-06-12 10:53:48 +02:00 (Migrated from edugit.org)
Copy link

ANother argument: Until I read more of the code, I thought you were actually talking about Django templates. Avoid such ambiguous naming. I propose source instead of template.

ANother argument: Until I read more of the code, I thought you were actually talking about Django templates. Avoid such ambiguous naming. I propose `source` instead of `template`.
nik commented 2020-06-12 10:54:47 +02:00 (Migrated from edugit.org)
Copy link

It seems that hard-coded German date format has a lot of occurances.

It seems that hard-coded German date format has a lot of occurances.
nik commented 2020-06-12 10:56:22 +02:00 (Migrated from edugit.org)
Copy link

This part of the code looks funny. You are allowed to make sarcastic comments about third-party software that explain why such a mess is necessary ;).

This part of the code looks funny. You *are* allowed to make sarcastic comments about third-party software that explain why such a mess is necessary ;).
nik commented 2020-06-12 10:58:17 +02:00 (Migrated from edugit.org)
Copy link

Already posted a a clear notion in other comments, but in general: I do not like the mixing of opinionated code, source-spiecific code, and global utility code. Each supported import source should be in a seaprate package. Probably using a base class that uses the general settings, and sub-classes with importer-specific defaults and code. Most of the utility functions should be methods on these classes. Object-orientation was invented for a reason :).

Already posted a a clear notion in other comments, but in general: I do not like the mixing of opinionated code, source-spiecific code, and global utility code. Each supported import source should be in a seaprate package. Probably using a base class that uses the general settings, and sub-classes with importer-specific defaults and code. Most of the utility functions should be methods on these classes. Object-orientation was invented for a reason :).
hansegucker commented 2020-06-12 10:59:50 +02:00 (Migrated from edugit.org)
Copy link

Feel free to do that ;-)

Feel free to do that ;-)
nik commented 2020-06-12 11:13:44 +02:00 (Migrated from edugit.org)
Copy link

Nope, not going to tidy up your code :).

Nope, not going to tidy up your code :).
nik commented 2020-06-12 11:16:34 +02:00 (Migrated from edugit.org)
Copy link

That makes it even more questionnable what this function is for. You cannot group a single object, "group by short_name" means there are several objects with a common key that are grouped together, resulting in a Dict[str, Sequence] type.

That makes it even more questionnable what this function is for. You cannot group a single object, "group by short_name" means there are several objects with a common key that are grouped together, resulting in a `Dict[str, Sequence]` type.
hansegucker commented 2020-06-12 11:50:33 +02:00 (Migrated from edugit.org)
Copy link

And your's also :) (a little bit)

And your's also :) (a little bit)
hansegucker commented 2020-06-13 16:03:05 +02:00 (Migrated from edugit.org)
Copy link

added 1 commit

  • a5bc6ac2 - Update path of progress template

Compare with previous version

added 1 commit <ul><li>a5bc6ac2 - Update path of progress template</li></ul> [Compare with previous version](/AlekSIS/official/AlekSIS-App-CSVImport/-/merge_requests/15/diffs?diff_id=2530&start_sha=30d0d43a2ebb3300c0c72438473f8fb7c3038b29)
nik commented 2020-06-24 12:39:42 +02:00 (Migrated from edugit.org)
Copy link

marked as a Work In Progress

marked as a **Work In Progress**
hansegucker commented 2020-06-27 14:23:10 +02:00 (Migrated from edugit.org)
Copy link

added 1 commit

  • efca55fb - Use virtual foreign key for linking subjects to groups

Compare with previous version

added 1 commit <ul><li>efca55fb - Use virtual foreign key for linking subjects to groups</li></ul> [Compare with previous version](/AlekSIS/official/AlekSIS-App-CSVImport/-/merge_requests/15/diffs?diff_id=2675&start_sha=a5bc6ac20e96276c4f11933971624bc4d35ab40f)
hansegucker commented 2020-07-02 12:18:41 +02:00 (Migrated from edugit.org)
Copy link

TODO: Use school term for groups

TODO: Use school term for groups
hansegucker commented 2020-07-06 17:04:26 +02:00 (Migrated from edugit.org)
Copy link

added 1 commit

  • ff5de98c - Add support for import of school term related data

Compare with previous version

added 1 commit <ul><li>ff5de98c - Add support for import of school term related data</li></ul> [Compare with previous version](/AlekSIS/official/AlekSIS-App-CSVImport/-/merge_requests/15/diffs?diff_id=2766&start_sha=efca55fbb28eaa260346cfaca6ebb4ac650e6836)
hansegucker commented 2020-07-19 21:07:15 +02:00 (Migrated from edugit.org)
Copy link

TODO: Set primary group of persons.

TODO: Set primary group of persons.
hansegucker commented 2020-07-25 13:43:38 +02:00 (Migrated from edugit.org)
Copy link

changed the description

changed the description
hansegucker commented 2020-07-25 13:44:07 +02:00 (Migrated from edugit.org)
Copy link

changed the description

changed the description
hansegucker commented 2020-07-25 13:44:45 +02:00 (Migrated from edugit.org)
Copy link

Done.

Done.
hansegucker commented 2020-07-25 15:37:00 +02:00 (Migrated from edugit.org)
Copy link

changed this line in version 26 of the diff

changed this line in [version 26 of the diff](/AlekSIS/official/AlekSIS-App-CSVImport/-/merge_requests/15/diffs?diff_id=2886&start_sha=ff5de98cf5e2f01eede560dbd40c5b0623a4fb8b#68783f4a8ecea2099b8c417c4cca7f064c872bd0_37_37)
hansegucker commented 2020-07-25 15:37:01 +02:00 (Migrated from edugit.org)
Copy link

added 2 commits

  • 67bf161c - Filter groups created while importing other objects by school term
  • 1b221201 - Import primary groups of persons correctly

Compare with previous version

added 2 commits <ul><li>67bf161c - Filter groups created while importing other objects by school term</li><li>1b221201 - Import primary groups of persons correctly</li></ul> [Compare with previous version](/AlekSIS/official/AlekSIS-App-CSVImport/-/merge_requests/15/diffs?diff_id=2886&start_sha=ff5de98cf5e2f01eede560dbd40c5b0623a4fb8b)
hansegucker commented 2020-07-25 15:41:09 +02:00 (Migrated from edugit.org)
Copy link

changed this line in version 27 of the diff

changed this line in [version 27 of the diff](/AlekSIS/official/AlekSIS-App-CSVImport/-/merge_requests/15/diffs?diff_id=2887&start_sha=1b221201ccb83894b8c8d43606a0f85ac5bc9824#4fbeb1e0f02b7e3e1437c4fbe36f48ce4bf9b07c_7_7)
hansegucker commented 2020-07-25 15:41:09 +02:00 (Migrated from edugit.org)
Copy link

added 2 commits

Compare with previous version

added 2 commits <ul><li>6f6794b9 - Update poetry.lock</li><li>91e484cc - Reformat</li></ul> [Compare with previous version](/AlekSIS/official/AlekSIS-App-CSVImport/-/merge_requests/15/diffs?diff_id=2887&start_sha=1b221201ccb83894b8c8d43606a0f85ac5bc9824)
hansegucker commented 2020-07-25 16:21:34 +02:00 (Migrated from edugit.org)
Copy link

added 1 commit

Compare with previous version

added 1 commit <ul><li>6f83472e - Fix URL to save button</li></ul> [Compare with previous version](/AlekSIS/official/AlekSIS-App-CSVImport/-/merge_requests/15/diffs?diff_id=2888&start_sha=91e484cc41fd77f1a9a4c27e613b95892af70272)
hansegucker commented 2020-08-11 18:13:13 +02:00 (Migrated from edugit.org)
Copy link

added 1 commit

  • 4a90c4cc - Catch IndexError exception while getting current school term

Compare with previous version

added 1 commit <ul><li>4a90c4cc - Catch IndexError exception while getting current school term</li></ul> [Compare with previous version](/AlekSIS/official/AlekSIS-App-CSVImport/-/merge_requests/15/diffs?diff_id=3055&start_sha=6f83472e76b2b5e1aa3b1a4af9b807cca44e490b)
hansegucker commented 2020-08-11 18:19:46 +02:00 (Migrated from edugit.org)
Copy link

added 1 commit

  • d30b60ce - Mark imported objects with flag to identify them later

Compare with previous version

added 1 commit <ul><li>d30b60ce - Mark imported objects with flag to identify them later</li></ul> [Compare with previous version](/AlekSIS/official/AlekSIS-App-CSVImport/-/merge_requests/15/diffs?diff_id=3056&start_sha=4a90c4cc5ec7d03da978c2d0d517a171348d0162)
hansegucker commented 2020-08-11 21:30:09 +02:00 (Migrated from edugit.org)
Copy link

added 1 commit

Compare with previous version

added 1 commit <ul><li>6575cef9 - Delete old group memberships</li></ul> [Compare with previous version](/AlekSIS/official/AlekSIS-App-CSVImport/-/merge_requests/15/diffs?diff_id=3062&start_sha=d30b60ce66db5a9dadc6138836c225f929d289ed)
hansegucker commented 2020-09-24 16:22:34 +02:00 (Migrated from edugit.org)
Copy link

added 1 commit

Compare with previous version

added 1 commit <ul><li>fc5402e1 - Fix import</li></ul> [Compare with previous version](/AlekSIS/official/AlekSIS-App-CSVImport/-/merge_requests/15/diffs?diff_id=3386&start_sha=6575cef9a18908c9e3d5a6caf212ff5ed55b2649)
hansegucker commented 2020-11-08 10:45:51 +01:00 (Migrated from edugit.org)
Copy link

changed this line in version 33 of the diff

changed this line in [version 33 of the diff](/AlekSIS/official/AlekSIS-App-CSVImport/-/merge_requests/15/diffs?diff_id=3602&start_sha=fc5402e188b652a3e8e6e840fec63483f01b9b0a#28efd6e53b5d3111abe7bd20c76fe5ff341582e3_30_30)
hansegucker commented 2020-11-08 10:45:53 +01:00 (Migrated from edugit.org)
Copy link

added 2 commits

Compare with previous version

added 2 commits <ul><li>85813975 - Catch error in app config</li><li>19d64d20 - Reformat</li></ul> [Compare with previous version](/AlekSIS/official/AlekSIS-App-CSVImport/-/merge_requests/15/diffs?diff_id=3602&start_sha=fc5402e188b652a3e8e6e840fec63483f01b9b0a)
hansegucker commented 2020-11-08 11:07:53 +01:00 (Migrated from edugit.org)
Copy link

changed the description

changed the description
hansegucker (Migrated from edugit.org) closed this pull request 2020-11-08 14:08:56 +01:00

Pull request closed

Please reopen this pull request to perform a merge.
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
Security

   
TeX
Needs LaTeX template

  
auto-update
  
board
done

  
board
ready

  
board
todo

  
check
delete-eslint-rc-js

  
check
update-builddeps-package-json

  
check
update-eslint-rc-js

  
check
update-gitignore

  
check
update-merge-request-template

  
check
update-prettier-ignore

  
check
update-pyproject-toml

  
check
update-renovate-json

  
check
update-tox-ini

  
part
backend
 Backend (view code, models, logic, etc.)

  
part
ci

  
part
docs
 Issue or addition to the documentation

  
part
frontend
 Frontend (templates, etc.)

  
part
i18n
 Translation or internationalisation issue

  
part
non-technical
 Non-technical (policy, maintenance, etc.) issue or discussion

  
part
packaging
 Issue concerning the packaging of AlekSIS and components

  
prio
1
 Priority High

  
prio
2
 Priority Medium

  
prio
3
 Priority Low

  
release-mr-5.x
  
size
large

  
size
medium

  
size
small

  
source
customer
 Requested by a customer or affiliated user

  
source
customer::fsmw

  
source
customer::fss

  
source
customer::teckids

  
source
downstream
 Issue or merge request from other projects using AlekSIS or parts of it

  
type
breaking

  
type
bug
 Bug

  
type
feature
 New feature

  
type
refactoring

  
workflow
blocked
 Issue is blocked by another issue

  
workflow
confirmed
 The bug is reproducible and will be fixed

  
workflow
current-todo

  
workflow
discussing
 Needs to be discussed before implementation

  
workflow
new-app
 Requires new AlekSIS app, which has to be created

  
workflow
wontfix
 The bug is not reproducible or the feature was decided against

No labels
Security
TeX
auto-update
board
done
board
ready
board
todo
check
delete-eslint-rc-js
check
update-builddeps-package-json
check
update-eslint-rc-js
check
update-gitignore
check
update-merge-request-template
check
update-prettier-ignore
check
update-pyproject-toml
check
update-renovate-json
check
update-tox-ini
part
backend
part
ci
part
docs
part
frontend
part
i18n
part
non-technical
part
packaging
prio
1
prio
2
prio
3
release-mr-5.x
size
large
size
medium
size
small
source
customer
source
customer::fsmw
source
customer::fss
source
customer::teckids
source
downstream
type
breaking
type
bug
type
feature
type
refactoring
workflow
blocked
workflow
confirmed
workflow
current-todo
workflow
discussing
workflow
new-app
workflow
wontfix
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
aleksis/AlekSIS-App-CSVImport!90
No description provided.