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

Import from event feature #4533

Merged
merged 19 commits into from
Aug 17, 2020
Merged

Conversation

meluru
Copy link
Contributor

@meluru meluru commented Jul 15, 2020

Closes #4518

image

Copy link
Member

@ThiefMaster ThiefMaster left a comment

Choose a reason for hiding this comment

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

Thanks, this is already looking really nice! I just left a few comments about things i noticed when having a quick look.

indico/modules/events/cloning.py Outdated Show resolved Hide resolved
indico/modules/events/cloning.py Outdated Show resolved Hide resolved
indico/modules/events/operations.py Outdated Show resolved Hide resolved
indico/modules/events/operations.py Outdated Show resolved Hide resolved
indico/modules/events/operations.py Outdated Show resolved Hide resolved
indico/modules/events/operations.py Outdated Show resolved Hide resolved
indico/modules/events/operations.py Outdated Show resolved Hide resolved
indico/modules/events/operations.py Outdated Show resolved Hide resolved
indico/modules/events/operations.py Outdated Show resolved Hide resolved
@meluru meluru force-pushed the wip/import-from-event branch 8 times, most recently from 2d67d84 to c24a184 Compare July 20, 2020 19:44
@meluru meluru marked this pull request as ready for review July 20, 2020 19:51
Copy link
Member

@plourenco plourenco left a comment

Choose a reason for hiding this comment

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

I've been manually checking most of the import conflicts and available conditions and this seems to be working well! 👍 I just added some notes below on the causes for features I wasn't able to import.

CHANGES.rst Outdated Show resolved Hide resolved
indico/modules/events/contributions/clone.py Outdated Show resolved Hide resolved
indico/modules/events/clone.py Outdated Show resolved Hide resolved
@meluru
Copy link
Contributor Author

meluru commented Jul 29, 2020

@plourenco Thank you for the review, it was a good catch indeed.
All updated now.

Copy link
Member

@plourenco plourenco left a comment

Choose a reason for hiding this comment

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

Works fine here. Looks very close to being ready! 👍 I just added a few notes regarding the last commit.

I also noticed the import dialog seems to output both messages even before any content, perhaps something was changed here in the last push?
image

CHANGES.rst Outdated Show resolved Hide resolved
indico/modules/events/editing/clone.py Outdated Show resolved Hide resolved
@meluru
Copy link
Contributor Author

meluru commented Jul 30, 2020

@plourenco
I didn't manage to reproduce this problem here and nothing changed ui-wise in the last push.
Does it always happen for you? Maybe it's some corner case or it's browser-specific?

@meluru meluru force-pushed the wip/import-from-event branch 2 times, most recently from d1526d7 to 37dbe31 Compare July 30, 2020 09:35
Copy link
Member

@plourenco plourenco left a comment

Choose a reason for hiding this comment

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

I can no longer reproduce the problem with the dialog, must have been a change I did while testing the last commit.

Thanks!! 🎉

indico/modules/events/management/forms.py Outdated Show resolved Hide resolved
indico/modules/events/management/forms.py Outdated Show resolved Hide resolved
indico/core/signals/event/core.py Outdated Show resolved Hide resolved
indico/modules/attachments/clone.py Outdated Show resolved Hide resolved
indico/modules/categories/schemas.py Outdated Show resolved Hide resolved
indico/modules/events/clone.py Outdated Show resolved Hide resolved
indico/modules/events/cloning.py Outdated Show resolved Hide resolved
indico/modules/events/contributions/clone.py Outdated Show resolved Hide resolved
indico/modules/events/management/blueprint.py Outdated Show resolved Hide resolved
indico/modules/events/management/controllers/cloning.py Outdated Show resolved Hide resolved
@meluru
Copy link
Contributor Author

meluru commented Aug 4, 2020

All updated now! I removed all friendly_id related changes and fixed the index on sessions.

@plourenco
Copy link
Member

It would be an interesting idea if we added an indication on why some fields are greyed out, as in: "You can't import because you already made changes there" or "Only unmodified/empty modules of the event can be imported". Could even be something generic, it would be more user friendly.

@meluru
Copy link
Contributor Author

meluru commented Aug 7, 2020

@plourenco that's a good point. I'll add a message. 👍 I think I should also remove from the list the modules that don't support importing at all, because having them greyed out might be confusing.

@meluru
Copy link
Contributor Author

meluru commented Aug 11, 2020

Updated. Let me know if the message is alright.

image

@ThiefMaster
Copy link
Member

I pushed some small fixes/improvements I noticed while testing.

When trying to import the timetable from a larger event from the past (indico workshop 2.0) I'm currently getting an error about the timetable being inconsistent; I'll have a look and see if it's a quick fix, otherwise I'll ask you to look into it :p

@ThiefMaster
Copy link
Member

The timetable error happens because my target event was shorter. Should we make it a conflict in the timetable importer when the source event is longer than the target event?

@ThiefMaster
Copy link
Member

I added a commit doing just that. Importing the indico workshop event works fine now, I'll test with some other events but I think it's pretty much ready to be merged!

@ThiefMaster
Copy link
Member

After discussing with the team we thought the button next to clone is a bit too prominent so I moved it inside the cog menu:

image

Not completely sure about the icon... but so far I couldn't find any better one. The import/export icons in icomoon don't look amazing either. If you have any good idea please let me know!

@ThiefMaster
Copy link
Member

@meluru from our side everything is fine now and I think this PR is ready to be merged. Maybe you can have a quick look at the changes I made before I merge it?

Copy link
Member

@ThiefMaster ThiefMaster left a comment

Choose a reason for hiding this comment

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

very nice and useful feature :)

@ThiefMaster ThiefMaster merged commit 6bb2b55 into indico:master Aug 17, 2020
@ThiefMaster ThiefMaster deleted the wip/import-from-event branch August 17, 2020 09:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Event cloning: Allow cloning into an existing event
3 participants