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

Fix/org choices #31

Merged
merged 9 commits into from
Dec 5, 2024
Merged

Fix/org choices #31

merged 9 commits into from
Dec 5, 2024

Conversation

Keyrxng
Copy link
Contributor

@Keyrxng Keyrxng commented Nov 27, 2024

Resolves #26
Requires #25

Changes:

  1. rest.users.getAuthenticated()
  2. rest.orgs.listForAuthenticatedUser()
  3. rest.orgs.listForUser(username)
  4. rest.repos.getCollaboratorPermissionLevel(org, .ubiquity-os)

QA:

image

Console:

image

Else:

(4) ['ubq-testing', 'ubiquity', 'ubiquibot', 'ubiquity-os']

I tried every .rest.org method that seemed relevant and none of them returned -marketplace for me so idk if that's a me problem or an org problem but it should be fetched at least and either errored in the console or displayed but it's neither. It also didn't display for you either @gentlementlegen and we are both public members of the org.

Could you try to see if -marketplace shows up for you now with this change?

@gentlementlegen
Copy link
Member

@Keyrxng Could we have preview deployments like we have for other frontend projects?

@gentlementlegen
Copy link
Member

The orgs show up properly on my side
image

Bug list

  • When you maintain a click on the set defaults, here it what happens
image
  • You can click and infinite amount of times on the "add" button
image
  • the toast asking you if you want to save disappear even when hovered
  • it set the value to "undefined" for OpenAiBaseUrl which I don't think is the desired behavior

@Keyrxng
Copy link
Contributor Author

Keyrxng commented Nov 29, 2024

The orgs show up properly on my side

Okay that's fine then ty

When you maintain a click on the set defaults, here it what happens

Will address that

You can click and infinite amount of times on the "add" button

If you clicked add but made a mistake/wanted to change a param, you'd want to be able to click add again yeah? But otherwise it should only allow you to click add once?

  1. user clicks add/remove > user clicks push to github > A) render pluginSelector B) disable add/remove
  2. user clicks add/remove > user closes the notification or it disappears > do nothing

the toast asking you if you want to save disappear even when hovered

  1. It should not disappear at all and requires the user to close or push?
  2. If hovered, do not disappear else disappear?

it set the value to "undefined" for OpenAiBaseUrl which I don't think is the desired behavior
Will address that

@Keyrxng Keyrxng mentioned this pull request Nov 29, 2024
@Keyrxng
Copy link
Contributor Author

Keyrxng commented Nov 29, 2024

@gentlementlegen

Is the preferred behaviour to be this:
image

or for the prop to not be included in the config?

@gentlementlegen
Copy link
Member

@Keyrxng Not to be included so the default value is used on Decode call. If set to an empty string the value then would be "" not the default.

@Keyrxng
Copy link
Contributor Author

Keyrxng commented Nov 29, 2024

the toast asking you if you want to save disappear even when hovered

If hovered, do not disappear else disappear?

Implemented

When you maintain a click on the set defaults, here it what happens

Will address that

Addressed

user clicks add/remove > user clicks push to github > A) render pluginSelector B) disable add/remove

Implemented:

  • user clicks add/remove > user closes the notification or it disappears > do nothing
  • user clicks add/remove > user clicks push to github > render pluginSelector

I'm unsure how you want me to handle multiple add clicks prior to push, please lmk if I should use another approach. Perhaps updating the values directly from input change and refactoring add into the push event and removing the notification as one alt? If you could create a new task for it that would be ideal as it'll involve a bit of work, ty.

@Keyrxng
Copy link
Contributor Author

Keyrxng commented Nov 29, 2024

@zugdev fancy doing a review bud?

@rndquu rndquu self-requested a review November 29, 2024 16:07
Copy link
Member

@rndquu rndquu left a comment

Choose a reason for hiding this comment

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

Getting 3 out of 4 expected organizations:
Screenshot 2024-11-29 at 19 09 30

The only missing org is https://github.com/rndquu-org where my account is an owner. It used to be displayed in the previous PRs but this PR somehow doesn't display it.

Pls fix.

@rndquu rndquu marked this pull request as draft November 29, 2024 16:11
@Keyrxng
Copy link
Contributor Author

Keyrxng commented Nov 29, 2024

The only missing org is https://github.com/rndquu-org where my account is an owner. It used to be displayed in the previous PRs but this PR somehow doesn't display it.

@rndquu your membership is private so it would only be returned by listForAuthenticatedUser, can you confirm that it returns it from that call pls? Just like how -marketplace doesn't appear anywhere for me but it's showing up for @gentlementlegen makes me believe that it can be settings related which is almost out of our control when considering arbitrary partner orgs.

Perhaps it's the permission === "write" check or something I'm not sure, but it's difficult for me to debug your conditions

@rndquu
Copy link
Member

rndquu commented Nov 29, 2024

@rndquu your membership is private so it would only be returned by listForAuthenticatedUser

The missing https://github.com/rndquu-org is displayed in this PR, so it's smth code related.

@Keyrxng
Copy link
Contributor Author

Keyrxng commented Nov 29, 2024

Ty for the clarification I will look into it

@Keyrxng
Copy link
Contributor Author

Keyrxng commented Nov 29, 2024

I expect the issue stems from your org not having a config repo, which we rely on to identify permissions to work around OAuth errors with org settings. I duplicated the logic here.

This approach:

  • Returns all orgs from listForAuthenticatedUser.
  • Includes publicly fetched orgs that have a .ubiquity-os repo to check permissions.

An alternative would be to list all org repos and check any repo if .ubiquity-os does not exist. Let me know which approach you'd prefer.

Hopefully this has resolved it for you, again it's hard for me to debug your conditions locally so my apologies for the back and forth

@zugdev
Copy link

zugdev commented Nov 29, 2024

I think the previous push toaster should disappear if I press on add again

@zugdev
Copy link

zugdev commented Nov 29, 2024

This doesnt change anything for me, so not sure

@Keyrxng Keyrxng marked this pull request as ready for review November 30, 2024 00:39
@rndquu rndquu self-requested a review December 2, 2024 13:40
Copy link
Member

@rndquu rndquu left a comment

Choose a reason for hiding this comment

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

Works fine, 4 out of 4 organizations are displayed for me, as expected

@rndquu rndquu requested review from zugdev and removed request for zugdev December 2, 2024 13:46
@rndquu rndquu merged commit f3e6f62 into ubiquity-os:main Dec 5, 2024
4 checks passed
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.

Organization selection choices
4 participants