Course settings refactoring #108

pstaabp · 2022-08-06T19:22:10Z

This replaces #106 with better rebasing against main.

This reworks the way course settings are handled on the back end. Specifically

store the global/default course settings in the database
store the override course settings as a single row in a table (much simpler than the old method)

There are new routes for fetching/updating/deleting global and course settings.

Because of this, the settings modules on the front end side changes to reflect the back end changes. Including

inheriting from the Model class for both global settings and course settings (overrides)
in the settings store, the global_settings and db_course_settings are stored separately (similar to other data in the stores) and then merged.

There are a lot more tests now including

t/db/002_course_settings.t is rewritten to reflect the new course settings back end database model.
t/mojolicious/015_course_settings.t is a new to test all of the routes
tests/unit-tests/settings.ts tests the new model and handling of exceptions.
tests/stores/settings.ts tests the settings store.

The UI is mostly unchanged except for using some new store getters and now has the ability to save course settings.

To test

run t/db/build_db.pl
prove -r t
npm run test
npm run test-store
The UI--mainly the Settings view as an instructor. Changes are sent immediately to the database and you should see a confirmation notification.

Note: I think GitHub is not accurately managing the changes. A git diff between this and main is 20 files, mostly related to settings.

pstaabp · 2022-08-10T17:07:43Z

I notice I should have posted as a draft. The UI is mostly working now. A couple things I'd like to do:

switch any time_duration setting to a number of seconds in the database and let the UI handle the parsing of strings (like 15 mins to 900) and vise versa.
Make sure the permissions are working for fetch/put settings.
There should be a way to override the default settings like all other config files.

pstaabp · 2022-08-12T10:53:48Z

This is ready to go now. One question. In the db for both global settings (defaults) and course settings (overrides), the value is stored as a JSON in the form {value => ....} because the value can be many different types. to handle this, I manually inflate (insert and pull out of the hash)--it only happens in a few places, but should I do something more generally? I haven't done that because it seems like it would be replacing the JSON::Serializer that is used now. Thoughts?

WIP: work on course_settings WIP: continued work on the course settings WIP: continued work on course_settings. TEST: mojolicious route testing for settings. WIP: continued work on course settings

perltidy

WIP: allow default settings to be defined in an override file.

…on UI.

WIP: fix up tests and general cleanup. WIP: fix up tests and general cleanup. FIX: errors after rebase

drdrew42

Initial read-through and feedback on the db side of this PR

lib/DB/Schema/ResultSet/Course.pm

WIP: create inflator and subsequent code cleanup.

drdrew42

Just a few more cleanup things that arise as a result of simplifying with the new inflate method.

places where we needed a variable, now we can just return instead
avoid unnecessary -> when traversing nested hashes

drdrew42 · 2022-08-29T16:25:09Z

lib/DB/Schema/ResultSet/Course.pm

+		message => 'The course setting with '
+			. (
+				$args{info}->{setting_name} ? " name: '$args{info}->{setting_name}'"
+				: "setting_id of $args{info}->{setting_id} is not a found in the course "


"is not a found"

drdrew42 · 2022-08-29T16:26:07Z

lib/DB/Schema/ResultSet/Course.pm

 	my $global_setting = $self->getGlobalSetting(info => $args{info}, as_result_set => 1);
 	DB::Exception::SettingNotFound->throw(
-		message => "The setting with name: '" . $args{info}->{setting_name} . "' is not a defined info.")
+		message => "The global setting with name: '" . $args{info}->{setting_name} . "' is not a defined info.")


What does this exception mean? "is not a defined info"

drdrew42 · 2022-08-29T16:29:27Z

lib/DB/Schema/ResultSet/Course.pm

+	DB::Exception::SettingNotFound->throw(
+		message => 'The course setting with '
+			. (
+				$args{info}->{setting_name} ? " name: '$args{info}->{setting_name}'"


$args{info}{setting_name} does not require any thin arrows. Thin arrows are only needed when de-referencing the initial HASH ref (or making method calls). This is a pattern I think we all picked up from working on ww2, it's not needed.

Fixed all of these.

drdrew42 · 2022-08-29T16:30:43Z

lib/DB/Schema/ResultSet/Course.pm

@@ -323,7 +319,6 @@ sub getGlobalSetting ($self, %args) {
 		unless $global_setting;
 	return $global_setting if $args{as_result_set};
 	my $setting_to_return = { $global_setting->get_inflated_columns };
-	$setting_to_return->{default_value} = $setting_to_return->{default_value}->{value};


we can ditch the line above this one as well and just return { $global_setting->get_inflated_columns };

drdrew42 · 2022-08-29T16:31:58Z

lib/DB/Schema/ResultSet/Course.pm

+		? { $up_setting->get_inflated_columns, $up_setting->global_setting->get_inflated_columns }
+		: { $up_setting->get_inflated_columns };


here we can also just return instead of creating $setting to return

drgrice1

I have reviewed the API changes in this pull request. At this point this pull request is no where close to being ready to go. This needs to be completely redesigned. The structure is convoluted and doesn't make any sense.

There is a huge problem with webwork3 in its entirety at this point. Nothing is coming together into a coherent whole. Every pull request makes things more structurally messy instead of unifying things.

The JSONValue.pm concept ... see my comments. See @drdrew42's comments. It is time for you to stop putting in pull requests so hastily. Take some time and think things through. Spend time testing them and developing better structure.

When the API portion of this is starting to look more reasonable, then I will look at the rest.

drgrice1 · 2022-08-29T23:03:13Z

lib/DB/Schema/Result/CourseSetting.pm

+	setting_id => {
+		data_type => 'integer',
+		size      => 16,
+	},


This is a bad name for this column, and the description above is equally poor. At first glance it is unclear why course_setting_id and setting_id columns both exist in this table, and the description doesn't clear that up. A better name would be global_setting_id, and a better description would be "database id of the global setting that the given setting is related to".

The primary key of the global_setting table should also be changed to be the global_setting_id as the current key is inconsistent with the primary key naming used in tables in general in what we have so far of the ww3 database.

drgrice1 · 2022-08-29T23:11:39Z

lib/DB/Schema/ResultSet/Course.pm

+	my @settings_to_return = map {
+		$args{merged}
+			? { $_->get_inflated_columns, $_->global_setting->get_inflated_columns }
+			: { $_->get_inflated_columns };
+	} @settings_from_db;
+	return \@settings_to_return;


Just return the map ... statement. There is no need for the @settings_to_return variable. Change this to

Suggested change

my @settings_to_return = map {

$args{merged}

? { $_->get_inflated_columns, $_->global_setting->get_inflated_columns }

: { $_->get_inflated_columns };

} @settings_from_db;

return \@settings_to_return;

return [

map {

$args{merged}

? { $_->get_inflated_columns, $_->global_setting->get_inflated_columns }

: { $_->get_inflated_columns };

} @settings_from_db

];

It looks like there are many other places where this needs to be done also. I commented on a few above, but there are several others in this file (and probably elsewhere in the code). Don't create a ..._to_return variable, and then immediately return it. Just return the object that you assign to that variable.

This is something I told you about before, and applies to almost all languages. Don't create variables that are used only once. Just use whatever it is that you assign to that variable directly.

I've been try to return arrayrefs from these functions and doesn't map just create an array? This is why I did this.

That doesn't change anything with the point here. No single use variables. I fixed my suggestion so that an array ref is returned.

Edit: Now it returns an array ref (it was temporarily a hash ref with potentially an odd number of entries).

Even better is to not check the $args{merged} value for each entry of the @settings_from_db array. That is constant for the iteration, and so this should be

return $args{merged} ? [ map { { $_->get_inflated_columns, $_->global_setting->get_inflated_columns } } @settings_from_db ] : [ map { { $_->get_inflated_columns } } @settings_from_db ];

This is what I needed. Switched to this.

drgrice1 · 2022-08-29T23:16:49Z

lib/DB/Schema/ResultSet/Course.pm

+	my @settings = map {
+		{ $_->get_inflated_columns };
+	} @global_settings;
+	return \@settings;


See my comment later in this file. @settings is not needed.

drgrice1 · 2022-08-29T23:17:13Z

lib/DB/Schema/ResultSet/Course.pm

+	my $setting_to_return = { $global_setting->get_inflated_columns };
+	return $setting_to_return;


Again, see later comment.

drgrice1 · 2022-08-29T23:20:53Z

lib/DB/Schema/ResultSet/Course.pm

+	my $course_setting = $course->course_settings->find({
+		setting_id => $global_setting->{setting_id}
+	});


This could be consolidated to one line. It is just as readable, and it is always a good idea to make methods shorter by doing so.

drgrice1 · 2022-08-29T23:40:45Z

lib/DB/Utils.pm

+=pod
+=head2 convertTimeDuration
+
+This subroutine converts time durations stored as a string in human-readable format
+to a number of seconds.
+
+=cut
+
+sub convertTimeDuration ($time_duration) {
+	if ($time_duration =~ /^(\d+)\s(sec)s?$/) {
+		return $1;
+	} elsif ($time_duration =~ /^(\d+)\s(min(ute)?)s?$/) {
+		return $1 * 60;
+	} elsif ($time_duration =~ /^(\d+)\s(h(ou)?r)s?$/) {
+		return $1 * 60 * 60;
+	} elsif ($time_duration =~ /^(\d+)\s(day)s?$/) {
+		return $1 * 60 * 60 * 24;
+	} elsif ($time_duration =~ /^(\d+)\s(week)s?$/) {
+		return $1 * 60 * 60 * 24 * 7;
+	} else {
+		return 0;
+	}
+}
+
+=pod
+=head2
+
+This coverts a number of seconds to a human-readable format
+
+=cut
+
+sub humanReadableTimeDuration ($td) {
+	my $times = {
+		week => int($td / 604800),
+		day  => int($td % 604800 / 86400),
+		hour => int($td % 86400 / 3600),
+		min  => int($td % 3600 / 60),
+		sec  => $td % 60
+	};
+
+	my $time_duration = '';
+	# Order is important so the keys are defined.
+	for (qw/week day hour min sec/) {
+		my $val = $times->{$_};
+		$time_duration .= ($time_duration ne '' && $val ? ', ' : '')
+			# pluralize for more than 1.
+			. ($val > 0 ? "$val $_" . ($val == 1 ? '' : 's') : '');
+	}
+	return $time_duration;
+}


This should be deleted and dealt with client side. The database (or the api in general) should not be dealing with anything related to "human readability". The database should return a unix timestamp (or at least a ISO timestamp). The client UI should handle converting that into a human readable thing.

The reason I have these is to have human-readable in the configuration files and these subroutines are used to translate that. Also, humanReadableTimeDuration is used in tests. If we keep this, this should be moved to the t/lib directory.

This is something that need to reconsidered even deeper. What settings here are being saved in config files? If a setting needs conversion to become readable then it probably is being done wrong. Basically, it is probably incorrect to have a date at all in a config file. Time durations can be purely in seconds (or minutes or whatever flat time unit) and still work perfectly well in a config file and be human readable.

My thought with this is that time durations, it's easier to think in terms of minutes or days often instead of numbers of seconds (although clearly easy to calculate), so providing this functionality is relatively simple. Again, we can chat more about this.

drgrice1 · 2022-08-30T00:53:41Z

lib/DBIx/Class/InflateColumn/JSONValue.pm

+	return;
+}
+
+1;


NO. Find a different way of accomplishing this. This is conceptually bad, and is not a robust data storage design. Absolutely not. Ugh! This poor design is part of what led to @drdrew42's comments on the {default_value}{value} in his review.

drgrice1 · 2022-08-30T00:59:01Z

lib/WeBWorK3/Controller/Settings.pm

+	my $settings = $c->schema->resultset('Course')->getGlobalSettings();
+	$c->render(json => $settings);
+	return;


This file is filled with more examples of unnecessary variable declarations. Just put $settings into the only place it is used. This method can be made quite concise by changing it to

Suggested change

my $settings = $c->schema->resultset('Course')->getGlobalSettings();

$c->render(json => $settings);

return;

$c->render(

json => $c->schema->resultset('Course')->getCourseSettings(

info => { course_id => int($c->param('course_id')) },

merged => 1

)

);

return;

Repeat throughout this file.

This looks good. In a future PR I can change the other Controller modules to do this as well.

Note this comment was meant to be for lines 35-42 in this file. Not lines 19-21.

Although the concept applies here too.

drgrice1 · 2022-08-30T01:28:37Z

I also am not convinced that the setup with both the permissions and course settings is a good idea. Having a YAML file that has these settings that must then be loaded into the database will be a system administrative nightmare. When you want to change a single setting for the site, then you have to modify the YAML file, then load it into the database. If things go wrong with that, then the entire site is in trouble. This will lead to an excessive need to constantly back up the database and deal with recovery in case of failure.

Instead it would make more sense for the YAML files to directly be the global defaults. The database should then store overrides for a course.

drgrice1 · 2022-08-30T03:04:36Z

lib/DBIx/Class/InflateColumn/JSONValue.pm

+			inflate => sub {
+				my $str = shift;
+				# This is a bit of a hack.  It appears that sometimes the deflate isn't called on the values
+				# of type string and number so they don't need to be decoded.


Did you think to try the allow_nonref option for the JSON serializer instead of this ugly hack?

I had tried this. It didn't seem to work, but right now, I don't recall why. I will try using allow_nonref again.

I realized the trouble with using JSON and allow_nonref was that of parsing empty strings. I've switched this back to using these two and working on handling empty strings. This is definitely the better way to go.

What is the issue with parsing empty strings? Also, what do you mean you switched bak to using "these two"? What two? And, what are you saying is definitely the better way to go?

Sorry for the confusion. I'll try to be more clear. I think this was all an issue with the InflateColumn package. I have switched over to using FilterColumn, which looks like the way to go.

pstaabp · 2022-08-30T10:28:33Z

Having a YAML file that has these settings that must then be loaded into the database will be a system administrative nightmare.

I was looking at this as similar to what we decided to do with the permissions, that is move them into the database. We can talk about this on Wednesday.

drgrice1 · 2022-08-30T10:35:30Z

Having a YAML file that has these settings that must then be loaded into the database will be a system administrative nightmare.

I was looking at this as similar to what we decided to do with the permissions, that is move them into the database. We can talk about this on Wednesday.

Yeah, that is my whole point. I listed permissions here, and I am saying that was a bad idea. I was not involved in that decision, and if I was I would have told you to go a different direction.

pstaabp · 2022-08-30T15:35:55Z

I don't think this is too late to switch without changing the API.

…Column FIX: setting_id -> global_setting_id and switching inflator to FilterColumn

pstaabp · 2022-08-31T11:28:57Z

Made all of the requested changes. The inflator for settings (either default_value or value) is now using FilterColumn, which has more flexibility over InflateColumn.

One argument for keeping the settings (and permissions) in the database is that they are checked before adding to the database, ensuring the types are correct. If they in a file, it would be simply parsed or checked on each file read.

drgrice1 · 2022-08-31T12:17:38Z

The problem is that you are thinking we need yml files at all for this. Better would be a administrative user interface for editing roles, and no yml files at all.

drgrice1 · 2022-08-31T12:25:02Z

For course settings there are already big problems with the yml file approach. The description and doc should not be in the yml file or database at all. Those should be implemented in the client ui. Part of the problem is that if those are API side, they will not work with translations. Having these in the config files was a huge mistake made for webwork2 to begin with not only for translations. Those are things that really should never be configurable. If they are in defaults.config, then a config file loaded later can override them, and that shouldn't be possible. For webwork2 they should have been in the code and added to the hash objects later. Hard coded and not user or even sys admin modifiable.

pstaabp · 2022-08-31T15:30:43Z

The description and doc should not be in the yml file or database at all. Those should be implemented in the client ui. Part of the problem is that if those are API side, they will not work with translations.

Great point and should either include a translation key or perhaps create a key from the setting_name.

Your other points are also good to think about moving forward. It is convenient at this point of having a single file with all of the settings. I think the settings will adapt quite a bit as we develop ww3. My hope was to have a flexible system to handle this, but I understand after we have nailed this down, allowing overrides for global settings provides difficulties.

pstaabp mentioned this pull request Aug 6, 2022

Rework the Course Settings #106

Closed

pstaabp force-pushed the course-settings-update branch from 045cec1 to 12689c9 Compare August 9, 2022 14:16

pstaabp force-pushed the course-settings-update branch 2 times, most recently from bf29ba3 to cf472ea Compare August 12, 2022 10:49

pstaabp force-pushed the course-settings-update branch from cf472ea to 4c26e8f Compare August 24, 2022 20:38

pstaabp added 10 commits August 26, 2022 16:50

WIP: initial checkin of some changes to handling settings.

caa5017

WIP: work on course_settings WIP: continued work on the course settings WIP: continued work on course_settings. TEST: mojolicious route testing for settings. WIP: continued work on course settings

WIP: work on course settings

d9bb5c6

FIX: errors after rebasing.

6db7fd1

FIX: update code/tests after rebase with main.

b8df850

WIP: settings view on the UI should now work for all setting types.

60d0405

perltidy

4312f56

perltidy

TEST: updated tests to include checking permissions

24b4a2c

WIP: allow default settings to be defined in an override file.

2d7409c

WIP: allow default settings to be defined in an override file.

WIP: use non-neg ints in db for time_durations and human readability …

d3ee1bb

…on UI.

WIP: fix up tests and general cleanup.

0fab2c4

WIP: fix up tests and general cleanup. WIP: fix up tests and general cleanup. FIX: errors after rebase

pstaabp force-pushed the course-settings-update branch from 4c26e8f to 0fab2c4 Compare August 28, 2022 09:54

drdrew42 reviewed Aug 28, 2022

View reviewed changes

WIP: create inflator and subsequent code cleanup.

259ff87

WIP: create inflator and subsequent code cleanup.

pstaabp force-pushed the course-settings-update branch from 212adc9 to 259ff87 Compare August 29, 2022 13:42

drdrew42 reviewed Aug 29, 2022

View reviewed changes

drgrice1 requested changes Aug 30, 2022

View reviewed changes

drgrice1 reviewed Aug 30, 2022

View reviewed changes

FIX: setting_id -> global_setting_id and switching inflator to Filter…

4f6069e

…Column FIX: setting_id -> global_setting_id and switching inflator to FilterColumn

pstaabp force-pushed the course-settings-update branch from 1b99d98 to 4f6069e Compare August 31, 2022 11:26

		? { $up_setting->get_inflated_columns, $up_setting->global_setting->get_inflated_columns }
		: { $up_setting->get_inflated_columns };

		my $setting_to_return = { $global_setting->get_inflated_columns };
		return $setting_to_return;

+              	return;
+              }
+;

Course settings refactoring #108

Are you sure you want to change the base?

Course settings refactoring #108

Conversation

pstaabp commented Aug 6, 2022

pstaabp commented Aug 10, 2022 • edited Loading

pstaabp commented Aug 12, 2022

drdrew42 left a comment

Choose a reason for hiding this comment

drdrew42 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

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

drgrice1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drgrice1 Aug 29, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drgrice1 Aug 30, 2022 • edited Loading

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

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

drgrice1 Aug 30, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drgrice1 Aug 30, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drgrice1 commented Aug 30, 2022

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

pstaabp commented Aug 30, 2022 • edited Loading

drgrice1 commented Aug 30, 2022

pstaabp commented Aug 30, 2022

pstaabp commented Aug 31, 2022

drgrice1 commented Aug 31, 2022

drgrice1 commented Aug 31, 2022

pstaabp commented Aug 31, 2022

pstaabp commented Aug 10, 2022 •

edited

Loading

drgrice1 Aug 29, 2022 •

edited

Loading

drgrice1 Aug 30, 2022 •

edited

Loading

drgrice1 Aug 30, 2022 •

edited

Loading

drgrice1 Aug 30, 2022 •

edited

Loading

pstaabp commented Aug 30, 2022 •

edited

Loading