Improvement to the general models #89

pstaabp · 2022-05-23T21:09:55Z

This builds on #88 which handles booleans on the server.

There are a number of related changes:

It is now assumed that that the mojolicious server sends proper data types (booleans, numbers, strings), encoded in JSON objects. On the client-side, the models now use expected datatypes. For example, previously, a boolean was parsed as either a string, number or boolean, now a field now just has a boolean type.
instead of parsing as models/objects are created with exceptions throw when bad data is passed, a model can store anything with its given datatype. For example a username can take on the invalid value: 'the user'
All models have a isValid method which return a boolean on whether or not the model is valid. For example, if the username is 'the user' the method isValid will return false.
The CourseUser model has a validate method which both validates any relevant field and otherwise returns a error string. This is helpful for adding users to courses (for example, via loading a file).
The enable_reducing_scoring is moved from the set_params of a HomeworkSet to the set_dates field, since that is needed for determining homework dates are valid or not.
The tests have all been 1) changed to reflect the new way that models are handled and 2) made more robust to ensure more thorough testing is done.

…ProblemSets and UserSets.

…sts to work (only in ET).

pstaabp · 2022-06-02T19:27:31Z

I realize there is git hub action error. It's a perlcritic thing and not sure how to avoid doing line 398 of lib/DB/Schema/ResultSet/UserSet.pm. This has to do with using an block for eval instead of eval(''...stuff to eval...")

…set_params to set_dates.

…store and set to the database. TEST: adding tests to ensure that only valid models are added to the store and set to the database.

FIX: cleanup and using true/false in tests.

mgage · 2022-07-15T16:22:38Z

you still cannot at a user. The name and role fields work, but the other fields (first and last name etc.) are closed and blank and submitting the new user raises a complaint that not all of the parameters have been supplied.

… issues too.

mgage · 2022-07-15T19:07:50Z

the recent changes help.
It now allows me to register gage as an instructor in the arithmetic course, but it does not allow me to fill in first_name, last_name, email or StudentID all of which appear blank for gage.

mgage · 2022-07-21T19:39:23Z

everything compiled after I pulled in the latest changes from PR#89 (in openwebwork/webwork3)

adding users.
I was able to add a user gage to lisa’s arithmetic course, but I can modify, name or email or ID. This makes some sense if you are using a separate database and/or dashboard to record/enter site users. In that case you should not be able to enter the user name either, but be forced to choose the user to enroll in the course from a drop down menu. You are allowed to choose section, recitation and role, which makes sense since these might depend not only on the user but on the course they are enrolling in.

It would be good to get the site user dashboard management system up and running soon, since it will interact substantially with the classlist manager.
Presumably to change the first/last name, email and studentID you would need to do this from the site user dashboard. Are there any use cases where a student would want a different name in different classes? How about different emails for different classes? If the site is used by more than one University perhaps the studentID would be different for different courses taken from different universities. If the site is used by more than one university securing a unique username for the student might be problematic. It would be worth referencing moodle, d2l, canvas to see how they handle these conflicts.
I can't do a good analysis of the classlist module until it's clear how it interacts with the site_user database.

The data I entered (new user gage, as instructor) was saved in the database and I could log in to the class using the name gage and add problems to hmwk set #2. I don't know if this data is stored on the server in some way that it will stick around.

running unit tests: prove -r t Got errors in 003_users_t (2 failures), 009_problems.t (3 failures)

after rebuilding the database I got no errors in the unit tests, but I could no longer log in as gage.

mgage · 2022-07-21T19:40:55Z

I reestablished myself as an instructor and added problems to hmwk set #2. Unit tests now give me:
provel -r t
.....
t/db/009_problems.t ............... 1/?
# Failed test 'getGlobalProblems: get all problems'
# at t/db/009_problems.t line 54.
# Structures begin differing at:
# $got->[17] = Does not exist
# $expected->[17] = HASH(0x561309ef5888)

# Failed test 'The set_problems table is returned to its original state.'
# at t/db/009_problems.t line 395.
# Structures begin differing at:
# $got->[17] = Does not exist
# $expected->[17] = HASH(0x561309f62940)

# Failed test 'check: Ensure that the set_problems table is restored.'
# at t/db/009_problems.t line 403.
# Structures begin differing at:
# $got->[17] = Does not exist
# $expected->[17] = HASH(0x561309f3e228)
# Looks like you failed 3 tests of 28.
t/db/009_problems.t ............... Dubious, test returned 3 (wstat 768, 0x300)
Failed 3/28 subtests
.....

mgage · 2022-07-21T20:23:59Z

how do you run the lint code base, unit tests and perltidy on the pull request? Is this done automatically?

pstaabp · 2022-07-21T20:26:31Z

The tests require that you have a clean run of build_db.pl and by adding a user, the tests will fail. It's comparing the users in the database with the users in the files in the t/db/sample_data folder. If you have a clean run of the perl tests after the build_db.pl script, the tests work.

For linting the javascript/typescript/css/vue files: npm run lint
I usually run perltidy with perltidy lib/**/*.pm bin/**/*.pl t/**/*.t t/**/*.pl

Linting, perltidy, and the run of the tests are performed on a push to GitHub and you can see that the last commit ran successfully.

drgrice1

I started to try to review this, see the few comments I left. However, there is something wrong with this branch. You need to merge main into this one. Hopefully that will fix it. Github's diff is not matching what I see when I look at actual diffs on the files.

drgrice1 · 2022-07-21T22:55:38Z

.eslintrc.js

 	'keyword-spacing': ['error'],
 	'space-before-blocks': ['error', 'always'],
 	'arrow-spacing': ['error'],
+	'template-curly-spacing': ['error', 'never'],


I have tested merging main into this branch or merging this branch into main, and in either case this being added here results in this rule being duplicated. Clearly you need to merge the main branch into this one, and do some clean up.

It is good to group related rules, so it is better where the rule is in main up above by object-curly-spacing.

Merged in main and deleted this.

drgrice1 · 2022-07-21T23:02:10Z

lib/DB/Schema/Result/UserSet.pm

 );

-my $set_type = {
+our $set_type = {


Why is this changed to our? Generally, in modern perl you don't want our at all. Is this variable even used? It doesn't look like it is.

I'll look at this again. There was some issue that it wasn't visible in a subclass I think.

I get errors when it accesses line 166 of Result::UserSet.pm, for reverse mapping the number of the subclass with the type.

If you have some other ways of doing this, I welcome them.

As far as I can tell, it is not an issue with our versus my. The $set_type variable just needs to be deleted. It is not used anywhere. I deleted it, and all tests still pass. The front end still works as well as it currently does. Why is it here?

drgrice1 · 2022-07-21T23:05:34Z

lib/DB/Schema/Result/UserSet/HWSet.pm


+sub optional_fields_in_dates ($=) {
+	return DB::Schema::Result::ProblemSet::HWSet::optional_fields_in_dates();
+}


This doesn't make any sense, and needs to be rethought. It doesn't make sense to have optional non-date fields in the dates.

The JSON column structure is going to need to be thought out better. Right now there are several bad design ideas going on with that. The original idea was for these to be for non-essential uncommon things. Not dates that are pretty well mapped out. Furthermore, this is making the implementation rather convoluted.

I moved the enable_reduced_scoring into the date structure so checking date validity is self-contained for cases of reduced scoring. This solved the issue of needing to pass in enable_reduced_scoring from the set_params field to check the validity. In order to not hard-code this field into the dates, I added this other method.

Let me know if you have some better thoughts on this or we can revert back to the way I handled it before.

Although I'm crazy about having a non-date field inside this, it does make a lot of other things easier to handle now.

taniwallach · 2022-07-22T09:18:28Z

I'm not up to speed on WW3, but started to look around to get an idea what is going on.

Why useMathQuill when webwork2 allows choosing from multiple entryAssist options?
At present MathQuill may be the only option relevant for webwork3, but in the future something else might also be relevant.
Maybe use something like a C enum where at present one setting would be none and the other MathQuill.

pstaabp · 2022-07-22T13:27:00Z

Why useMathQuill when webwork2 allows choosing from multiple entryAssist options?
At present MathQuill may be the only option relevant for webwork3, but in the future something else might also be relevant.
Maybe use something like a C enum where at present one setting would be none and the other MathQuill.

I feel like we're just getting started on some of the options like this for ww3. The overall structure is nearly there and is quite flexible, so if we need to adjust fields like this, it shouldn't be too difficult.

drgrice1 · 2022-07-22T15:50:59Z

lib/DB/TestUtils.pm

+		} elsif (grep { $_ eq $key } @{ $config->{non_neg_float_fields} }) {
 			$output->{$key} = 0 + $input->{$key} if defined($input->{$key});
 		} else {
 			$output->{$key} = $input->{$key};


This file has bugged me for a while. Why is this part of the main lib? It would be in the t directory.

I can move this.

drgrice1

It is time that you start focusing your work. There is a whole mesh of changes here, most of which do not fall under the title of improving general models, and that is why so many files were changed.

There are a lot of changes to the JSON values that should have been in the pull request before related to that. So that pull request should have been completed before you started this one.

There are changes of double to single quotes. Finish that in a separate pull request next time.

It would make it much easier for us to review your work if it were focused on the real point.

pstaabp · 2022-07-22T17:26:20Z

I've been trying to get more focused on these PRs and will do better. I realize that such large PRs are hard to review.

drdrew42

TODO:

remove our $set_type declaration from lib/DB/Schema/Result/UserSet.pm
move TestUtils.pm out of lib/DB/ and into t/lib? (will need to use the testing lib directory)
dates remain in JSON field for now -- but we should all discuss pros/cons of this approach before getting into scoring (or other features that rely on dates)

…able in UserSet.pm

drgrice1

This looks good for now. I will merge it once the tests complete.

pstaabp added 15 commits May 16, 2022 06:26

WIP: change Course model to handle fields

b25c5a6

WIP: updating courses and generic problem sets on improvements.

e0de746

WIP: more work on switching models.

b7ff6fb

WIP: work on getting the UI simpler with newer models.

fc7dd5d

WIP: fixed loading of users by file.

bc83691

WIP: cleanup

bd8538b

Merge branch 'db-send-json-boolean' into model-improvements

2b60eaf

FIX: set_visible for problem_sets was not a JSON boolean.

887f84f

WIP: Move the enable_reduced_scoring from set_params to set_dates in …

6ab7ec4

…ProblemSets and UserSets.

WIP: contined work on the models and tests.

9280563

Merge branch 'db-send-json-boolean' into model-improvements

9265b45

FIX: for dates in csv files, remove time-zone and also hack to get te…

e138f0a

…sts to work (only in ET).

FIX: store tests

ed81b0e

FIX: refactor the way csv files are loaded in the perl test files.

133098c

FIX: linting and perlcritic cleanup.

d3fb3f6

pstaabp added 11 commits June 8, 2022 16:41

FIX: updated UI to reflect that enable_reduced_scoring is moved from …

cbfab52

…set_params to set_dates.

Merge branch 'main' into model-improvements

0692d06

FIX: errors with tests after merge.

5c924d4

Merge branch 'db-send-json-boolean' into model-improvements

9af6bc2

TEST: adding tests to ensure that only valid models are added to the …

75080a6

…store and set to the database. TEST: adding tests to ensure that only valid models are added to the store and set to the database.

FIX: linting and typos

55a31fb

FIX: tests now return the database to it's original state.

d879ccd

FIX: linting

1edc7ab

Merge branch 'db-send-json-boolean' into model-improvements

e532ccf

FIX: testing errors

e328665

FIX: cleanup and using true/false in tests.

f6a0977

FIX: cleanup and using true/false in tests.

pstaabp mentioned this pull request Jun 21, 2022

Redo roles and permissions #95

Merged

pstaabp added 2 commits June 29, 2022 12:02

Merge branch 'db-send-json-boolean' into model-improvements

753e0be

WIP: check that enable_reduced_scoring is a boolean.

c02696c

pstaabp added 3 commits July 5, 2022 06:58

FIX: cleanup after merge

e4a5dc9

FIX: unit tests for review sets.

282c1ea

Merge branch 'db-send-json-boolean' into model-improvements

597aa7f

pstaabp mentioned this pull request Jul 8, 2022

permissions problems related to UI #99

Closed

pstaabp added 2 commits July 8, 2022 13:57

FIX: linting after merge.

7f6baed

FIX: perltidy

f688760

FIX: add users manually wasn't working. There was related model/store…

dbe2f02

… issues too.

pstaabp added 2 commits July 15, 2022 15:14

FIX: linting error.

9750fa0

FIX: make a server call only if the username is valid

89c5842

pstaabp mentioned this pull request Jul 20, 2022

Create an Instructor Dashboard #102

Closed

drgrice1 requested changes Jul 21, 2022

View reviewed changes

pstaabp added 2 commits July 22, 2022 09:30

Merge branch 'main' into model-improvements

14919ce

FIX: duplicated rule

5ce8918

drgrice1 reviewed Jul 22, 2022

View reviewed changes

drgrice1 approved these changes Jul 22, 2022

View reviewed changes

drdrew42 approved these changes Jul 22, 2022

View reviewed changes

pstaabp added 2 commits July 22, 2022 14:07

FIX: move TestUtils.pm to t/lib and remove unnecessary $set_type vari…

7731de8

…able in UserSet.pm

FIX: updated lib in build_db.pl script.

a365b0a

drgrice1 approved these changes Jul 22, 2022

View reviewed changes

drgrice1 merged commit 62fae8f into openwebwork:main Jul 22, 2022

pstaabp deleted the model-improvements branch July 24, 2022 15:19

Improvement to the general models #89

Improvement to the general models #89

Uh oh!

Conversation

pstaabp commented May 23, 2022

Uh oh!

pstaabp commented Jun 2, 2022

Uh oh!

mgage commented Jul 15, 2022

Uh oh!

mgage commented Jul 15, 2022

Uh oh!

mgage commented Jul 21, 2022

Uh oh!

mgage commented Jul 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mgage commented Jul 21, 2022

Uh oh!

pstaabp commented Jul 21, 2022

Uh oh!

drgrice1 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

taniwallach commented Jul 22, 2022

Uh oh!

pstaabp commented Jul 22, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

drgrice1 left a comment

Choose a reason for hiding this comment

Uh oh!

pstaabp commented Jul 22, 2022

Uh oh!

drdrew42 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

drgrice1 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mgage commented Jul 21, 2022 •

edited

Loading

drdrew42 left a comment •

edited

Loading