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.

Sign up for GitHub

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

Jump to bottom

Issue1880 small import changes #1889

Closed
wants to merge 17 commits into from
Closed

Issue1880 small import changes #1889

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
a6dfea0
intermediate commit with renames
r-peschke Aug 21, 2023
e245e4c
Base account import on Lookup class and fix some smaller problems in …
r-peschke Aug 29, 2023
e372a85
format
r-peschke Aug 29, 2023
85c55f4
set username from saml_id, if no first-/lastname is present, but the …
r-peschke Aug 29, 2023
d4a1104
Rename from user.import to account.import
r-peschke Aug 30, 2023
84abcd1
json_upload ready for client test, import with state errors
r-peschke Aug 31, 2023
29e413d
Fix some user.create bugs concerning saml_id
r-peschke Sep 1, 2023
70ce3aa
fixed bugs in account.json_upload
r-peschke Sep 1, 2023
dbd0f8a
fix some account import errors
r-peschke Sep 4, 2023
28ba44f
test fix
r-peschke Sep 4, 2023
0ed68b0
Pre ready for review, still with some errors in test
r-peschke Sep 5, 2023
e6ea1c0
fix acoount_import tests
r-peschke Sep 5, 2023
b8ed687
fixed all errors
r-peschke Sep 6, 2023
9216821
fix code review remarks
r-peschke Sep 6, 2023
982d165
add validate_instance
r-peschke Sep 6, 2023
b509233
add 3 fields default_number, default_structure_level and default_vote…
r-peschke Sep 7, 2023
845b347
fix review remark list(ImportState)
r-peschke Sep 12, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions global/meta/models.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3825,6 +3825,9 @@ action_worker:
- running
- end
- aborted
- warning
- error
- done
Comment on lines +3828 to +3830
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure you want to add these to the general action worker? What's e.g. the difference between end and done? warning and error are really import-specific and do not convey meaning about the status of the action worker.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree and we knew, that this is a misuse of the action_worker. We did it with the idea that this is the faster way to develop the imports. It should be a separate collection, but also to be written in a separate transaction in parallel to the data. We will separate action_worker and its functionality from preview_import-worker on moving to relational db-design.
The meaning of the action_worker states is not compatible with that of the preview_import.
end in action_worker means, that the action_worker ended and can be deleted from database. The last point is, why we didn't wanted to use this state for the preview_import. abort may be similar.
We could use always running, maybe with some statistical problems or deleting long running action workers.

done in preview_import means, that everything is okay without warnings (could be imported) or error (whole import may not be imported). A program, cleaning or statistic job can easily decide by the state if it is an action worker or a preview_import.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but I don't think we should mix the stati of action workers and imports. I would suggest to leave the state of an import inside the data saved in the action worker and leave the state at running until the import is either sucessful or aborted. If we implement some cleanup tool for action workers, it can, as you said, easily decide for every action worker still running whether it is an actual action worker or an import and then decide to keep or delete it. To summarize: We already have a spearate import state inside the data, I would prefer not to mix it with the action worker's state.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no separate state inside the data, there is only a row state. Lets talk about this, I'm not willing to change

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New collection import_preview introduced and not using the action_worker for this anymore.
I'll close this in favor of #1912

restriction_mode: A
created:
type: timestamp
Expand Down
4 changes: 2 additions & 2 deletions openslides_backend/action/actions/user/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
from . import ( # noqa
account_import,
account_json_upload,
assign_meetings,
create,
delete,
forget_password,
forget_password_confirm,
generate_new_password,
import_,
json_upload,
merge_together,
reset_password_to_default,
save_saml_account,
Expand Down
13 changes: 6 additions & 7 deletions ...es_backend/action/actions/user/import_.py → ...end/action/actions/user/account_import.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,8 @@
from .user_mixin import DuplicateCheckMixin


@register_action("user.import")
class UserImport(DuplicateCheckMixin, ImportMixin):
@register_action("account.import")
class AccountImport(DuplicateCheckMixin, ImportMixin):
"""
Action to import a result from the action_worker.
"""
Expand Down Expand Up @@ -43,7 +43,8 @@ def update_instance(self, instance: Dict[str, Any]) -> Dict[str, Any]:
if field in entry["data"]:
if field == "username" and "id" in entry["data"][field]:
entry["data"]["id"] = entry["data"][field]["id"]
entry["data"][field] = entry["data"][field]["value"]
if type(dvalue := entry["data"][field]) == dict:
entry["data"][field] = dvalue["value"]

search_data_list = [
{
Expand Down Expand Up @@ -131,10 +132,8 @@ def update_instance(self, instance: Dict[str, Any]) -> Dict[str, Any]:
"Error: want to update, but found search data doesn't match."
)
else:
for field in ("username", "saml_id"):
if field in entry["data"]:
del entry["data"][field]

if "username" in entry["data"]:
del entry["data"]["username"]
update_action_payload.append(entry["data"])
else:
self.error = True
Expand Down
Loading