FEATURE: option to update default notification level of existing users. (PR #14084)

Previously, a group’s default_notification_level change will only affect the users added after it.

GitHub

I’m wondering if we need this final assertion since it doesn’t test for any state change on the server side. The PUT request here should be idempotent so there is no need for us to test for a 200 response.

Since user1's group_user notification level has been updated to 1, should we be asserting that his notification level remains at 1?

Since the description of this test is about updating the notification level, I think the assertions here need to assert that the desired notification level has been updated. I’m not quite sure what user_count represents but I do not think it is sufficient to test for a number of user that has been changed.

Should this assertion be against user1 instead of user2 since we changed user1's notification level in a prior request?

Here it’s correctly returning success: true instead of user_count: 3 like in this line. This final assertion is little different from the first one.

Do you mean success: 'OK' is not always in the response even if the status code is 200?

yes, it will return user_count: X in some conditions.

if user1 notification level is updated to 2 wrongly then we will get incorrect out in response.parsed_body["user_count"]. Anyway, it would be better to make sure the user1’s notification level after the update :+1: I will do the change.

No, it’s correct. user1 is not affected by this update since it has a custom notification level now.

But user2's notification level has already been updated to 2 in a prior request, why will this request update user2's notification level again?

Nope. That’s where you misunderstood. In the previous request, it will return only the user count (number of users who are going to be updated). It will update existing users only if the update_existing_users: true param is available.

Anyway, I added one more important fix to correctly skip existing users.

          group.update!(default_notification_level: NotificationLevels.all[:watching])
          user1 = Fabricate(:user)
          user2 = Fabricate(:user)
          group.add(user1)
          group.add(user2)
          group_user1 = user1.group_users.first
          group_user2 = user2.group_users.first

          put "/groups/#{group.id}.json", params: {
            group: {
              default_notification_level: NotificationLevels.all[:tracking]
            }
          }

          expect(response.status).to eq(200)

          expect(group_user1.reload.notification_level).to eq(NotificationLevels.all[:watching])
          expect(group_user2.reload.notification_level).to eq(NotificationLevels.all[:watching])

          group_users = group.group_users
          expect(response.parsed_body["user_count"]).to eq(group_users.count)

          group_user1.update!(notification_level: NotificationLevels.all[:regular])

          put "/groups/#{group.id}.json", params: {
            group: {
              default_notification_level: NotificationLevels.all[:tracking]
            }
          }

          expect(response.status).to eq(200)
          expect(response.parsed_body["user_count"]).to eq(group.group_users.count - 1)
          expect(group_user1.reload.notification_level).to eq(NotificationLevels.all[:watching])
          expect(group_user2.reload.notification_level).to eq(NotificationLevels.all[:watching])

          put "/groups/#{group.id}.json", params: {
            group: {
              default_notification_level: NotificationLevels.all[:tracking]
            },
            update_existing_users: true
          }

          expect(response.status).to eq(200)
          expect(response.parsed_body["success"]).to eq("OK")
          expect(group_user1.reload.notification_level).to eq(NotificationLevels.all[:tracking])
          expect(group_user2.reload.notification_level).to eq(NotificationLevels.all[:tracking])

          put "/groups/#{group.id}.json", params: {
            group: {
              default_notification_level: NotificationLevels.all[:regular]
            },
            update_existing_users: false
          }

          expect(response.status).to eq(200)
          expect(response.parsed_body["success"]).to eq("OK")
          expect(group_user1.reload.notification_level).to eq(NotificationLevels.all[:tracking])
          expect(group_user2.reload.notification_level).to eq(NotificationLevels.all[:tracking])
        end

@vinothkannans I wrote up how I would go about writing the tests which asserts for the state of the resources which we’re interested in. In particular, I think the difference in the response when a user has been updated vs when a user has not been updated is confusing and I hope we can fix that at some point.

Anything blocking a merge here? I am open to a followup PR if we must. @vinothkannans / @tgxworld ?

@SamSaffron it just requires few changes in tests. Either I will push it today or I will do the updates in a follow-up PR.

@tgxworld I committed your suggession with a fix. Thank you! :+1: