Fix BB-768 : Unset Author Credit leads to bugs #1051

Tarunmeena0901 · 2024-01-10T13:12:15Z

Problem

BB-768

refer to this discussion for more details

Solution

In edition.js and edition-group.js I added a check before displaying a warning of unset credit . this will not show any warning on created edition or edition-group page, if we click the checkbox while creating the edition or edition group
The checkbox was not getting checked and was needed to be clicked two times before disabling the edition credit , the reason was the redux initial state of authorCreditEnable was undefined rather than being true .

Areas of Impact

./entities/edition.js
./entities/edition-group.js
redux reducers.ts for edition and edition-groups
actions.ts in author-credit-editor

src/client/entity-editor/author-credit-editor/author-credit-section.tsx

Tarunmeena0901 · 2024-01-31T22:38:59Z

well..... I was just reviewing my own PR today and find out that my solution for this problem was so naive 😆 , so I had another look at this issue and fixed it in a better way in my upcoming commit

Tarunmeena0901 · 2024-02-01T10:32:43Z

so there was two cases when authorCreditEnable was being undefined whenever the page loads

when we try to create an edition
when we try to edit an edition group

the reason behind this in first case was that the rootstate had an empty edition section with no authorCreditEnable field to set the default state with

and the reason behind the second case was we were not sending any default authorCreditEnable field in enditionGroupSection rootstate to set the default state from

after fix for case1 :

same the bug is fixed for edition-group edit routes

kellnerd · 2024-02-01T11:45:58Z

src/client/components/pages/entities/edition-group.js

@@ -79,7 +79,7 @@ EditionGroupAttributes.propTypes = {
 };


-function EditionGroupDisplayPage({entity, identifierTypes, user, wikipediaExtract}) {
+function EditionGroupDisplayPage({entity, identifierTypes, user, wikipediaExtract}, authorCreditEnable) {


This parameter and its usage below are probably a leftover from your previous attempt to solve the issue. Modern React components have no second function parameter as far as I know.
Same goes for the EditionDisplayPage component.

YES ! you are correct, Kell. React components do not have a second function parameter. Thank you for pointing this out. This is indeed something I overlooked in my previous solution, where I attempted to prevent the warning about unset author credits from displaying on the edition and edition group pages. However, it's clear now that this approach won't work , i will fix this

but now as I think that when we are creating a edition or an edition group we don't allow to submit the form until the author credit is filled or "No author credit" checkbox is checked

so we have two cases here while creating

when someone as set that there is no author credit

author credits are provided while creating

in both the cases after creating the edition or edition group , the main page should not show any warning then why do we even have the warning ???
maybe we should remove it completely

we have two cases here while creating

You are right about this, but the warning is not there only for new entities.
We do want to show the warning for existing entites that are missing that information.

Oh i see now

MonkeyDo · 2024-02-01T17:38:23Z

Thanks for looking into this issue again, I appreciate your efforts!
I will review this very soon, thanks for your continued patience.

MonkeyDo · 2024-02-02T17:20:43Z

Thanks again for working on this.
I realized there is a crucial piece missing in this puzzle, and the feature won't work properly until it is resolved.

In short, I think we do not currently have a a way to store in the database that an edition has been set as "no author credit". The web page works fine, but we don't have a has_author_credit boolean column in the edition_data table, which is required if we want the checkbox to actually do something and being able to hide the warning.

Some modifications need to be done on the bookbrainz-site repo, and others on the bookbrainz-data-js repo (our ORM).
I'll get back to you when I'm able to work on this, hopefully next week.

Tarunmeena0901 · 2024-02-03T17:51:20Z

Thanks again for working on this. I realized there is a crucial piece missing in this puzzle, and the feature won't work properly until it is resolved.

In short, I think we do not currently have a a way to store in the database that an edition has been set as "no author credit". The web page works fine, but we don't have a has_author_credit boolean column in the edition_data table, which is required if we want the checkbox to actually do something and being able to hide the warning.

Some modifications need to be done on the bookbrainz-site repo, and others on the bookbrainz-data-js repo (our ORM). I'll get back to you when I'm able to work on this, hopefully next week.

Yesss, exactly! Currently, I was trying to do just that—saving a boolean variable in the database to indicate whether author credits are set or unset for an edition or edition group. I made some schema changes to the edition_data table and edition_group_data table yesterday. I would have completed it today, but I got a little busy. I'll surely figure out the next part; give me some time, and I'll be back with a fix. ✅✅

Tarunmeena0901 · 2024-02-07T21:43:27Z

In my last commit, I added a credit_section boolean column to our edition_data and edition_group_data tables. I also added the same column to our views.

The purpose is to set credit_section to false if author credits are disabled. This signifies that the entity does not require author credits, thus no warning will be shown. If credit_section for an entity is true, along with other pre-existing checks, it means that the author_credits section exists but with empty author credits, and a warning will be displayed.

The next step is to use the credit_section state in the frontend. Until then, any suggestions for improvement or a better approach to solving this aspect of the issue ?. 😊

…ontend to set or unset warning on entity page and enable or disable credit state whenever edit page loads

Tarunmeena0901 · 2024-02-13T11:31:43Z

My apologies for finalizing this PR later than expected. I was occupied with my college responsibilities. With this commit, consider this issue resolved.

In my latest commit, I utilized the saved state of author credits in both the edition and edition_group in the frontend to determine whether to display the "Unset author credit warning." Additionally, the "Author credit enable" checkbox in the edit page is no longer set to true by default; instead, it utilizes the previously saved state of the credit section to determine whether it should be checked or unchecked.

this commit fix BB-768 as well as fix BB-767 . The only remaining task is to address the failing test and make changes in bookbrainz-data-js .
Thank you for your patience.

…unified form and added the edition_data.credit_section state in bb.edition_import view

Tarunmeena0901 · 2024-02-25T14:48:34Z

i was having a look at it again and after all this time i noticed that unified-form is crying in the corner and i haven't made any changes there , so i did it this time and i noticed two view in bookbrainz.sql file which were not present in my local postgres setup
whenever i tried to do \dv , so i am not sure if those needs the the new credit_section field.

in edition and edition-group pages rather than displaying nothing if the author credits are not provide while creating the entity i thought it would be better to display something like Author Credits: N/A so if someone see it and remember the credits they may edit the entity accordingly

…t think were correct cause mocha test fail so maybe we should leave the test part for another pr ( truth is i am not good at writing test )

MonkeyDo · 2024-03-18T15:55:05Z

it means that the author_credits section exists but with empty author credits

While re-reading this comment, a lightbulb turned on above my head:
I suppose all we really need is to modify the entity creation code to allow creating an empty author_credit (i.e. an AC without any author_credit_name attached to it) when the "empty author credit" checkbox is checked.

Similarly, when loading an existing entity from the database, if it has an author_credit but the author_credit has no author_credit_name attached to it we can consider it's on purpose, and we don't show the "author credit unset" warning. And if we try editing that entity, the "no author credit" checkbox will be pre-checked.

There is also an author_count (INT) column in the author_credit table, which we don't currently use anywhere as far as I can tell. Perhaps manually setting it to 0 could serve the same purpose? Or maybe as a secondary check that the entity has "no AC" set on purpose? Not sure it's necessary.

Tarunmeena0901 · 2024-04-07T20:03:36Z

While re-reading this comment, a lightbulb turned on above my head: I suppose all we really need is to modify the entity creation code to allow creating an empty author_credit (i.e. an AC without any author_credit_name attached to it) when the "empty author credit" checkbox is checked.

I should have asked this question earlier, but since I've started working on this now, I'm not able to comprehend that by using this method, how do we handle entities that are already present . Since they didn't have empty author credits, are we gonna edit each entity and check the 'don't have author credit' checkbox?
This doesn't seem feasible as we have over 2000 editions and edition groups.

like you said here
#1051 (comment)

MonkeyDo

Sorry for the delay in reviewing this!
I had to get in deep and try some things myself to ensure everything works as expected.
You did a really nice job of it!
I do have comments and suggestions to improve the code, it might look like a lot but it's a lot of repeating the same things since we have code for editions and edition groups.

There is one piece of code to be added at the very top of the processAuthorCredit function in src/server/routes/entity/entity.tsx
It gets the value of creditSection that we return in transformNewForm, and uses that to set the author credit id to null on the database model if the user checked the box:

const authorCreditEnabled = _.get(currentEntity, ['creditSection']);
	if (!authorCreditEnabled) {
		return {
			authorCreditId: null
		};
	}

MonkeyDo · 2024-05-24T10:34:36Z

sql/schemas/bookbrainz.sql

 );
+UPDATE bookbrainz.edition_data SET credit_section = true;


This update statement is not required considering we have a default already, and probably not wanted

MonkeyDo · 2024-05-24T10:35:00Z

sql/schemas/bookbrainz.sql

 );
+
+UPDATE bookbrainz.edition_group_data SET credit_section = true;


Same as above

MonkeyDo · 2024-05-24T14:08:17Z

src/server/routes/entity/edition-group.ts

+		authorCreditEnable: editionGroup.creditSection,
+		creditSection: editionGroup.creditSection,


Same comment as above here, we don't need to send creditSection to the redux state as it duplicates authorCreditEnable.

MonkeyDo · 2024-05-24T18:32:04Z

src/client/components/pages/entities/edition-group.js

+	else if (!entity.deleted && hasAuthorCredits === false) {
+		authorCreditSection = (
+			<div className="alert alert-warning">
+				Author Credits : N/A
+			</div>);
+	}


I don't think the banner here improves anything, it makes it look like something is wrong when in fact a user ticked a box to clarify it doesn't have an author, so everything is good.
I don't think we need to display anything in this case.

MonkeyDo · 2024-05-24T18:32:18Z

src/client/components/pages/entities/edition.js

+	else if (!entity.deleted && hasAuthorCredits === false) {
+		authorCreditSection = (
+			<div className="alert alert-warning">
+				Author Credits : N/A
+			</div>);
+	}


Same as above

MonkeyDo · 2024-05-24T18:44:54Z

src/server/routes/entity/edition.ts

@@ -476,7 +479,8 @@ export function editionToFormState(edition) {
 	const editionGroup = utils.entityToOption(edition.editionGroup);

 	const editionSection = {
-		authorCreditEnable: true,
+		authorCreditEnable: edition.creditSection,
+		creditSection: edition.creditSection,


And I think that's our final creditSection to remove from the state!

MonkeyDo · 2024-05-24T18:46:10Z

sql/migrations/2024-08-23-credit-section/up.sql

+UPDATE bookbrainz.edition_data SET credit_section = true;
+UPDATE bookbrainz.edition_group_data SET credit_section = true;


These are not wanted, we do not want to set the value to true for ALL records!
This is what these lines do, without a WHERE clause. If this was the intent, updating all records in a DB is in The Danger Zone and should always be treated with care :)

MonkeyDo · 2024-05-24T18:46:51Z

sql/migrations/2024-08-23-credit-section/up.sql

+UPDATE bookbrainz.edition_data SET credit_section = true;
+UPDATE bookbrainz.edition_group_data SET credit_section = true;
+
+-- Recreate the view with the new definition


We want to drop the two views we're about to recreate:

-- Drop the existing view if it exists DROP VIEW IF EXISTS bookbrainz.edition, bookbrainz.edition_group;

MonkeyDo · 2024-05-24T18:52:17Z

sql/migrations/2024-08-23-credit-section/down.sql

+DROP VIEW IF EXISTS 
+    bookbrainz.edition,
+    bookbrainz.edition_group;
+


The down file is supposed to allow you to return to the previous version of the schema.
In this case, that means deleting the new credit_section column for the edition_data and edition_group_data tables, as well as dropping and recreating the views as they were before which you can copy directly from master: https://github.com/metabrainz/bookbrainz-site/blob/master/sql/schemas/bookbrainz.sql#L869C1-L883C27

MonkeyDo · 2024-05-24T18:52:59Z

sql/migrations/2024-08-23-credit-section/up.sql

@@ -0,0 +1,90 @@
+-----------------------------------------------------------------------
+-- Rename entities Creator -> Author and Publication -> Edition Group --


Suggested change

-- Rename entities Creator -> Author and Publication -> Edition Group --

-- Adds a credit_section boolean column to edition_data and edition_group_data to make author credits optional --

MonkeyDo · 2024-05-24T19:18:39Z

One thing I'll need to figure out on the heels of this fix is a good way to allow users to keep an Edition and its Edition Group in sync. In MusicBrainz it's a simple checkbox which only appears when the author credit has been modified:

Tarunmeena0901 added 2 commits January 10, 2024 18:14

bug(Edition & Edition-Group): fixes Unset Author Credit leads to bugs

b40057a

fixed linting errors

0066632

Tarunmeena0901 changed the title ~~Fix BB-768 : Unset Author Credit leads to bugs~~ Fix BB-768 & BB-767: Unset Author Credit leads to bugs Jan 10, 2024

Tarunmeena0901 commented Jan 18, 2024

View reviewed changes

src/client/entity-editor/author-credit-editor/author-credit-section.tsx Outdated Show resolved Hide resolved

Tarunmeena0901 changed the title ~~Fix BB-768 & BB-767: Unset Author Credit leads to bugs~~ Fix BB-768 : Unset Author Credit leads to bugs Jan 30, 2024

better solution

dabbd36

kellnerd reviewed Feb 1, 2024

View reviewed changes

removed some previous unecessary changes

c701344

Tarunmeena0901 added 2 commits February 8, 2024 02:48

saving the credit section state in database also made schema changes

fd60e88

linting error fix

d175cea

fix(Auhtor-Credit-section) : used saved state of credit section in fr…

f5c5320

…ontend to set or unset warning on entity page and enable or disable credit state whenever edit page loads

Tarunmeena0901 mentioned this pull request Feb 13, 2024

part of issue BB-768 : updating the edition model function metabrainz/bookbrainz-data-js#315

Open

Tarunmeena0901 added 2 commits February 14, 2024 02:00

Merge branch 'metabrainz:master' into BB-768

64ef0a0

made changes in bookbrainz.sql and fix linting error

3b73396

Tarunmeena0901 requested review from MonkeyDo and kellnerd February 14, 2024 19:00

Tarunmeena0901 added 3 commits February 25, 2024 20:04

better warning display checks , also used the creditsection state in …

2b853b3

…unified form and added the edition_data.credit_section state in bb.edition_import view

changes in bb.edition_group_import view

0d69622

fix lint error

ac12b87

Tarunmeena0901 added 3 commits March 6, 2024 02:19

minor fix

472dc68

fix failing build test

bba6428

fail build test fix metabrainz#2

e32636b

Tarunmeena0901 and others added 2 commits March 6, 2024 20:14

Merge branch 'metabrainz:master' into BB-768

d1bcb7f

merge latest changes and also i made changes in test file which i don…

d35e293

…t think were correct cause mocha test fail so maybe we should leave the test part for another pr ( truth is i am not good at writing test )

Tarunmeena0901 marked this pull request as draft May 21, 2024 18:15

MonkeyDo requested changes May 24, 2024

View reviewed changes

Merge branch 'master' into BB-768

ff859a6

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix BB-768 : Unset Author Credit leads to bugs #1051

Fix BB-768 : Unset Author Credit leads to bugs #1051

Tarunmeena0901 commented Jan 10, 2024 •

edited

Tarunmeena0901 commented Jan 31, 2024

Tarunmeena0901 commented Feb 1, 2024

kellnerd Feb 1, 2024

Tarunmeena0901 Feb 1, 2024 •

edited

Tarunmeena0901 Feb 1, 2024 •

edited

MonkeyDo Feb 2, 2024

Tarunmeena0901 Feb 3, 2024

MonkeyDo commented Feb 1, 2024

MonkeyDo commented Feb 2, 2024

Tarunmeena0901 commented Feb 3, 2024 •

edited

Tarunmeena0901 commented Feb 7, 2024 •

edited

Tarunmeena0901 commented Feb 13, 2024

Tarunmeena0901 commented Feb 25, 2024 •

edited

MonkeyDo commented Mar 18, 2024

Tarunmeena0901 commented Apr 7, 2024 •

edited

MonkeyDo left a comment

MonkeyDo May 24, 2024

MonkeyDo May 24, 2024

MonkeyDo May 24, 2024

MonkeyDo May 24, 2024

MonkeyDo May 24, 2024

MonkeyDo May 24, 2024

MonkeyDo May 24, 2024

MonkeyDo May 24, 2024

MonkeyDo May 24, 2024

MonkeyDo May 24, 2024

MonkeyDo commented May 24, 2024

		);

		UPDATE bookbrainz.edition_group_data SET credit_section = true;

		authorCreditEnable: editionGroup.creditSection,
		creditSection: editionGroup.creditSection,

		UPDATE bookbrainz.edition_data SET credit_section = true;
		UPDATE bookbrainz.edition_group_data SET credit_section = true;

		@@ -0,0 +1,90 @@
		-----------------------------------------------------------------------
		-- Rename entities Creator -> Author and Publication -> Edition Group --

	-- Rename entities Creator -> Author and Publication -> Edition Group --
	-- Adds a credit_section boolean column to edition_data and edition_group_data to make author credits optional --

Fix BB-768 : Unset Author Credit leads to bugs #1051

Are you sure you want to change the base?

Fix BB-768 : Unset Author Credit leads to bugs #1051

Conversation

Tarunmeena0901 commented Jan 10, 2024 • edited

Problem

Solution

Areas of Impact

Tarunmeena0901 commented Jan 31, 2024

Tarunmeena0901 commented Feb 1, 2024

Choose a reason for hiding this comment

Tarunmeena0901 Feb 1, 2024 • edited

Choose a reason for hiding this comment

Tarunmeena0901 Feb 1, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MonkeyDo commented Feb 1, 2024

MonkeyDo commented Feb 2, 2024

Tarunmeena0901 commented Feb 3, 2024 • edited

Tarunmeena0901 commented Feb 7, 2024 • edited

Tarunmeena0901 commented Feb 13, 2024

Tarunmeena0901 commented Feb 25, 2024 • edited

MonkeyDo commented Mar 18, 2024

Tarunmeena0901 commented Apr 7, 2024 • edited

MonkeyDo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MonkeyDo commented May 24, 2024

Tarunmeena0901 commented Jan 10, 2024 •

edited

Tarunmeena0901 Feb 1, 2024 •

edited

Tarunmeena0901 Feb 1, 2024 •

edited

Tarunmeena0901 commented Feb 3, 2024 •

edited

Tarunmeena0901 commented Feb 7, 2024 •

edited

Tarunmeena0901 commented Feb 25, 2024 •

edited

Tarunmeena0901 commented Apr 7, 2024 •

edited