DMC/scipro
12
0
Fork 0
Code Issues 3 Pull Requests 3 Actions Wiki Activity

task/3382: Harmonisera tabellnamn #6

Merged
ansv7779 merged 104 commits from task/3382 into develop 2024-11-12 13:33:44 +01:00
Conversation 4 Commits 104 Files Changed 129 +8653 -3217
tozh4728 commented 2024-06-10 15:44:23 +02:00
Owner
Copy Link
No description provided.
tozh4728 added 2 commits 2024-06-10 15:44:23 +02:00
ansv7779 changed title from task/3382: Harmonisera tabellnamn (Work in progress) to WIP: task/3382: Harmonisera tabellnamn 2024-06-10 16:28:39 +02:00
tozh4728 commented 2024-06-10 16:50:28 +02:00
Author
Owner
Copy Link

Code Standard:
Annotation value assignment, shall equals sign have space around or not? Code style is not consistent within SciPro.

// Prefer this? (IntelliJ code completion generates this style of code.)

@Column(name = "last_run")

// or shall we prefer this:

@Column(name="last_run")
Code Standard: Annotation value assignment, shall equals sign have space around or not? Code style is not consistent within SciPro. ```java // Prefer this? (IntelliJ code completion generates this style of code.) @Column(name = "last_run") // or shall we prefer this: @Column(name="last_run") ```
ansv7779 commented 2024-06-11 11:59:21 +02:00
Owner
Copy Link

I would prefer with spaces (first option).

I would prefer with spaces (first option).
tozh4728 added 2 commits 2024-06-17 10:48:47 +02:00
tozh4728 added 1 commit 2024-06-17 10:53:16 +02:00
tozh4728 added 1 commit 2024-06-17 11:14:28 +02:00
tozh4728 added 1 commit 2024-06-17 11:58:26 +02:00
tozh4728 added 3 commits 2024-06-17 16:33:16 +02:00
tozh4728 added 1 commit 2024-06-19 11:10:59 +02:00
tozh4728 added 4 commits 2024-06-19 15:24:35 +02:00
tozh4728 added 2 commits 2024-06-19 15:54:53 +02:00
tozh4728 added 3 commits 2024-06-20 16:13:17 +02:00
tozh4728 added 21 commits 2024-07-05 16:05:54 +02:00 
Since JPA MappedSuperclass is used, additional tables like GradingCriterionPoint, criterion,
GradingCriterion are partially fixed by renaming some of their columns.
Remove foreign key from table idea to table ProjectType, rename the foreign key column at idea.
Change JPA-metadata in Idea entity class as well.
Remove foreign key from table project to table ProjectType, rename the foreign key column at table
project. Change JPA-metadata in Project entity class as well.
fixed table target, without foreign key references to coming table application_period and project_type,
fixed entity class as well.
Fix table project_partner, except foreign key references to comming table application_period and
project_type. Fixed its entity class as well.
Fixed table ApplicationPeriodProjectType except any foreign key references,
fixed its entity class as well.
Fixed table ActivityTemplate, except foreign key to coming table activity_plan_template.
Fixed its entity class as well.
Fix table ActivityPlanTemplate and its entity class. Put back foreign key references from
activity_template and application_period_project_type as well.
Table applicationperiodexemption is fixed except foreign key to coming table application_period.
Its related entity class is fixed as well. Table idea removed foreign key reference to table
ApplicationPeriod.
tozh4728 added 2 commits 2024-07-10 13:53:12 +02:00 
Fix a few errors in SQL-script and JPA-mappings.
tozh4728 added 4 commits 2024-07-11 10:59:43 +02:00 
Rename foreign keys of table project_user_note and fixed a typo in JPA-mapping of entity class GradingReportTemplate.
tozh4728 added 8 commits 2024-07-11 16:55:18 +02:00 
Use single import statement for DeliveryConfiguration and Program. Missed it earlier.
Fixed a few tables related to checklist, such as:

* checklist
* checklist_category
* checklist_checklist_category
* checklist_user_last_open_date
* checklist_checklist_question

JPA-mapping is located at entity class Checklist and ChecklistCategory.
One value of ChecklistAnswerEnum shall be NOT_APPLICABLE, but it's wrongly spelled as NOT_APLICABLE.
Fixed following tables:

* checklist_question
* checklist_answer
* checklist_question_checklist_answer

Their related JPA-mappings are fixed in entity class ChecklistQuestion and ChecklistAnswer as well.
Fixed following tables:

* checklist_template
* checklist_template_question
* checklist_template_checklist_category

Their related JPA-mappings are fixed in entity class ChecklistTemplate.
tozh4728 added 2 commits 2024-08-09 14:38:09 +02:00
tozh4728 added 7 commits 2024-09-26 13:30:07 +02:00
tozh4728 added 1 commit 2024-09-26 15:09:35 +02:00
tozh4728 added 6 commits 2024-10-28 16:04:14 +01:00
tozh4728 changed title from WIP: task/3382: Harmonisera tabellnamn to task/3382: Harmonisera tabellnamn 2024-10-28 16:04:54 +01:00
tozh4728 added 1 commit 2024-10-30 08:35:33 +01:00
tozh4728 added 1 commit 2024-10-30 09:04:27 +01:00
tozh4728 added 1 commit 2024-10-30 10:21:08 +01:00
tozh4728 added 1 commit 2024-10-30 11:32:47 +01:00
tozh4728 added 1 commit 2024-10-31 13:23:23 +01:00
ansv7779 commented 2024-11-04 10:25:16 +01:00
Owner
Copy Link

Branch needs to be updated against latest develop

Branch needs to be updated against latest develop
tozh4728 added 8 commits 2024-11-07 09:48:51 +01:00
task/3382: Fix a few SQL errors, and ajusted JPA-mappings
Some checks failed
Build and test / build-and-test (push) Failing after 6m26s
Details
cfd42bcaf5
tozh4728 added 1 commit 2024-11-07 10:02:16 +01:00
Merge branch 'develop' into task/3382
Some checks failed
Build and test / build-and-test (push) Failing after 6m26s
Details
3649c14f70
tozh4728 added 1 commit 2024-11-07 12:42:17 +01:00
task/3382: Use editor-fold to surround different sections of entity classes
Some checks failed
Build and test / build-and-test (push) Failing after 6m26s
Details
f507fcf0ec
tozh4728 added 3 commits 2024-11-07 15:54:32 +01:00
task/3382: Add editor-folding support to ActivityPlanTemplate and ActivityTemplate
All checks were successful
Build and test / build-and-test (push) Successful in 6m43s
Details
8b705706b5
ansv7779 commented 2024-11-08 16:34:02 +01:00
Owner
Copy Link

All the re-ordering of things is making it very difficult to review the actual changes made. Please avoid doing this in the future or, at the very least, do the re-ordering in a separate commit that can be ignored when viewing the changes.

All the re-ordering of things is making it very difficult to review the actual changes made. Please avoid doing this in the future or, at the very least, do the re-ordering in a separate commit that can be ignored when viewing the changes.
ansv7779 requested review from ansv7779 2024-11-08 16:34:27 +01:00
tozh4728 added 1 commit 2024-11-11 10:08:58 +01:00
task/3382: Add missing column name to dateCreated for IdeaParticipation
All checks were successful
Build and test / build-and-test (push) Successful in 6m48s
Details
75cc952d48
tozh4728 added 1 commit 2024-11-11 10:20:23 +01:00
task/3382: Update table answer with enum value NOT_APPLICABLE as well
All checks were successful
Build and test / build-and-test (push) Successful in 6m56s
Details
004b68b98d
tozh4728 added 1 commit 2024-11-11 11:03:19 +01:00
task/3382: Add JPA-annotations to all children classes to NotificationEvent (missed these earlier)
All checks were successful
Build and test / build-and-test (push) Successful in 6m48s
Details
a5b2bb614b
tozh4728 added 1 commit 2024-11-11 11:12:10 +01:00
task/3382: Move back public nested types Event to top of the class definition to improved readability.
All checks were successful
Build and test / build-and-test (push) Successful in 10m57s
Details
353ead7966
ansv7779 added 1 commit 2024-11-11 15:44:56 +01:00
Fix foreign key names
All checks were successful
Build and test / build-and-test (push) Successful in 6m47s
Details
f11fdcfb98 
The names in production are somehow different from what the migrations say they should be. The production database has been manually corrected to match the names in the migration scripts.
ansv7779 requested changes 2024-11-11 15:46:24 +01:00
Dismissed
ansv7779 left a comment
Owner
Copy Link

While I appreciate the effort of re-organizing and cleaning up the code it should have been done in, at the very least, separate commits. Ideally with automated tooling to adhere to the formatting/organization established so that it is easy to maintain in the future.

But when it was done interspersed with the actual changes it made the pull request way bigger and harder to review than it had to be.

With that said it is still admirable work and something that was long overdue. I noticed you also fixed some plural to singular form to make it even more uniform.

There are a couple of user related foreign keys where I think the new column name lost some useful contextual information that I think should be fixed. There are many other columns that are named user_id but they were named that way beforehand as well so I'm ignoring those. Most columns were changed to <context>_user_id which is a good name but a few were changed from <context>_id to just user_id which I'm not a fan of.

If you could fix those couple of columns it looks good to me.

I ran the migrations against an empty schema and noticed foreign key names were mismatched with earlier migrations but they instead matched against what is in production. I've manually corrected the names in production and pushed a fix for the migration script to use the expected names based on earlier migrations.

I've clicked on basically every page in SciPro and they all work.

While I appreciate the effort of re-organizing and cleaning up the code it should have been done in, at the very least, separate commits. Ideally with automated tooling to adhere to the formatting/organization established so that it is easy to maintain in the future. But when it was done interspersed with the actual changes it made the pull request way bigger and harder to review than it had to be. With that said it is still admirable work and something that was long overdue. I noticed you also fixed some plural to singular form to make it even more uniform. There are a couple of user related foreign keys where I think the new column name lost some useful contextual information that I think should be fixed. There are many other columns that are named `user_id` but they were named that way beforehand as well so I'm ignoring those. Most columns were changed to `<context>_user_id` which is a good name but a few were changed from `<context>_id` to just `user_id` which I'm not a fan of. If you could fix those couple of columns it looks good to me. I ran the migrations against an empty schema and noticed foreign key names were mismatched with earlier migrations but they instead matched against what is in production. I've manually corrected the names in production and pushed a fix for the migration script to use the expected names based on earlier migrations. I've clicked on basically every page in SciPro and they all work.
core/src/main/java/se/su/dsv/scipro/activityplan/Activity.java Outdated
@ -55,0 +72,4 @@
private Checklist checklist;
@OneToOne(optional = true, cascade = CascadeType.ALL)
@JoinColumn(name = "file_reference_id", referencedColumnName = "id")
ansv7779 commented 2024-11-11 14:08:59 +01:00
Owner
Copy Link

This feels like a downgrade in terms of documentation from the column name. file_upload_reference_id tells you that it is an uploaded file to the activity while file_reference_id conveys no such information.

This feels like a downgrade in terms of documentation from the column name. `file_upload_reference_id` tells you that it is an uploaded file to the activity while `file_reference_id` conveys no such information.
tozh4728 commented 2024-11-12 11:17:24 +01:00
Author
Owner
Copy Link

It's renamed to 'upload_file_reference_id', following the same scheme _user_id. "file_reference" is the table name.

It's renamed to 'upload_file_reference_id', following the same scheme <context>_user_id. "file_reference" is the table name.
tozh4728 marked this conversation as resolved
core/src/main/java/se/su/dsv/scipro/activityplan/ActivityPlanTemplate.java Outdated
@ -53,1 +51,3 @@
}
//<editor-fold desc="JPA-mappings of foreign keys in this table (activity_plan_template) referencing other tables.">
@ManyToOne(optional=false)
@JoinColumn(name = "user_id")
ansv7779 commented 2024-11-11 14:12:21 +01:00
Owner
Copy Link

Again less self describing column name. creator_id tells you it is the user who created the template rather than just a generic user_id. The referenced table/type is visible from the foreign key/class.

Again less self describing column name. `creator_id` tells you it is the user who created the template rather than just a generic `user_id`. The referenced table/type is visible from the foreign key/class.
tozh4728 marked this conversation as resolved
core/src/main/java/se/su/dsv/scipro/checklist/ChecklistTemplate.java Outdated
@ -31,3 +51,4 @@
private List<String> questions = new ArrayList<>(1);
@ManyToOne(optional = false)
@JoinColumn(name = "user_id")
ansv7779 commented 2024-11-11 14:17:07 +01:00
Owner
Copy Link

Less self describing column name. creator_id tells you it is the user who created the template rather than just a generic user_id. The referenced table/type is visible from the foreign key/class.

Less self describing column name. `creator_id` tells you it is the user who created the template rather than just a generic `user_id`. The referenced table/type is visible from the foreign key/class.
tozh4728 marked this conversation as resolved
core/src/main/java/se/su/dsv/scipro/finalseminar/FinalSeminar.java Outdated
@ -59,0 +95,4 @@
* to delete the document
*/
@OneToOne(cascade = CascadeType.ALL)
@JoinColumn(name = "file_reference_id", referencedColumnName = "id")
ansv7779 commented 2024-11-11 14:20:49 +01:00
Owner
Copy Link

Less self describing column name. document_reference_id tells you something extra rather than just a generic file_reference_id. The referenced table/type is visible from the foreign key/class. Each seminar also has multiple documents related to it so a better name is useful. Especially since the column document_upload_date references some "document" but there is no document file column.

Less self describing column name. `document_reference_id` tells you something extra rather than just a generic `file_reference_id`. The referenced table/type is visible from the foreign key/class. Each seminar also has multiple documents related to it so a better name is useful. Especially since the column `document_upload_date` references some "document" but there is no document file column.
tozh4728 marked this conversation as resolved
core/src/main/java/se/su/dsv/scipro/peer/Comment.java Outdated
@ -16,12 +16,14 @@ public class Comment extends DomainObject {
private Long id;
@ManyToOne(optional = false)
@JoinColumn(name = "user_id")
ansv7779 commented 2024-11-11 14:39:51 +01:00
Owner
Copy Link

The renaming of some user foreign key columns seem a bit weird. In some places it was a loss of information by going to just user_id while others they got _user_id appended to them and some were left as-is. Out of the three options just user_id is the worst one when there is additional context to be had. In this case "creator", so I think the column should be creator_id or creator_user_id to convey as much information as possible. It is also consistent with other foreign keys that use the <context>_user_id naming scheme. See for example core/src/main/java/se/su/dsv/scipro/peer/PeerRequest.java and its requester_user_id.

The renaming of some user foreign key columns seem a bit weird. In some places it was a loss of information by going to just `user_id` while others they got `_user_id` appended to them and some were left as-is. Out of the three options just `user_id` is the worst one when there is additional context to be had. In this case "creator", so I think the column should be `creator_id` or `creator_user_id` to convey as much information as possible. It is also consistent with other foreign keys that use the `<context>_user_id` naming scheme. See for example `core/src/main/java/se/su/dsv/scipro/peer/PeerRequest.java` and its `requester_user_id`.
tozh4728 marked this conversation as resolved
core/src/main/java/se/su/dsv/scipro/plagiarism/urkund/UrkundSettings.java Outdated
@ -18,3 +20,3 @@
private boolean enabled = false;
@Basic
@Column(name = "user_name")
ansv7779 commented 2024-11-11 14:44:20 +01:00
Owner
Copy Link

Username is one word. "User name" is the user's name (as in John Doe) while username is the unique john@doe.example.

Username is one word. "User name" is the user's name (as in John Doe) while username is the unique `john@doe.example`.
tozh4728 marked this conversation as resolved
core/src/main/java/se/su/dsv/scipro/reviewing/ReviewerTarget.java Outdated
@ -22,3 +21,3 @@
@ManyToOne(optional = false)
@JoinColumn(name = "reviewer_id", nullable = false)
@JoinColumn(name = "user_id", nullable = false)
ansv7779 commented 2024-11-11 14:50:20 +01:00
Owner
Copy Link

Loss of information in the column name, should be reviewer_user_id.

Loss of information in the column name, should be `reviewer_user_id`.
tozh4728 marked this conversation as resolved
core/src/main/java/se/su/dsv/scipro/system/Username.java Outdated
@ -6,3 +6,3 @@
@Entity
@Cacheable(true)
@Table(name="username", uniqueConstraints={@UniqueConstraint(columnNames={"username"})})
@Table(name="user_name", uniqueConstraints={@UniqueConstraint(name = "uk_user_name", columnNames={"user_name"})})
ansv7779 commented 2024-11-11 14:56:10 +01:00
Owner
Copy Link

Username is one word, not two. User name is a user's name (John Doe) while username is their unique identifier john@doe.example.

Username is one word, not two. User name is a user's name (John Doe) while username is their unique identifier `john@doe.example`.
tozh4728 marked this conversation as resolved
core/src/main/java/se/su/dsv/scipro/system/Username.java Outdated
@ -12,3 +12,3 @@
private Long id;
@Basic(optional=false)
@Column(name = "user_name", nullable = false)
ansv7779 commented 2024-11-11 14:56:15 +01:00
Owner
Copy Link

Username is one word, not two. User name is a user's name (John Doe) while username is their unique identifier john@doe.example.

Username is one word, not two. User name is a user's name (John Doe) while username is their unique identifier `john@doe.example`.
tozh4728 marked this conversation as resolved
tozh4728 added 1 commit 2024-11-12 09:47:08 +01:00
task/3382: Rename 'user_name' to 'username'
All checks were successful
Build and test / build-and-test (push) Successful in 6m51s
Details
df29afdacc
tozh4728 added 1 commit 2024-11-12 09:58:47 +01:00
task/3382: Rename 'user_name' to 'username' for urkund_settings
All checks were successful
Build and test / build-and-test (push) Successful in 6m51s
Details
05e5725581
tozh4728 added 1 commit 2024-11-12 10:14:45 +01:00
task/3382: Rename 'user_id' to 'reviewer_user_id' for reviewer_target
All checks were successful
Build and test / build-and-test (push) Successful in 6m46s
Details
ccdfe6d979
tozh4728 added 1 commit 2024-11-12 10:48:18 +01:00
task/3382: Rename 'user_id' to 'creator_user_id' for comment
All checks were successful
Build and test / build-and-test (push) Successful in 6m46s
Details
c833426a69
tozh4728 added 1 commit 2024-11-12 11:00:58 +01:00
task/3382: Rename 'user_id' to 'creator_user_id' for activity_plan_template
All checks were successful
Build and test / build-and-test (push) Successful in 6m46s
Details
9a8604d76a
tozh4728 added 1 commit 2024-11-12 11:14:47 +01:00
task/3382: Rename 'user_id' to 'creator_user_id' for checklist_template
All checks were successful
Build and test / build-and-test (push) Successful in 6m47s
Details
0fb9e6dbc3
tozh4728 added 1 commit 2024-11-12 11:27:39 +01:00
task/3382: Rename 'file_reference_id' to 'upload_file_reference_id' for activity
All checks were successful
Build and test / build-and-test (push) Successful in 6m49s
Details
28e617ef9b
tozh4728 added 1 commit 2024-11-12 11:47:54 +01:00
task/3382: Rename 'file_reference_id' to 'document_file_reference_id' for final_seminar
Some checks failed
Build and test / build-and-test (push) Has been cancelled
Details
e36018f927
tozh4728 added 2 commits 2024-11-12 11:53:32 +01:00
task/3382: rename migration script version to V391 from V390
All checks were successful
Build and test / build-and-test (push) Successful in 6m53s
Details
2f1e927ab7
ansv7779 approved these changes 2024-11-12 13:32:37 +01:00
ansv7779 left a comment
Owner
Copy Link

Looks good.

Looks good.
ansv7779 merged commit cfe61a9ed8 into develop 2024-11-12 13:33:44 +01:00
ansv7779 deleted branch task/3382 2024-11-12 13:33:44 +01:00
Sign in to join this conversation.
Reviewers
No reviewers
ansv7779
Labels
Clear labels
No items
No Label
Milestone
Clear milestone
No items
No Milestone
Assignees
Clear assignees
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: DMC/scipro#6
No description provided.
Delete Branch "task/3382"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?