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

Add begin date and end date attribute to relationship #1073

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

aabbi15
Copy link

@aabbi15 aabbi15 commented Feb 29, 2024

Problem

Added Begin date and End date attributes to required relationship. Related to BB-490 and BB-283

Solution

I have added a new created attribute named 'Begin date' and 'End date'. In the relationship_type__attribute_type table I have appended these attributes there for certain relationship. I have listed them in the comments as well.

Areas of Impact

The bookbrainz database in SQL was updated with the said attributes added. There is not much other effect on the codebase.

@Tarunmeena0901
Copy link
Contributor

hey , may be you forget but i think we also have to make changes in bookbrainz.sql file accordingly after making any sql changes. our test uses the same file to test the updated database

@aabbi15
Copy link
Author

aabbi15 commented Mar 8, 2024

@Tarunmeena0901 I have added it now. Kindly check.

Copy link
Member

@MonkeyDo MonkeyDo left a comment

Choose a reason for hiding this comment

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

Sorry for only getting to reviewing this @aabbi15, and thanks for working on it !
Looks quite good, I have a suggestion or two below:


INSERT INTO bookbrainz.relationship_attribute_type (id, parent, root, child_order, name, description)
VALUES
(3, NULL, 1, 0, 'Begin date', 'This attribute indicates when the relationship begin.'),
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
(3, NULL, 1, 0, 'Begin date', 'This attribute indicates when the relationship begin.'),
(3, NULL, 1, 0, 'Begin date', 'This attribute indicates when the relationship began.'),

Comment on lines +3 to +5
INSERT INTO bookbrainz.relationship_attribute_type (id, parent, root, child_order, name, description)
VALUES
(3, NULL, 1, 0, 'Begin date', 'This attribute indicates when the relationship begin.'),
Copy link
Member

Choose a reason for hiding this comment

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

Here you can omit the id, as it is defined to be of type SERIAL in the SQL schema, it will be automatically incremented (and will avoid potential conflicts if the script was run out of order):

Suggested change
INSERT INTO bookbrainz.relationship_attribute_type (id, parent, root, child_order, name, description)
VALUES
(3, NULL, 1, 0, 'Begin date', 'This attribute indicates when the relationship begin.'),
INSERT INTO bookbrainz.relationship_attribute_type (parent, root, child_order, name, description)
VALUES
(NULL, 1, 0, 'Begin date', 'This attribute indicates when the relationship begin.'),

INSERT INTO bookbrainz.relationship_attribute_type (id, parent, root, child_order, name, description)
VALUES
(3, NULL, 1, 0, 'Begin date', 'This attribute indicates when the relationship begin.'),
(4, NULL, 1, 0, 'End date', 'This attribute indicates when the relationship ended.');
Copy link
Member

Choose a reason for hiding this comment

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

Same as above:
Plus we can defined a child_order here (I don't think it's currently in use on the website, but for the future it allows sorting siblings):

Suggested change
(4, NULL, 1, 0, 'End date', 'This attribute indicates when the relationship ended.');
(NULL, 1, 1, 'End date', 'This attribute indicates when the relationship ended.');

(21,3),
(21,4);


Copy link
Member

Choose a reason for hiding this comment

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

A few suggested additions, after looking more closely at the relationship_type table:

  • 18, 90 -> Pen name, Persona
  • 35, 69 -> Reconstruction
  • 36 -> Previous attribution
  • 40 -> Owner (publishers)
  • 41 -> Renamed (publishers)
  • 44, 63 -> Copyright
  • 45, 46 & 51 -> Licensor, Licensee
  • 57 -> Reprint

@@ -1055,4 +1055,121 @@ CREATE VIEW bookbrainz.work_import AS
LEFT JOIN bookbrainz.alias_set alias_set ON work_data.alias_set_id = alias_set.id
WHERE import.type = 'Work';


INSERT INTO bookbrainz.relationship_attribute_type (id, parent, root, child_order, name, description)
Copy link
Member

Choose a reason for hiding this comment

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

This file should not be changed in this case.
We modify it when we add or modify a table or something like that, but not when we add rows of data to the database, that is why we have separate migration scripts.
So you can remove all these additions in bookbrainz.sql

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