Refactor Event module #4191

Ana06 · 2017-12-06T12:50:37Z

Refactor Event module, to make things in a more Rails way, so we don't need to do strange things to use this module More specifically:

Have every class inside Event in a different file, as it is normally done in Rails.
Include CommentEvent in the Event module.
Do not use shortcut for classes definitions.
Remove the explicit inclusion, as now that all classes follow Rails name convention, they are not needed.
Regenerate rubocop todo file, as we move code to different files and the exclusion doesn't have effect anymore.
Remove Event::Package.
Add schema information for all new classes.
Remove old comment.

There are more details in the commit messages.

Have every `Event::Build` subclass in a different file, as it is normally done in Rails.

Have every `Event::Package` subclass in a different file, as it is normally done in Rails.

Have every `Event::Project` subclass in a different file, as it is normally done in Rails.

Have every `Event::Request` subclass in a different file, as it is normally done in Rails.

Now that all the classes follow Rails name convention, we do not need to include the files one by one. Remove also the `require_dependency 'event/all'` in `request_controller`.

This comment was introduced in: 6589e16 But the code it is commenting was removed in: 20740ef But the comment was not removed.

As Event module is now structure in a Rails way the requires are not longer needed.

When moving the code into different classes the exclusion in the `rubocop_todo.yml` doesn't have effect any more. Also, we had an inclussion of a helper outside a class/module. Rubocop was not complainning before due to a bug, that is already fixed in master and will be in the next release. Regenerating the todo file solve this as well. As we need to solve this in more places, it seems to be fine to exclude it by now.

To allow that the classes which includes it can find it, as now we do things in the Rails way.

Using `Event::Class` oblies us to include `CommentEvent` with an explicit namespace qualifier (`Event::CommentEvent`), because Ruby will not look inside `Event` for this name.

For consistency with the rest classes of the `Event` module.

And now that we are following Rails way, they are correct!

`Event::Package` is a class with almost nothing that is used as superclass for: - `Event::BranchCommand` - `Event::Build` - `Event::CommentForPackage` - `Event::Commit` - `Event::CreatePackage` - `Event::DeletePackage` - `Event::ServiceFail` - `Event::ServiceSuccess` - `Event::UndeletePackage` - `Event::UpdatePackage` - `Event::Upload` - `Event::VersionChange` The `Event::Package` causes problem as we also have a `Package` model in the root level. So once one of the two packages models is loaded, Rails doens't look for the second one, which lead to uses of the wrong model. To avoid that Rails uses the `Package` in the root level in `Event::Package` subclasses we can call the Package specifying the module. But for the other case there is nothing we can do. This is all caused because we shouldn't have models with repeated names even if they are in different levels. As the `Event::Package` is useless, we can just remove it and add its data directly to the subclasses.

`Event::Package` is a class with almost nothing that is used as superclass for: - `Event::CommentForProject` - `Event::CreateProject` - `Event::DeleteProject` - `Event::UndeleteProject` - `Event::UpdateProjectConfig` - `Event::UpdateProject` The `Event::Project` causes problem as we also have a `Project` model in the root level. So once one of the two projects models is loaded, Rails doens't look for the second one, which lead to uses of the wrong model. To avoid that Rails uses the `Project` in the root level in `Event::Project` subclasses we can call the Project specifying the module. But for the other case there is nothing we can do. This is all caused because we shouldn't have models with repeated names even if they are in different levels. As the `Event::Project` is useless, we can just remove it and add its data directly to the subclasses.

evanrolfe

Nice work! Couple small comments but nothing major. Thanks

evanrolfe · 2017-12-06T12:52:40Z

src/api/app/models/event.rb

@@ -1 +1,2 @@
-require_relative 'event/all'
+module Event


I'm pretty sure this file isn't necessary?

If not Event is not defined. You'll get something like:

LoadError: Unable to autoload constant Event, expected /obs/src/api/app/models/event.rb to define it

What line does that error come from?

That's what you get if you try to access to Event from the console.

I created another branch exactly like this one, but deleting this two lines and that's the Travis result: https://travis-ci.org/Ana06/open-build-service/builds/312423246

As you can see it fails as Event definition is not where Rails expected it to be (see rspec for example). So it can not be removed. 😉

evanrolfe · 2017-12-06T12:58:49Z

src/api/script/delayed_job_stats.rb

+               "Event::DeleteProject",
+               "Event::UndeleteProject",
+               "Event::UpdateProjectConfig",
+               "Event::UpdateProject"]


Wouldn't the best thing here be to list all subclasses of Event? Otherwise we have to update this file if we add a new Event class.

But it is not all of the subclasses of Event. This is only the subclasses of the old Event::Package and Event::Project. So now, the classes in Event related to Project and Package. I think the only way is to list them directly.

@Ana06 Please add a comment, mentioning those are only the subclasses related to Project and Package

@bgeuken ok! 👍

bgeuken

Looks good. Well done!

mschnitzer

Only nitpicks from my side. Haven't had time to fully review it.

PS: I know that you sometimes copied just lines but it would by very cool if you could refactor my nitpicks if you have time.

mschnitzer · 2017-12-07T07:10:35Z

src/api/app/models/event/build.rb

-    "#{Configuration.amqp_namespace}.package.build_unchanged"
+    def custom_headers
+      mid = my_message_id
+      h = super
