Make `autofollow_on_join_user` configuration a list #8430

abrahamparayil · 2023-06-15T05:01:52Z

Fixes: Make 'autofollow_on_join_user' configuration a list #4986

abrahamparayil · 2023-06-15T05:13:07Z

@denschub @SuperTux88 I am unable to run the specs locally. I am getting the following error:

WARNING: Namespace test not found in /home/avronr/worksapce/diaspora/config/diaspora.toml

The Jasmine Ruby gems are deprecated. There will be no further releases after
the end of the Jasmine 3.x series. We recommend that most users migrate to the
jasmine-browser-runner npm package, which is the direct replacement for the
jasmine gem. See <https://jasmine.github.io/setup/browser.html> for setup
instructions, including for Rails applications that use either Sprockets or
Webpacker.

If jasmine-browser-runner doesn't meet your needs, one of these might:

* The jasmine npm package to run specs in Node.js:
  <https://github.com/jasmine/jasmine-npm>
* The standalone distribution to run specs in browsers with no additional
  tools: <https://github.com/jasmine/jasmine#installation>
* The jasmine-core npm package if all you need is the Jasmine assets:
  <https://github.com/jasmine/jasmine>. This is the direct equivalent of the
  jasmine-core Ruby gem.

To prevent this message from appearing, set the SUPPRESS_JASMINE_DEPRECATION
environment variable.

RSpec is shutting down and will print the summary report... Interrupt again to force quit.

Couldn't find any documentation for running tests either could you folks help me out here? Thanks!

config/diaspora.toml.example

config/defaults.yml

app/models/user.rb

denschub · 2023-06-15T13:39:46Z

Welcome to this project, @abrahamparayil! :)

Couldn't find any documentation for running tests either could you folks help me out here?

Have a look at this wiki page which explains the generics. If you still run into issues, head over to our Discourse and create a new thread in the "Development" section, so we can debug and help there without adding too much "not related to this PR" comments here.

jhass

Good progress! A few more tests, a deprecation warning on boot time for the old setting and a changelog entry describing how to migrate the setting and then this looks good to me 😇

jhass · 2023-06-15T14:10:59Z

app/models/user.rb

+        autofollow_accounts.push(autofollow_user) if autofollow_user.present?
+
+        begin
+          autofollow_accounts.uniq.each { |user_id| follow_account(user_id, aq) } if autofollow_accounts.present?


Suggested change

autofollow_accounts.uniq.each { |user_id| follow_account(user_id, aq) } if autofollow_accounts.present?

autofollow_accounts.uniq.each { |user_id| follow_account(user_id, aq) }

Array.wrap will ensure it's an array already, the #push call above would already fail if it's not and each on an empty array is a noop. So I think we can do without the if condition :)

Resolved here: 25ae7a9.

jhass · 2023-06-15T14:12:02Z

app/models/user.rb

+
+        begin
+          autofollow_accounts.uniq.each { |user_id| follow_account(user_id, aq) } if autofollow_accounts.present?
+        rescue DiasporaFederation::Discovery::DiscoveryError


Should we fold the rescue into the loop (or follow_account that is) perhaps? That is wouldn't we want to attempt to follow the remaining users regardless of anyone failing?

Resolved here: 25ae7a9.

app/models/user.rb

spec/models/user_spec.rb

abrahamparayil · 2023-06-15T16:17:05Z

@denschub All tests are failing with the following error:

ActiveRecord::InvalidForeignKey:
  PG::ForeignKeyViolation: ERROR:  insert or update on table "aspect_memberships" violates foreign key constraint "aspect_memberships_aspect_id_fk"
  DETAIL:  Key (aspect_id)=(10) is not present in table "aspects".

Am I missing something here? 😅

@jhass I there a specific place (as a convention) to add boot time warnings within the project?

jhass · 2023-06-19T10:37:26Z

@abrahamparayil Sorry for the late reply. I think config/load_config.rb would be a good place for this one.

config/load_config.rb

abrahamparayil · 2023-06-19T12:11:13Z

@jhass No worries. I've added the warning. :)

spec/models/user_spec.rb

SuperTux88 · 2023-11-12T20:29:55Z

Changelog.md

+- New:
+```toml
+autofollow_on_join_accounts = "hq@pod.diaspora.software"
+  OR


Can you make this line a comment, so it doesn't show up as TOML-syntax error.

Suggested change

OR

# OR

SuperTux88 · 2023-11-12T20:50:06Z

app/models/user.rb

+      if AppConfig.settings.autofollow_on_join?
+        autofollow_user = AppConfig.settings.autofollow_on_join_user
+        autofollow_accounts = Array.wrap(AppConfig.settings.autofollow_on_join_accounts)
+        autofollow_accounts.push(autofollow_user) if autofollow_user.present?


I'm not sure if just adding the old autofollow_on_join_user to the new default value of autofollow_on_join_accounts is the correct thing to do. For podmins who didn't configure anything, nothing chnages, but podmins who manually set another account, it silently adds the HQ-account to the list (as it's the default value for the new setting), until they manually migrate their config to the new autofollow_on_join_accounts and decide if they want to include the HQ-account now (in addition to their account), or if they still want to keep it with only one account.

My expectations would have been, that if the old autofollow_on_join_user is set, it still behaves as it was before, so just following that one user. And only if the new autofollow_on_join_accounts is the only option used, it follows multiple users.

But maybe the behavior is fine, as this will be released in a major version with a big changelog entry, so we can expect podmins to read that and react accordingly if they don't want the HQ-Account to be added by default? 🤔

I'm fine with breaking things in major releases. It's not our fault if people don't read changelogs, and in this case, this seems like a worthy change.

SuperTux88 · 2023-11-12T20:58:32Z

spec/models/user_spec.rb

+            expect(Person).to
+            receive(:find_or_fetch_by_identifier).with(person.diaspora_handle).and_return(person)


This syntax (with receive on a separate line) breaks the tests (as it should be a parameter for to) , it needs to be like this:

Suggested change

expect(Person).to

receive(:find_or_fetch_by_identifier).with(person.diaspora_handle).and_return(person)

expect(Person).to receive(:find_or_fetch_by_identifier)

.with(person.diaspora_handle).and_return(person)

SuperTux88 · 2023-11-12T20:58:59Z

spec/models/user_spec.rb

+            expect(Person).to
+            receive(:find_or_fetch_by_identifier).with(person.diaspora_handle).and_return(person)


Same as above:

Suggested change

expect(Person).to

receive(:find_or_fetch_by_identifier).with(person.diaspora_handle).and_return(person)

expect(Person).to receive(:find_or_fetch_by_identifier)

.with(person.diaspora_handle).and_return(person)

SuperTux88 · 2023-11-12T21:04:49Z

spec/models/user_spec.rb

+          expect(Person).to
+          receive(:find_or_fetch_by_identifier).with(person.diaspora_handle).and_return(person)


This test is broken, as it expects the behavior I described above. It wants that only the autofollow_on_join_user (person) is followed, but since autofollow_on_join_accounts has a default value of the HQ account, it also tries to follow that one, which isn't expected by this test. So either change the behavior that when autofollow_on_join_user is set, it ignores the value of autofollow_on_join_accounts (which always has a default value set), or change the test, to also expect the default value of autofollow_on_join_accounts.

Also there is a syntax error with the receive on it's own line.

abrahamparayil added 5 commits June 15, 2023 10:20

Update model code

5930d75

Update default configuration

39ee7f7

Update example config

5cc3e58

Fix linting

02383ef

Update spec

24ebe36

abrahamparayil marked this pull request as ready for review June 15, 2023 05:24

jhass requested changes Jun 15, 2023

View reviewed changes

config/diaspora.toml.example Outdated Show resolved Hide resolved

config/defaults.yml Outdated Show resolved Hide resolved

app/models/user.rb Outdated Show resolved Hide resolved

abrahamparayil added 2 commits June 15, 2023 19:13

Remove existing config

16f699b

Update logic

38e13f6

jhass requested changes Jun 15, 2023

View reviewed changes

Added code refactor

25ae7a9

github-actions bot reviewed Jun 15, 2023

View reviewed changes

app/models/user.rb Outdated Show resolved Hide resolved

app/models/user.rb Outdated Show resolved Hide resolved

spec/models/user_spec.rb Outdated Show resolved Hide resolved

spec/models/user_spec.rb Outdated Show resolved Hide resolved

abrahamparayil added 2 commits June 15, 2023 20:25

Add changelog entry

8208518

Fix lints

7d8ee57

github-actions bot reviewed Jun 15, 2023

View reviewed changes

spec/models/user_spec.rb Outdated Show resolved Hide resolved

spec/models/user_spec.rb Outdated Show resolved Hide resolved

indentation fix

dafaf7e

Add warning on boot time

9593831

github-actions bot reviewed Jun 19, 2023

View reviewed changes

config/load_config.rb Outdated Show resolved Hide resolved

Fix indentation

b700add

Add more specs

a80e2e8

github-actions bot reviewed Jun 19, 2023

View reviewed changes

spec/models/user_spec.rb Outdated Show resolved Hide resolved

spec/models/user_spec.rb Outdated Show resolved Hide resolved

spec/models/user_spec.rb Outdated Show resolved Hide resolved

spec/models/user_spec.rb Outdated Show resolved Hide resolved

abrahamparayil added 2 commits June 19, 2023 18:29

Fix indentation

a2df942

Updated spec

4d97cbf

SuperTux88 requested changes Nov 12, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make `autofollow_on_join_user` configuration a list #8430

Make `autofollow_on_join_user` configuration a list #8430

abrahamparayil commented Jun 15, 2023

abrahamparayil commented Jun 15, 2023

denschub commented Jun 15, 2023

jhass left a comment

jhass Jun 15, 2023

abrahamparayil Jun 15, 2023

jhass Jun 15, 2023

abrahamparayil Jun 15, 2023

abrahamparayil commented Jun 15, 2023

jhass commented Jun 19, 2023

abrahamparayil commented Jun 19, 2023

SuperTux88 Nov 12, 2023

SuperTux88 Nov 12, 2023

denschub Nov 12, 2023

SuperTux88 Nov 12, 2023

SuperTux88 Nov 12, 2023

SuperTux88 Nov 12, 2023

	autofollow_accounts.uniq.each { \|user_id\| follow_account(user_id, aq) } if autofollow_accounts.present?
	autofollow_accounts.uniq.each { \|user_id\| follow_account(user_id, aq) }

		expect(Person).to
		receive(:find_or_fetch_by_identifier).with(person.diaspora_handle).and_return(person)

Make autofollow_on_join_user configuration a list #8430

Are you sure you want to change the base?

Make autofollow_on_join_user configuration a list #8430

Conversation

abrahamparayil commented Jun 15, 2023

abrahamparayil commented Jun 15, 2023

denschub commented Jun 15, 2023

jhass left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abrahamparayil commented Jun 15, 2023

jhass commented Jun 19, 2023

abrahamparayil commented Jun 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Make `autofollow_on_join_user` configuration a list #8430

Make `autofollow_on_join_user` configuration a list #8430