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

MonkeyDo · 2024-06-03T16:31:57Z

I had some issues with the automated deployment, sorry about that !
Taking the opportunity to check the new package so I pushed the commit.

Tarunmeena0901 · 2024-06-03T16:35:08Z

ohh ok i just updated it locally but for some reason in package.json it still shows 5.0.0

MonkeyDo · 2024-06-03T16:36:40Z

ohh ok i just updated it locally but for some reason in package.json it still shows 5.0.0

It just wasn't published until a minute ago 😅 Now version 5.1.1 should be installed

Tarunmeena0901 · 2024-06-03T16:37:35Z

oh ok ok 👍 i will wait some more before updating

MonkeyDo · 2024-06-03T16:44:31Z

oh ok ok 👍 i will wait some more before updating

The poackage is already updated, I pushed a commit for it.

However tests are still failing, but looks unrelated to the changes in your PR. Let me look into it

Tarunmeena0901 · 2024-06-03T16:49:32Z

Oh, I didn't notice that you updated the package. Thanks!!!
I also don't think the failing test is related to this PR, but let me know if it is.

sorry i forgot to remove some comments 🫢 thanks for removing them too !

Remove console log, + formatting

Not required, and unwanted

…to pr/1051

MonkeyDo · 2024-06-04T12:14:17Z

Hello!

I updated the PR and got rid of all the unrelated issues, phew!
I deployed the PR for testing on test.bookbrainz.org, and found that there is a remaining issue that is most likely related to the failing test (https://github.com/metabrainz/bookbrainz-site/runs/25781165934#r0s38)

Creating an Edition without an AC works absolutely fine, and when on the edition display page the "no AC set" warning is not shown, so that's working 💯 ! See https://test.bookbrainz.org/revision/30808

However if I then edit the edition to add an AC, it looks like the AC is not saved to the database, and the "no AC set" warning is shown, which suggests the tickbox state ("this edition doesn't have an AC") is saved correctly, but not the AC itself.
See https://test.bookbrainz.org/revision/30809

Want to investigate what is happening in the noodly code of entity saving? 🤣
I'll go hunting too when I can but two heads are better than one !

Tarunmeena0901 · 2024-06-04T13:19:06Z

Actually, the problem is that from the very beginning, my Elasticsearch was not working, and I left it as it was. I was still able to work on most things, except for some tasks like setting new author credits or relationships. Therefore, I won't be able to test it locally before fixing my Elasticsearch. ☹️

However, I will still take a look at it, and maybe I can find the cause. I may trouble you for testing purposes later. 😊

Tarunmeena0901 · 2024-06-16T18:54:24Z

finally my elastic search problem is fixed and i can work on the new issue we have here let me know if you want me to start a fresh pr this one has become little MESSY.

MonkeyDo · 2024-06-21T14:57:10Z

finally my elastic search problem is fixed and i can work on the new issue we have here let me know if you want me to start a fresh pr this one has become little MESSY.

Glad you got your local setup working :)
No need to open another PR, we can work with this one. I might end up squashing the commits so that the history in the main branch is clean.

processAuthorCredit was using the old entity to determine if it should save an author credit, which meant that an edition without AC was never allowed to have an AC. Also refactored processEditionSets to await promises directly instead of using makePromiseFromObject

MonkeyDo · 2024-06-21T16:20:44Z

I found the culprit!
In processAuthorCredit we were checking if authorCreditEnabled but using the old state of the entity rather than the new one the client just sent, so if you created an edition with no AC and then tried adding an AC, authorCreditEnabled would be false and it would not create an AC.

The date was copied from another folder but not changed

Tarunmeena0901 · 2024-06-21T17:17:49Z

THANKS!!!! You just saved my weekend. I was going to look at it tomorrow, and this may have taken me hours to figure out the problem. Thanks again! 🙌 😇

I also noticed that you replaced some promises with async/await syntax. It looks great!✨ But here’s a twist, I had already made those changes in the old PR for BB-672 #1058. Déjà vu, developer style! 😂

Look, they are almost similar
https://github.com/metabrainz/bookbrainz-site/pull/1058/files#diff-69e04ad0b67c34fe4770d91cc33d661af834447e45415e53ba9cf48023eb6537L739-R819

MonkeyDo · 2024-06-21T17:22:40Z

Ahhaaa yes, I thought it seemed familiar!
Sorry I haven't got that one merged yet! Maybe I should do that other one first?

I took the liberty to quickly fix the revision diff which was showing nothing for deleted ACs.

EDIT: I see the issue! I had started a review god knows when, but never clicked the submit button and it is still pending...

MonkeyDo · 2024-06-21T17:27:35Z

P.S.: I'm rebuilding the image and deploying it to test.bookbrainz if you want to do some manual testing to check everything.

Tarunmeena0901 · 2024-06-21T17:32:13Z

P.S.: I'm rebuilding the image and deploying it to test.bookbrainz if you want to do some manual testing to check everything.

sure ! I will check

Ahhaaa yes, I thought it seemed familiar!
Sorry I haven't got that one merged yet! Maybe I should do that other one first?

no problem , I will just merge the conflicting changes there once this PR is done

Tarunmeena0901 · 2024-06-21T17:43:31Z

P.S.: I'm rebuilding the image and deploying it to test.bookbrainz if you want to do some manual testing to check everything.

Looks good to me 👍
✅ First, when creating the edition and even after setting the AC the first time during creation, the ACs were not saving. Now it does!
✅ After setting AC and then later editing the edition or edition group and setting new ACs was not working, but now it does!

MonkeyDo · 2024-06-25T10:50:00Z

OK, PR is ready to merge as far as I can tell !
I'm going to deploy it to beta for a week or so for further testing with a wider pool of users.

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

Merged

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

MonkeyDo added 2 commits June 3, 2024 18:59

Linting

c8d1097

Remove console log, + formatting

Merge branch 'master' into pr/1051

fdb4b0f

MonkeyDo marked this pull request as ready for review June 4, 2024 09:36

MonkeyDo and others added 4 commits June 4, 2024 11:36

Merge branch 'master' into BB-768

21dd8cf

Remove SQL column setting

0bfbe8f

Not required, and unwanted

Merge branch 'BB-768' of github.com:Tarunmeena0901/bookbrainz-site in…

ac5d9b7

…to pr/1051

Merge branch 'master' into BB-768

87d5560

Merge branch 'metabrainz:master' into BB-768

9d0de38

Rename migration folder

96a28a2

The date was copied from another folder but not changed

Fix revision diff display when removing author credits

ccc1c2b

MonkeyDo approved these changes Jun 25, 2024

View reviewed changes

MonkeyDo merged commit 059c0e6 into metabrainz:master Jun 25, 2024
4 checks passed

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

Loading

Tarunmeena0901 commented Jan 31, 2024

Tarunmeena0901 commented Feb 1, 2024

kellnerd Feb 1, 2024

Tarunmeena0901 Feb 1, 2024 •

edited

Loading

Tarunmeena0901 Feb 1, 2024 •

edited

Loading

MonkeyDo Feb 2, 2024

Tarunmeena0901 Feb 3, 2024

MonkeyDo commented Feb 1, 2024

MonkeyDo commented Feb 2, 2024

Tarunmeena0901 commented Feb 3, 2024 •

edited

Loading

Tarunmeena0901 commented Feb 7, 2024 •

edited

Loading

Tarunmeena0901 commented Feb 13, 2024

Tarunmeena0901 commented Feb 25, 2024 •

edited

Loading

MonkeyDo commented Jun 3, 2024

Tarunmeena0901 commented Jun 3, 2024

MonkeyDo commented Jun 3, 2024

Tarunmeena0901 commented Jun 3, 2024

MonkeyDo commented Jun 3, 2024

Tarunmeena0901 commented Jun 3, 2024 •

edited

Loading

MonkeyDo commented Jun 4, 2024

Tarunmeena0901 commented Jun 4, 2024 •

edited

Loading

Tarunmeena0901 commented Jun 16, 2024

MonkeyDo commented Jun 21, 2024

MonkeyDo commented Jun 21, 2024

Tarunmeena0901 commented Jun 21, 2024

MonkeyDo commented Jun 21, 2024 •

edited

Loading

MonkeyDo commented Jun 21, 2024

Tarunmeena0901 commented Jun 21, 2024

Tarunmeena0901 commented Jun 21, 2024

MonkeyDo commented Jun 25, 2024

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

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

Conversation

Tarunmeena0901 commented Jan 10, 2024 • edited Loading

Problem

Solution

Areas of Impact

Tarunmeena0901 commented Jan 31, 2024

Tarunmeena0901 commented Feb 1, 2024

kellnerd Feb 1, 2024

Choose a reason for hiding this comment

Tarunmeena0901 Feb 1, 2024 • edited Loading

Choose a reason for hiding this comment

Tarunmeena0901 Feb 1, 2024 • edited Loading

Choose a reason for hiding this comment

MonkeyDo Feb 2, 2024

Choose a reason for hiding this comment

Tarunmeena0901 Feb 3, 2024

Choose a reason for hiding this comment

MonkeyDo commented Feb 1, 2024

MonkeyDo commented Feb 2, 2024

Tarunmeena0901 commented Feb 3, 2024 • edited Loading

Tarunmeena0901 commented Feb 7, 2024 • edited Loading

Tarunmeena0901 commented Feb 13, 2024

Tarunmeena0901 commented Feb 25, 2024 • edited Loading

MonkeyDo commented Jun 3, 2024

Tarunmeena0901 commented Jun 3, 2024

MonkeyDo commented Jun 3, 2024

Tarunmeena0901 commented Jun 3, 2024

MonkeyDo commented Jun 3, 2024

Tarunmeena0901 commented Jun 3, 2024 • edited Loading

MonkeyDo commented Jun 4, 2024

Tarunmeena0901 commented Jun 4, 2024 • edited Loading

Tarunmeena0901 commented Jun 16, 2024

MonkeyDo commented Jun 21, 2024

MonkeyDo commented Jun 21, 2024

Tarunmeena0901 commented Jun 21, 2024

MonkeyDo commented Jun 21, 2024 • edited Loading

MonkeyDo commented Jun 21, 2024

Tarunmeena0901 commented Jun 21, 2024

Tarunmeena0901 commented Jun 21, 2024

MonkeyDo commented Jun 25, 2024

Tarunmeena0901 commented Jan 10, 2024 •

edited

Loading

Tarunmeena0901 Feb 1, 2024 •

edited

Loading

Tarunmeena0901 Feb 1, 2024 •

edited

Loading

Tarunmeena0901 commented Feb 3, 2024 •

edited

Loading

Tarunmeena0901 commented Feb 7, 2024 •

edited

Loading

Tarunmeena0901 commented Feb 25, 2024 •

edited

Loading

Tarunmeena0901 commented Jun 3, 2024 •

edited

Loading

Tarunmeena0901 commented Jun 4, 2024 •

edited

Loading

MonkeyDo commented Jun 21, 2024 •

edited

Loading