Previously it was always necessary to provide a full list of users when calling the service.
Now it's also possible to provide a delta that should be achieved.
This allows to simplify calls to the service, when only the delta is known (e.g. "add this user").
It also makes those calls safer, since the internals of the Groups::UpdateService are already locked
through an advisory lock (via BaseContracted), thus concurrent changes to group memberships are serialized
properly inside the service. However, previous implementations would have read the current members of a group
outside of the service scope, where race conditions could have occured.
The subject test was `Member.find_by(principal: users.last).roles`. As
there are 3 different `Member` records having the same principal, it
could return any of them. Most of the time it would always be the first
one, which is a project role membership and satisfies the test
conditions, but occasionnally, it would be the global role membership or
the work package role membership and the test would fail.
Using `Member.where(principal: users.last).flat_map(&:roles)` and
testing for all roles fixes the test flakiness.
Also changed some wording here and there to make the test easier to
understand.
We're spending over 50% of the time spent in factories. By leveraging the
`shared_let` strategy we can chop this down significantly to something more
reasonable.
Performance was improved by 50% as well as with a 50% reduction in time spent in
factories.
** Before **
Finished in 4.82 seconds (files took 9.09 seconds to load)
46 examples, 0 failures
Randomized with seed 3124
[TEST PROF INFO] Time spent in factories: 00:04.020 (57.97% of total time)
[TEST PROF INFO] Factories usage
Total: 499
Total top-level: 366
Total time: 00:04.020 (out of 00:11.324)
Total uniq factories: 9
total top-level total time time per call top-level time name
133 0 0.2047s 0.0015s 0.0000s notification_setting
88 88 0.9106s 0.0103s 0.9106s user
79 79 0.2084s 0.0026s 0.2084s project_role
50 50 0.6100s 0.0122s 0.6100s group
45 45 0.3730s 0.0083s 0.3730s admin
42 42 0.1811s 0.0043s 0.1811s member
38 38 1.6674s 0.0439s 1.6674s project
16 16 0.0382s 0.0024s 0.0382s global_role
8 8 0.0321s 0.0040s 0.0321s global_member
** After **
Finished in 2.15 seconds (files took 8.89 seconds to load)
46 examples, 0 failures
Randomized with seed 44002
[TEST PROF INFO] Time spent in factories: 00:01.168 (27.67% of total time)
[TEST PROF INFO] Factories usage
Total: 121
Total top-level: 117
Total time: 00:01.168 (out of 00:08.508)
Total uniq factories: 9
total top-level total time time per call top-level time name
50 50 0.5710s 0.0114s 0.5710s group
42 42 0.1891s 0.0045s 0.1891s member
9 9 0.0561s 0.0062s 0.0561s project_role
8 8 0.0338s 0.0042s 0.0338s global_member
4 0 0.0159s 0.0040s 0.0000s notification_setting
3 3 0.0233s 0.0078s 0.0233s user
3 3 0.0101s 0.0034s 0.0101s global_role
1 1 0.0376s 0.0376s 0.0376s admin
1 1 0.2474s 0.2474s 0.2474s project
There is no case of factory cascading as demonstrated by the test-prof output
in the below ** BEFORE ** section. However, we're creating a lot of factories
and spending ~40% of the time in factories.
By using `shared_let`, we can reduce the number of factories being created
and speed up the test with the same behavior as it currently has.
Execution time was cut to < 1 second, meaning the spec file is now 2.6x faster.
** BEFORE **
Finished in 2.47 seconds (files took 9.5 seconds to load)
17 examples, 0 failures
Randomized with seed 8911
[TEST PROF INFO] Time spent in factories: 00:01.863 (40.63% of total time)
[TEST PROF INFO] Factories usage
Total: 210
Total top-level: 160
Total time: 00:01.863 (out of 00:08.721)
Total uniq factories: 9
total top-level total time time per call top-level time name
50 0 0.0872s 0.0017s 0.0000s notification_setting
34 34 0.4331s 0.0127s 0.4331s user
21 21 0.0600s 0.0029s 0.0600s role
21 21 0.0551s 0.0026s 0.0551s global_role
17 17 0.2831s 0.0167s 0.2831s group
17 17 0.0770s 0.0045s 0.0770s member
17 17 0.0518s 0.0030s 0.0518s global_member
17 17 0.7791s 0.0458s 0.7791s project
16 16 0.1244s 0.0078s 0.1244s admin
** AFTER **
Finished in 0.95206 seconds (files took 8.81 seconds to load)
17 examples, 0 failures
Randomized with seed 27229
[TEST PROF INFO] Time spent in factories: 00:00.441 (14.48% of total time)
[TEST PROF INFO] Factories usage
Total: 14
Total top-level: 11
Total time: 00:00.441 (out of 00:07.169)
Total uniq factories: 9
total top-level total time time per call top-level time name
3 0 0.0168s 0.0056s 0.0000s notification_setting
2 2 0.0156s 0.0078s 0.0156s user
2 2 0.0121s 0.0061s 0.0121s global_role
2 2 0.0234s 0.0117s 0.0234s role
1 1 0.2310s 0.2310s 0.2310s project
1 1 0.0031s 0.0031s 0.0031s global_member
1 1 0.0440s 0.0440s 0.0440s admin
1 1 0.0872s 0.0872s 0.0872s group
1 1 0.0250s 0.0250s 0.0250s member
When calling a service with `send_notifications: false`, the
`Journal::NotificationConfiguration.active?` will be set to `false` and
subsequent calls to set it to `true` will have no effect and log a
warning.
For this reason, it's better to use `nil` as default value for
`send_notifications` so that
`Journal::NotificationConfiguration.active?` is changed only when the
value is explicitly `true` or `false`, and ignored when the value is
`nil`.
Before, all the projects the group might have been in before the new
membership got created were considered by the SQL which potentially
results in a lot of records to be processed (the results were correct).
Now, only the project the new membership is created for is considered in
the SQL as that is the only project in which the group's users could
rightfully become members now.
before, the email sent on group membership removal/ deletion was from Anonymous user. Now it is from the real user having performed the action in the administration
* include custom message in membership forms
* keep pristine params to be able to pass it to the state
* fix indentation on projects api docs
* pass grape instance around in default endpoints
The grape instance has readily available access to all the objects (params, current_user) so less individual parameters need to be passed. This also avoids having to store the grape endpoint in a potentials not thread safe variable
* send custom message out on membership creation
* send custom message on membership update
* send custom message even if setting disabled
* restore params interface
* add custom message description to schema
* describe notificationMessage in the api documentation
* extract meta payload functionality into mixin
* ensure password in spec meets requirements
* Update docs/api/apiv3/endpoints/members.apib
Co-authored-by: Oliver Günther <mail@oliverguenther.de>
* Update docs/api/apiv3/endpoints/members.apib
Co-authored-by: Oliver Günther <mail@oliverguenther.de>
* Update docs/api/apiv3/endpoints/members.apib
Co-authored-by: Oliver Günther <mail@oliverguenther.de>
* Update docs/api/apiv3/endpoints/members.apib
Co-authored-by: Oliver Günther <mail@oliverguenther.de>
* Update docs/api/apiv3/endpoints/members.apib
Co-authored-by: Oliver Günther <mail@oliverguenther.de>
Co-authored-by: Oliver Günther <mail@oliverguenther.de>
* spec with correctly scoped links
* move db check into own file - fix deprecation
* basic spec for member creation service
* use constants for all notifications
* send an OP notification after member has been created
* send an OP notification after member has been updated
* mails on group member added
Depending on whether the membership existed before or not, an updated or
a created notification is send. This is done asynchronously.
* move all mail sender background jobs into namespace
* wip
* wip
* correct handling group member notifications
* add setting enable/disable mail sending on member alterations
* use services in members controller
* move Notifiable to OpenProject
* remove member after save hooks
* cleanup/testing/linting
* render member mails in receiver locale
* remove add_member! method
* use mailer layout for all mailers
* Update app/services/groups/cleanup_inherited_roles_service.rb
Co-authored-by: Oliver Günther <mail@oliverguenther.de>
* use around callback to avoid prepending
* handle nil params
Co-authored-by: Oliver Günther <mail@oliverguenther.de>
Before, it was implemented by passing the changed attribut keys over to
the contract to whitelist them.
This lead to:
* The contract interface becoming bloated
* Having to rely on the knowledge of the developer not to falsely
whitelist an attribute. The developer would also have to make sure
to not perform a mass assignment after the attribute has been
whitelisted
The new approach it to integrate the behaviour into the model which is
first altered in the service before it is scrutinized in the contract.
The information about the changed attributes is now stored inside the
model which removes the necessity to flag the whitelisted attribute
separately. Additionally, the exact change is tracked. So if an
attribute is set to one value inside a whitelisted block there is no
risk in later on performing a mass assignment.
This comes at the cost of extending the models which is weird also it is
build into the default SetAttributesService so child classes do not have
to worry. One might include the module into every AR model but currently
we only need it for a very specific use case.
Klaus Zanders