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

v0.19 upgrade #1277

Merged
merged 9 commits into from
Nov 28, 2023
Merged

v0.19 upgrade #1277

merged 9 commits into from
Nov 28, 2023

Conversation

dessalines
Copy link
Member

@dessalines dessalines commented Nov 15, 2023

@MV-GH this is ready for you to look at.

To summarize my changes:

  • Upgraded to the new auth method.
  • Upgraded all the structs, and fixed some issues with the copy_generated_types script.
  • Added the new pagination_cursor method.
  • Ran a code analysis and fixed a lot of issues there.
  • Removed all the version-checking sorting enums.

I tested most things, and they seem to be working correctly.

Limitations:

  • No backwards-compatibility.
    • I didn't change the MINIMUM_VERSION in the code, because viewing the site does work.
  • The validate_auth sections of the code: I'm not entirely sure how they should work, but that new endpoint is deployed on voyager.lemmy.ml .

Next week we'll be testing some 0.19 on lemmy.ml to catch any last bugs, which will unfortunately probably break apps for a few days, before we do the 0.19.0 lemmy backend release.

I defer you to you on the best way to handle the upgrade to your new backwards-compatibility library / method. Even if we don't get it merged in time for this release, it'll still be great to have for future ones. I hope we don't get trapped in merge hell with our two PRs, but its not the end of the world if we do, we'll work through it. LMK anything I can do to help there.

cc @Nutomic

@MV-GH
Copy link
Collaborator

MV-GH commented Nov 15, 2023

My Api is getting pretty far too. Doing initial integration attempts. Not sure what the timeline is for v0.19. I probably could get it done in 2weeks. Really depends in what problems in run into.

btw this has a good guide for publishing to maven

https://central.sonatype.org/publish/publish-maven/

@dessalines
Copy link
Member Author

Cool. Its tough to say yet when we'll release v0.19, definitely not this week, but possibly next week. Obvi I'll make sure jerboa is ready before then. But for now at least I'm just planning on supporting v0.19, considering all the API changes.

If you get your backwards-compatible API done soonish tho, we can use that one instead / in favor.

@dessalines dessalines marked this pull request as ready for review November 27, 2023 02:05
@dessalines dessalines changed the title Partially done with v0.19 upgrade. v0.19 upgrade Nov 27, 2023
Copy link
Collaborator

@MV-GH MV-GH left a comment

Choose a reason for hiding this comment

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

I was originally gonna just take over the changes from this PR into mine. But that no longer seems manageable, rebase it will be then not easy for sure.

I don't agree with some of the analysis changes

For the verification changes, the validateAuth, , success -> good, 4XX -> bad, 500 -> retry. So if validateAuth has the same behaviour is good. I have emulate validateAuth with that getMentionsCall for my API (for 0.18).

Also might have missed it but there should also be changes to the settings, the removal of that one. The postnotif setting do we completely remove it our just the UI parts. (Idc tbh)

app/src/main/java/com/jerboa/JerboaAppState.kt Outdated Show resolved Hide resolved
app/src/main/java/com/jerboa/Utils.kt Outdated Show resolved Hide resolved
app/src/main/java/com/jerboa/api/Http.kt Outdated Show resolved Hide resolved
app/src/main/java/com/jerboa/model/LoginViewModel.kt Outdated Show resolved Hide resolved
@MV-GH
Copy link
Collaborator

MV-GH commented Nov 27, 2023

Also you will have to redo the generate Types thing, there were some changes again

@dessalines dessalines marked this pull request as draft November 27, 2023 21:52
@dessalines dessalines marked this pull request as ready for review November 28, 2023 17:17
@dessalines
Copy link
Member Author

K, addressed some of these above. Re-running copy_generated_types didn't seem to pick up anything new.

@dessalines
Copy link
Member Author

BTW I also tested image uploading on voyager.lemmy.ml from this PR (I removed that cookie stuff), and it works.

@MV-GH
Copy link
Collaborator

MV-GH commented Nov 28, 2023

I was trying to test this PR but voyager seems to be having issues atm. I will test it later

@MV-GH
Copy link
Collaborator

MV-GH commented Nov 28, 2023

Alright it works but it seems to constantly display, "this account is being verified" on return out of a post. (It should not even be verifying the account)

Using 0.18 accs seems to work better than I expected, I can browse but it does display the verification thingy stuck on jwt (prob cuz that doesn't fall back thus jwt verification fails as auth=null) (Maybe add a version warning? we can keep as is but we will definitely get issues around this)

Copy link
Collaborator

@MV-GH MV-GH left a comment

Choose a reason for hiding this comment

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

I can't reproduce the previous "The account is being verified" issue anymore but there is definitely an edgecase somewhere where it doesn't properly purge the lock

@dessalines
Copy link
Member Author

Using 0.18 accs seems to work better than I expected, I can browse but it does display the verification thingy stuck on jwt (prob cuz that doesn't fall back thus jwt verification fails as auth=null) (Maybe add a version warning? we can keep as is but we will definitely get issues around this)

Where would be the best place to add that toast?

@MV-GH
Copy link
Collaborator

MV-GH commented Nov 28, 2023

We had one in the root of our app by twiz, but that one is based of getSite which wouldn't work as getSite doesn't work across version. You could reuse it but add that nodeinfo stuff to get version but that's a lotta work. Or you could wait until my api is done.(merge as is, release in the meantime) Or we can use my LemmyApi.getVersion that does functionality but i would have to cut a prelease first.

@dessalines
Copy link
Member Author

I'll wait until your API is done, I'll just merge this as is. Again, sry about the conflicts, lmk if there's anything I can do to help there.

@dessalines dessalines merged commit 7cc5fdd into main Nov 28, 2023
1 check passed
@MV-GH
Copy link
Collaborator

MV-GH commented Nov 29, 2023

The hardest part that I still need to figure out, is that the API instance creation, now is suspended, (has to do a version call) meaning that it has todo that first before any other call, currently it doesn't adhere to that, which causes some race condition issues. As initial getSite / getPosts will done for the default.

Currently the approaches that I can think of are:

  • Loading screen, where it inits the api. Don't like this as when it fails the call it should allow the user to still navigate through the app and switch users.
  • do something like we do with account everywhere, observe its state and update relevant composables. Don't like this as we would have to do this everywhere
  • Make getInstance suspend until it is init. If init fails create default instance, but guess that it is the latest version. If it isn't no problem other calls fails but allows you to switch accounts etc. I prefer this approach. Seems the most ergonomic but I need to figure out the "suspend until init" part.

@dessalines
Copy link
Member Author

(3) Makes the most sense to me also. Could be a part of the changeInstance function, which I believe is called on startup, as well as when switching accounts.

@MV-GH
Copy link
Collaborator

MV-GH commented Nov 29, 2023

While fixing merge conflicts, I found some issues,

  • SaveUserSettings no longer returns LoginResponse it Returns successResponse now
  • MarkPostResponse no longer returns the post instead successResponse

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.

None yet

2 participants