Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revised handling of PERSISTENCE_HASH data #1940

Merged
merged 1 commit into from
Apr 27, 2023

Conversation

taniwallach
Copy link
Member

Persistent problem data - directly into a new database field.
Paired with openwebwork/pg#809

@taniwallach
Copy link
Member Author

Note: The saving of data to the database is done right after a problem is rendered by a Problem.pm and GatewayQuiz.pm so that data set by an "initial" render gets saved. If a problem author does not want revised data to be saved on a Preview, the data in the PERSISTENCE_HASH should not be modified when processing a Preview.

The functionality provided here is used by the revised code of openwebwork/pg#506 .

Future use by a revised version of openwebwork/pg#482 is likely.

@taniwallach taniwallach marked this pull request as ready for review April 13, 2023 22:43
Copy link
Member

@drgrice1 drgrice1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be rethought a bit. Is there any real reason that the processing needs to be done where you have done it?

lib/WeBWorK/ContentGenerator/Problem.pm Outdated Show resolved Hide resolved
lib/WeBWorK/ContentGenerator/GatewayQuiz.pm Outdated Show resolved Hide resolved
lib/WeBWorK/Utils/Rendering.pm Outdated Show resolved Hide resolved
taniwallach added a commit to taniwallach/webwork2 that referenced this pull request Apr 19, 2023
Includes changes and suggestions from Dr. Glenn Rice.
See: openwebwork#1940
@taniwallach taniwallach requested a review from drgrice1 April 19, 2023 12:45
Copy link
Member

@drgrice1 drgrice1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good now. I need to do better testing of it yet though. I think the best way to do that will be by testing openwebwork/pg#506.

Copy link
Member

@drgrice1 drgrice1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did some testing with openwebwork/pg#506, and I think this is good.

@taniwallach taniwallach changed the base branch from develop to WeBWorK-2.18 April 22, 2023 19:19
@taniwallach
Copy link
Member Author

I just ran into a problem with this... I tried to view a problem (separate window) from the assignment editor for a question in the "main" assignment of a quiz, and got:

Can't call method "problem_data" on unblessed reference at /opt/webwork/webwork2/lib/WeBWorK/ContentGenerator/GatewayQuiz.pm line 1220.

This does not happen when using the link from a specific version of the quiz.
I'm looking into how to prevent this issue.

Includes changes and suggestions from Dr. Glenn Rice.
See: openwebwork#1940

Fixes a minor POD error in lib/WeBWorK/HTML/AttemptsTable.pm.
@taniwallach
Copy link
Member Author

Fix in the new version, by keeping track of when a "fake" set is being used, and not trying to save persistent data in that case.
A warning is issues, so if problem testing is attempted in such a case - the user will be aware that persistent data functionality is impaired in such a setting.
A minor fix to POD documentation was fixed in an unrelated file lib/WeBWorK/HTML/AttemptsTable.pm as the file/module was moved from WeBWorK/utils to WeBWorK/HTML but one location in the was not updated.

@taniwallach taniwallach requested review from pstaabp and removed request for pstaabp April 25, 2023 22:15
@pstaabp pstaabp merged commit 611855e into openwebwork:WeBWorK-2.18 Apr 27, 2023
drgrice1 added a commit to drgrice1/webwork2 that referenced this pull request Dec 17, 2024
This makes it so that persistence hash is only saved to the database
when answers are submitted. In the case that answers can be checked by
the user (via the "Check Answers" button), then the persistence hash is
saved JSON encoded in a hidden form field.  That case does not need the
security of saving to the database, and in that case it shouldn't be
saved to the database because answers aren't being recorded and the data
in that case should temporary. So a hidden form field is just right for
this.

I objected to the previous implementation when openwebwork#1940 was submitted, but
allowed it at that time, but this is how the problem_data should have
been implemented. Nothing should be written to the database for this
when answers are not being recorded.  If an instructor is acting as a
student, the previous code was saving the persistent data to the
student's problem even when the instructor just enters the problem.  Of
course it was also saving every time the instructor did anything with
the problem including using the "Preview My Answers" button, the "Check
Answers" button, the "Show Correct Answers" button, the "Show Problem
Grader" button.  Literally any form submission of the page.  That just
was not thought out.

The render_rpc (and html2xml) routes use the hidden form field approach
to also support saving the problem data from the persistence hash. This
means that problems that use the persistence hash can be tested in the
PG problem editor.

Note that the PERSISTENCE_HASH_UPDATE flag is removed.  That didn't
improve efficiency at all.  The processing that was done with that was
too much.  Even if this were saved when it was before it would have been
more efficient to just save it.

The handling of the persistence hash is also reworked for PG in a
corresponding pull request. PG just sends the hash, and it can contain
anything that can be JSON encoded.  Webwork2 just JSON encodes it and
stores it, but only when answers are submitted.
drgrice1 added a commit to drgrice1/webwork2 that referenced this pull request Dec 17, 2024
This makes it so that persistence hash is only saved to the database
when answers are submitted. In the case that answers can be checked by
the user (via the "Check Answers" button), then the persistence hash is
saved JSON encoded in a hidden form field.  That case does not need the
security of saving to the database, and in that case it shouldn't be
saved to the database because answers aren't being recorded and the data
in that case should temporary. So a hidden form field is just right for
this.

I objected to the previous implementation when openwebwork#1940 was submitted, but
allowed it at that time, but this is how the problem_data should have
been implemented. Nothing should be written to the database for this
when answers are not being recorded.  If an instructor is acting as a
student, the previous code was saving the persistent data to the
student's problem even when the instructor just enters the problem.  Of
course it was also saving every time the instructor did anything with
the problem including using the "Preview My Answers" button, the "Check
Answers" button, the "Show Correct Answers" button, the "Show Problem
Grader" button.  Literally any form submission of the page.  That just
was not thought out.

The render_rpc (and html2xml) routes use the hidden form field approach
to also support saving the problem data from the persistence hash. This
means that problems that use the persistence hash can be tested in the
PG problem editor.

Note that the PERSISTENCE_HASH_UPDATE flag is removed.  That didn't
improve efficiency at all.  The processing that was done with that was
too much.  Even if this were saved when it was before it would have been
more efficient to just save it.

The handling of the persistence hash is also reworked for PG in a
corresponding pull request. PG just sends the hash, and it can contain
anything that can be JSON encoded.  Webwork2 just JSON encodes it and
stores it, but only when answers are submitted.
drgrice1 added a commit to drgrice1/webwork2 that referenced this pull request Dec 19, 2024
This makes it so that persistence hash is only saved to the database
when answers are submitted. In the case that answers can be checked by
the user (via the "Check Answers" button), then the persistence hash is
saved JSON encoded in a hidden form field.  That case does not need the
security of saving to the database, and in that case it shouldn't be
saved to the database because answers aren't being recorded and the data
in that case should temporary. So a hidden form field is just right for
this.

I objected to the previous implementation when openwebwork#1940 was submitted, but
allowed it at that time, but this is how the problem_data should have
been implemented. Nothing should be written to the database for this
when answers are not being recorded.  If an instructor is acting as a
student, the previous code was saving the persistent data to the
student's problem even when the instructor just enters the problem.  Of
course it was also saving every time the instructor did anything with
the problem including using the "Preview My Answers" button, the "Check
Answers" button, the "Show Correct Answers" button, the "Show Problem
Grader" button.  Literally any form submission of the page.  That just
was not thought out.

The render_rpc (and html2xml) routes use the hidden form field approach
to also support saving the problem data from the persistence hash. This
means that problems that use the persistence hash can be tested in the
PG problem editor.

Note that the PERSISTENCE_HASH_UPDATE flag is removed.  That didn't
improve efficiency at all.  The processing that was done with that was
too much.  Even if this were saved when it was before it would have been
more efficient to just save it.

The handling of the persistence hash is also reworked for PG in a
corresponding pull request. PG just sends the hash, and it can contain
anything that can be JSON encoded.  Webwork2 just JSON encodes it and
stores it, but only when answers are submitted.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants