Fix public favorites #390

lamp · 2024-05-06T10:02:09Z

lamp commented

2024-05-06 10:02:09 +00:00

First-time contributor

closes #389

Oneric commented

2024-05-06 18:27:05 +00:00

Thanks for porting this! I haven’t looked closely at this yet, but i think the attribution needs to be clearer. Both because attibuting original authors is the nice thing to do and because not doing so can become legally problematic.

iiuc the first commit ports over the two Pleroma PRs linked in the issue and those original commit hashes are mentioned in the commit messages. However, (a) there’s no information about the original author, (b) those commits don’t exists in akkoma-fe and thus can’t be easily looked up and (c) afaiu you’re not the original author of those patches.

If the original commits applied cleanly or only with minor changes. ideally they’d ideally retain the original primary author, possibly with an added Cherry-picked-from: line. (e.g. by exporting from pleroma-fe with git format-patch and then applying to akkoma-fe with git am, or just using git commit --amend --author="...")
If significant changes were needed, it’s probably fine to keep yourself as the primary author, but you should at least add something like this to the commit message:

Based on the below but with many adjustments
necessary to adapt it to akkoma-fe:

Cherry-picked-from: https://git.pleroma.social/pleroma/pleroma-fe/-/commit/6f452d672fe740035cf1d29d03bcda0d39438753
Cherry-picked-from: https://git.pleroma.social/pleroma/pleroma-fe/-/commit/1ceffb4e713b4b20d70121fba92d2b50f2d3cadf

Co-authored-by: marcin mikołajczak <git@mkljczk.pl>

Thanks for porting this! I haven’t looked closely at this yet, but i think the attribution needs to be clearer. Both because attibuting original authors is the nice thing to do and because not doing so can become legally problematic. iiuc the first commit ports over the two Pleroma PRs linked in the issue and those original commit hashes are mentioned in the commit messages. However, (a) there’s no information about the original author, (b) those commits don’t exists in akkoma-fe and thus can’t be easily looked up and (c) afaiu you’re not the original author of those patches. If the original commits applied cleanly or only with minor changes. ideally they’d ideally retain the original primary author, possibly with an added `Cherry-picked-from:` line. *(e.g. by exporting from pleroma-fe with `git format-patch` and then applying to akkoma-fe with `git am`, or just using `git commit --amend --author="..."`)* If significant changes were needed, it’s probably fine to keep yourself as the primary author, but you should at least add something like this to the commit message: ``` Based on the below but with many adjustments necessary to adapt it to akkoma-fe: Cherry-picked-from: https://git.pleroma.social/pleroma/pleroma-fe/-/commit/6f452d672fe740035cf1d29d03bcda0d39438753 Cherry-picked-from: https://git.pleroma.social/pleroma/pleroma-fe/-/commit/1ceffb4e713b4b20d70121fba92d2b50f2d3cadf Co-authored-by: marcin mikołajczak <git@mkljczk.pl> ```

lamp force-pushed public-favorites from 893ce69a1b to 4d91a7b2c3

2024-05-07 00:55:46 +00:00

Compare

lamp commented

2024-05-07 00:56:49 +00:00

First-time contributor

ok i

Oneric reviewed 2024-05-12 23:42:47 +00:00

Oneric left a comment

The basics seem good to me except for one change to potentially clean things up a bit

src/components/user_profile/user_profile.js

					
				@ -117,2 +120,2 @@

				      // only we can see our own favourites

				      if (this.isUs) timelineTabMap['favorites'] = 'favorites'

				      if (this.favoritesTabVisible) timelineTabMap['favorites'] = 'favorites'

Oneric commented

2024-05-12 23:38:05 +00:00

provided it works well with user_profile.vue (not sure tbh; frontend stuff isn’t my strong suit), it would imho be cleaner to properly distinguish one’s own favourites from publicly accessible favourites here and get rid off the userId checks and mangling in the other parts, i.e.:

-      // only we can see our own favourites
       if (this.isUs) timelineTabMap['favorites'] = 'favorites'
+      else if(this.favoritesTabVisible) timelineTabMap['favorites'] = 'publicFavorites'

provided it works well with `user_profile.vue` *(not sure tbh; frontend stuff isn’t my strong suit)*, it would imho be cleaner to properly distinguish one’s own favourites from publicly accessible favourites here and get rid off the `userId` checks and mangling in the other parts, i.e.: ```diff - // only we can see our own favourites if (this.isUs) timelineTabMap['favorites'] = 'favorites' + else if(this.favoritesTabVisible) timelineTabMap['favorites'] = 'publicFavorites' ```

Oneric commented

2024-05-12 23:44:17 +00:00

oh also, can you add: Fixes: https://akkoma.dev/AkkomaGang/akkoma-fe/issues/389 as the last line of the commit message? This way the issue will be closed automatically (just mentioning it in the PR comment doesn't actually to work ime)

oh also, can you add: `Fixes: https://akkoma.dev/AkkomaGang/akkoma-fe/issues/389` as the last line of the commit message? This way the issue will be closed automatically (just mentioning it in the PR comment doesn't actually to work ime)