Skip to content
This repository has been archived by the owner on Apr 17, 2023. It is now read-only.

Fixed some bugs related to activities #1144

Merged
merged 1 commit into from
Jan 20, 2017
Merged

Conversation

mssola
Copy link
Collaborator

@mssola mssola commented Jan 13, 2017

Fixes #1104
Fixes #1106

Signed-off-by: Miquel Sabaté Solà msabate@suse.com

@mssola
Copy link
Collaborator Author

mssola commented Jan 13, 2017

This is a series of bug fixes and improvements in regards to activity handling. DON'T merge this yet, since I still want to confirm some other issues (this part of the code is a bit of a whack-a-mole experience...)

@mssola
Copy link
Collaborator Author

mssola commented Jan 18, 2017

@monstermunchkin @ereslibre now it should be reviewable 😉 There's still some checks that I want to do, and the coverage for activities can certainly be improved, but I'll try to do this in future PRs.

@mssola
Copy link
Collaborator Author

mssola commented Jan 19, 2017

Just added another commit. It should pass tests, but anyways I think it should be safe to review the whole thing. I'll merge this myself once I get lgtm's 😉

Copy link

@ereslibre ereslibre left a comment

Choose a reason for hiding this comment

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

LGTM 👍 Thank you Miquel.

a.owner_type = nil
a.parameters = a.parameters.merge(owner_name: display_username)
a.save
end

Choose a reason for hiding this comment

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

Maybe we can replace the find_each with an update_all so we perform only one query to the database.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's what we had before, but we need to:

a.parameters = a.parameters.merge(owner_name: display_username)

That's one of the bugs that I caught 😉 Maybe a transaction would've been better so at least everything is done atomically.

Choose a reason for hiding this comment

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

Of course, sorry for the noise. 👍

- if activity.owner == activity.recipient
- if activity.parameters[:team_user] && activity.parameters[:owner_name]
= "#{activity_owner(activity)} changed the role of #{activity_user_recipient(activity, :team_user)} "
- elsif activity.owner == activity.recipient
= "#{activity_owner(activity)} changed its role "

Choose a reason for hiding this comment

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

Maybe use their role instead of its role?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed

Besides the mentioned issues, this commit also fixes some activities when
deleting users. This commit also fixes most of the issues in SUSE#899, but
since this wasn't the goal of this commit, fixing this issue has been
left as something to be done in the near future (I'd still need to
properly review it).

Fixes SUSE#1104
Fixes SUSE#1106
See SUSE#899

Signed-off-by: Miquel Sabaté Solà <msabate@suse.com>
Copy link
Contributor

@monstermunchkin monstermunchkin left a comment

Choose a reason for hiding this comment

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

LGTM

@mssola mssola merged commit 1f43e2a into SUSE:master Jan 20, 2017
@mssola mssola deleted the csv-activity branch January 20, 2017 10:35
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants