removal of awards

[lilfade]

Just a generic removal of awards, seems the community was okay with this for the time being as it's not used.

[nobodyatroot]

More a question for @melroy89 since I haven't messed with any translations lately, but is it necessary to remove all instances of "awards" from every translation file or will the Weblate two way sync happen automagically once it has been removed only from the English translation?

[e-five256]

I learned in #157 (comment) that the non-en translation files shouldn't be touched in this repo

[nobodyatroot]

I learned in #157 (comment) that the non-en translation files shouldn't be touched in this repo

That's what my understanding was as well, @lilfade you should undo those commits to the non-english translations, they'll all follow the English base translation once Weblate syncs after merging.

[lilfade]

Good to know, will get that done here after bit.

[lilfade]

Fixed, still unsure on the modifications to the tables that I commented out, not a Postgres wizard so anyone willing to check that out and push a fix if needed would be welcome. This should just remove the dead code no one wants currently so should be good to review and turn into a PR now if everything looks good.

[melroy89]

More a question for @melroy89 since I haven't messed with any translations lately, but is it necessary to remove all instances of "awards" from every translation file or will the Weblate two way sync happen automagically once it has been removed only from the English translation?

No, just English translation is fine. English is configured to be the "base" language in Weblate. And due to the two way sync we have setup correctly, this will automatically put in Weblate and remove it from other languages as well. I hope 😆 . See "Monolingual base language file" (within Weblate->Mbin->Mbin->Settings->Files):

[melroy89]

Let's indeed not remove/drop ap_activity. Is still used after all.

[lilfade]

Yea noticed that one. Unsure why it relies on the award for the foreign key though on this one. Thoughts on a fix for this one?

[melroy89]

There was a "many-on-one" DB relation as you can see in ApActivity.php between this ap_activity table and the users as well as the ap_activity and the magazines.

I think dropping ALTER TABLE ap_activity DROP user_id & ALTER TABLE ap_activity DROP magazine_id might be fine .. let me check the tables in DB later today..

[e-five256]

I would've expected the FK on ap_activity to user_id on user to help make sure all activitypub events correspond to a local user. I'm not really understanding what it has to do with awards 🤔

[melroy89]

@e-five256 yea I didn't check the DB to be honest yet (now I did). But if the user_id and magazine_id has a relation only towards the awards table it can be removed. If the same foreign keys are used for other reasons, then not.

Again looking at the change of @lilfade he only removed the Doctrine annotations, which are NOT just comments. These annotations are talking something about the DB design. So if you wish the remove the many-to-one relation related to the awards table (inversedBy), then also remove public User $user; and public ?Magazine $magazine; lines below the Doctrine annotations (after all the annotations are belong to the PHP lines below that).

Meaning all these lines then need to be removed (but keep subject_id):

    #[ManyToOne(targetEntity: User::class, inversedBy: 'awards')]
    #[JoinColumn(nullable: false, onDelete: 'CASCADE')]
    public User $user;
    #[ManyToOne(targetEntity: Magazine::class, inversedBy: 'awards')]
    #[JoinColumn(nullable: true, onDelete: 'CASCADE')]
    public ?Magazine $magazine;

If you want to keep the user_id and magazine_id then you need to understand how/why this data was related to the awards table. And if we want to keep it or not. We need to understand the underlying code and how it was used (or not used at all?).

For doctrine see: https://www.doctrine-project.org/projects/doctrine-orm/en/2.16/reference/association-mapping.html#one-to-many-bidirectional

[melroy89]

I also checked the data in my database for this ap_activity table. I don't have any rows.. This table used at all??

SELECT * FROM ap_activity;
 id | user_id | magazine_id | subject_id | type | body | created_at 
----+---------+-------------+------------+------+------+------------
(0 rows)

[nobodyatroot]

@melroy89 can double check me, but so far so good. I migrated a base docker dev environment with the test fixtures loaded over to this branch without any issues.

[melroy89]

@lilfade Either keep the User & Magazine relation of the ApActivity and think about another Doctrine annotation. Or if you want to remove this ManyToOne relation of the ApActivity, you need to remove the PHP lines belonging to the Doctrine annotation...

Read more about doctrine first, before continuing with this change or even before considering merging this PR: https://www.doctrine-project.org/projects/doctrine-orm/en/2.16/reference/association-mapping.html#one-to-many-bidirectional

@nobodyatroot yea, I doubled checked, but the Doctrine changes are incorrect.

Problem with doctrine migration

[melroy89]

No update?

I don't want to be the blocker

[nobodyatroot]

Please re-review migration, I think it looks OK now, also fixed merge conflicts since this is 3 weeks old.

[e-five256]

lgtm

[nobodyatroot]

I won't merge this until @melroy89 and/or others approve, probably a good habit for anyone merging doctrine migrations... having as many eyes on it as possible.

[nobodyatroot]

Branch is now running live on kbin.run