Skip to content

Create Model for Term Of Use acceptance - #977

Merged
dati18 merged 26 commits into
mainfrom
tou-backend
Oct 16, 2025
Merged

Create Model for Term Of Use acceptance#977
dati18 merged 26 commits into
mainfrom
tou-backend

Conversation

@dati18

@dati18dati18 commented Sep 25, 2025

Copy link
Copy Markdown
Contributor

This introduces:

  • two new models for Terms of Use Versions and Acceptance of Terms of Use
  • two DB migrations to support persisting them
  • a job to create the first Terms of Use Versions
  • a job to mark all existing users as accepting that version

In order to test or deploy this PR it's necessary to run both migrations.
To test the two jobs they need to be run in the order listed above since
a Terms of Use Version needs to exist before users can accept it.

Bug: T401165

@outdooracornoutdooracorn left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

PDOException: SQLSTATE[42S02]: Base table or view not found: 1146 Table 'apidb.user_term_of_use_acceptances' doesn't exist in /var/www/html/vendor/laravel/framework/src/Illuminate/Database/Connection.php:581

I wonder if these errors are because you haven't created a database migration?

@deer-wmde

Copy link
Copy Markdown
Contributor

I noticed when commenting out the V1 or V2 cases this check makes the logic silently skip them because these examples start with uppercase 'V' - code and tests run fine but the old versions still gets used without the test failing. I recommend to fix the spelling of the example comments and maybe even remove that check to be sure we won't fall into that trap in the future

outdooracorn
outdooracorn previously requested changes Oct 7, 2025
Comment threadapp/UserTermOfUseAcceptance.php Outdated
Comment threadapp/UserTermOfUseAcceptance.php Outdated
Comment threadapp/TermOfUseVersion.php Outdated
Comment threadapp/TermOfUseVersion.php Outdated
@dati18
dati18 requested a review from CopilotOctober 8, 2025 22:45

CopilotAI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull Request Overview

This PR implements a Terms of Use acceptance tracking system by creating a new model and database structure to record when users accept different versions of the terms of use.

  • Creates UserTermsOfUseAcceptance model with database migration to track user acceptance
  • Implements TermsOfUseVersion enum with versioning logic to manage different TOU versions
  • Adds automatic TOU acceptance creation during user registration and bulk processing job

Reviewed Changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.

Show a summary per file
FileDescription
app/UserTermsOfUseAcceptance.phpNew model for tracking TOU acceptances with proper casting and fillable fields
app/TermsOfUseVersion.phpEnum defining TOU versions with latest version detection logic
database/migrations/2025_09_29_194758_tou_acceptances.phpMigration creating tou_acceptances table with proper constraints
app/Jobs/UserTouAcceptanceJob.phpJob for bulk processing existing users to create TOU acceptances
app/Jobs/UserCreateJob.phpUpdated to automatically create TOU acceptance for new users
app/User.phpAdded relationship method for TOU acceptances
tests/Routes/User/UserTermsOfUseAcceptanceTest.phpTest validating TOU acceptance creation during user creation
tests/Jobs/UserTouAcceptanceJobTest.phpTest for bulk TOU acceptance job functionality

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment threadapp/User.php Outdated
Comment threadapp/Jobs/UserTouAcceptanceJob.php Outdated
Comment threadtests/Jobs/UserTouAcceptanceJobTest.php Outdated
Comment threadtests/Jobs/UserTouAcceptanceJobTest.php Outdated
Comment threadtests/Jobs/UserTouAcceptanceJobTest.php Outdated
Comment threadtests/Jobs/UserTermsOfUseAcceptanceTest.php
Comment threadapp/UserTermsOfUseAcceptance.php
Comment threadapp/UserTermsOfUseAcceptance.php
@tarrow

Copy link
Copy Markdown
Contributor

Please don't see any of these comments from me as blocking; feel free to charge forwards on Friday.

I'd also be fine if this PR just introduced the model and you followed up with the "Job" (or even another migration?) to record the acceptance data for the existing users if doing these two things at the same time is what makes it hard to get over the line.

Comment threadapp/UserTermsOfUseAcceptance.php Outdated
Comment threadapp/Jobs/UserTouAcceptanceJob.php Outdated
Comment threadapp/Jobs/UserTouAcceptanceJob.php
Comment threadapp/Jobs/UserTouAcceptanceJob.php
Comment threadapp/Jobs/UserTouAcceptanceJob.php Outdated
Comment threadapp/Jobs/UserTouAcceptanceJob.php Outdated
Comment threadapp/Jobs/CreateFirstTermsOfUseVersionJob.php Outdated
Comment threaddatabase/migrations/2025_10_14_091126_tou_versions.php Outdated
Comment threadapp/TermsOfUseVersion.php Outdated

@tarrowtarrow left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Approved; tested in a docker compose setup on my machine. I think this has been in flight quite long enough we ought to try it on staging soon :)

Comment threadapp/TermsOfUseVersion.php Outdated
@dati18
dati18 requested a review from tarrowOctober 15, 2025 18:10

@tarrowtarrow left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

cool; I think this is now all looking fairly decent!

@tarrow
tarrow dismissed outdooracorn’s stale reviewOctober 16, 2025 07:51

I think this has now all been addressed; sorry if I missed something Ollie

@dati18
dati18 merged commit fb17a47 into mainOct 16, 2025
5 checks passed
@dati18
dati18 deleted the tou-backend branch October 16, 2025 11:14

@outdooracornoutdooracorn left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think all my previous comments have been addressed, and this is looking good. However, I did just notice that the database schemas don't look quite right to me.

Schema::create('tou_acceptances', function (Blueprint $table) {
$table->id();
$table->unsignedInteger('user_id');
$table->string('tou_version', 10);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This doesn't seem to be linked to the tou_versions table.

public function up(): void {
Schema::create('tou_versions', function (Blueprint $table) {
$table->id();
$table->string('version')->unique();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

How come the tou_acceptances table uses string('version', 10) and this doesn't?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Also, as the tou_version string will always be 10 chars (yyyy-mm-dd), I wonder if we should be using $table->char('tou_version', 10) which is a fixed width CHAR(10) rather than $table->string('tou_version', 10) which is a variable width VARCHAR(10).

*/
public function up(): void {
Schema::create('tou_versions', function (Blueprint $table) {
$table->id();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is this ID ever used? It's not used in the tou_acceptances table as a foreign key. Although, I'm not sure if an ID is needed here. The version is already a unique identifier, and what is used in the tou_acceptances table, why not make that the primary key?

deer-wmde pushed a commit that referenced this pull request Dec 15, 2025
* Create Model for Term Of Use acceptance
* minor fix
* Add missing Migration for tou_acceptances table
* add lastest() method to get lastest version of ToU
* Add test case
* Add test for ToU acceptance when user was created
* Fix test
* fix linting errors
* fix linting errors
* fix linting errors
* Fix typo in classes' names
* Add Job to generate 'tou_version' and 'tou_accepted_at' for existing users
* fix linting errors
* Change UserTermsOfUseAcceptances' relation with 'user_id'
* Minor fix
* fix UserTouAcceptanceJobTest
* fix linting error
* Refactor: Change TermsOfUseVersion from enum to Model
* Fix: Remove hard dependency on pre-seeded ToU version
* fix linting errors
* fix linting error
* minor changes
* Fix: removed unnecessary fields
* fix testing wrong version
* fix linting error
* rename the method that retrieve latest active version of ToU
Sign up for freeto 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.

5 participants

@dati18@deer-wmde@tarrow@outdooracorn