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

Allow users to clone a contribution #3207

Merged
merged 2 commits into from
Jan 26, 2018

Conversation

mic4ael
Copy link
Member

@mic4ael mic4ael commented Jan 17, 2018

No description provided.

self._synchronize_friendly_id(new_event)
db.session.flush()
return {'contrib_map': self._contrib_map, 'subcontrib_map': self._subcontrib_map}

def _clone_contribs(self, new_event):
def _create_new_contribution(self, event, contribution, excluded_attrs=None):
Copy link
Member

Choose a reason for hiding this comment

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

I think keeping it named old_contrib is clearer, since then you have <new|old>_contrib instead of new_contrib and contribution.


class RHCloneContribution(RHManageContributionBase):
def _process(self):
clone_contribution(self.event, self.contrib, False)
Copy link
Member

Choose a reason for hiding this comment

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

I would use a kwarg for the last one, or just omit it and set a default value of False.

def _process(self):
clone_contribution(self.event, self.contrib, False)
db.session.commit()
return jsonify_data(html=self.list_generator.render_list()['html'])
Copy link
Member

Choose a reason for hiding this comment

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

What about using the same way like we do for all the other endpoints?

return jsonify_data(**self.list_generator.render_list())

Not sure if there's anything besides html but if it's used to update the contribution list it'd probably break if we ever added something else since here that would then be missing.

}

if preserve_session:
shared_data['sessions']['session_map'][contribution.session] = contribution.session
Copy link
Member

Choose a reason for hiding this comment

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

You could always populate this above; no harm in having it even when the session is not used.


def clone_contribution(event, contribution, preserve_session):
shared_data = {
'event_persons': {'person_map': {person: person for person in event.persons}},
Copy link
Member

Choose a reason for hiding this comment

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

You could actually do this: dict(zip(event.persons, event.persons))

The literals you used are also quite clear, just mentioning that this would work too :p

shared_data['sessions']['session_block_map'][contribution.session_block] = contribution.session_block

cloner = ContributionCloner(event, contribution=contribution, preserve_session=preserve_session)
cloner.run(event, [], shared_data)
Copy link
Member

Choose a reason for hiding this comment

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

the 2nd argument is usually a set. so maybe passing an empty set() is safer

.first())

def _process(self):
clone_contribution(self.event, self.contribution, True)
Copy link
Member

Choose a reason for hiding this comment

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

kwarg style for the last arg


def _process(self):
clone_contribution(self.event, self.contribution, True)
db.session.commit()
Copy link
Member

Choose a reason for hiding this comment

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

why are you committing here? db.session.flush() is all you need if you just need to populate the id and default args

class RHCloneContribution(RHManageContributionBase):
def _process(self):
clone_contribution(self.event, self.contrib, False)
db.session.commit()
Copy link
Member

Choose a reason for hiding this comment

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

why are you committing here? db.session.flush() is all you need if you just need to populate the id and default args

new_contrib.acl_entries = clone_principals(ContributionPrincipal, old_contrib.acl_entries)
new_contrib.references = list(self._clone_references(ContributionReference, old_contrib.references))
new_contrib.person_links = list(self._clone_person_links(ContributionPersonLink,
old_contrib.person_links))
Copy link
Member

Choose a reason for hiding this comment

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

I think this now fits in one line

if self.preserve_session:
entry = schedule_contribution(new_contribution, self.contribution.timetable_entry.start_dt,
session_block=new_contribution.session_block)
self.old_event.timetable_entries.append(entry)
Copy link
Member

Choose a reason for hiding this comment

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

I really dislike the idea of using old_event for any operation that creates something new. When cloning something within the event, the new event should be the same as old_event anyway, but by not using the old event you remove the risk of code actually messing with the old event during normal event cloning.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point.

self._clone_contrib()
else:
self._clone_contribs(new_event)

self._synchronize_friendly_id(new_event)
Copy link
Member

Choose a reason for hiding this comment

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

This shouldn't run when cloning a single contribution.

@mic4ael mic4ael force-pushed the cloning-contributions branch 6 times, most recently from 9330c58 to 643aa54 Compare January 26, 2018 14:55
Also:

- add a changelog entry
- fix timetable balloon behavior when clicking the clone button
@ThiefMaster ThiefMaster merged commit 1dfd5a9 into indico:master Jan 26, 2018
@mic4ael mic4ael deleted the cloning-contributions branch January 29, 2018 06:50
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.

None yet

2 participants