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

Tan 3576/useideastatuses variant usage #10100

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

brentguf
Copy link
Contributor

@brentguf brentguf commented Jan 17, 2025

Make participation_method query param required for useIdeaStatuses.

This work started because I thought there was a bug, but there wasn't. It's still an improvement worth merging, IMO. We'll have more explicit code that potentially prevents bugs (in case someone forgets to add the participation_method).

Changelog

@brentguf brentguf marked this pull request as draft January 17, 2025 16:44
Copy link

@brentguf brentguf marked this pull request as ready for review January 21, 2025 07:41
@brentguf brentguf requested a review from IvaKop January 21, 2025 07:54
Copy link
Contributor

@IvaKop IvaKop left a comment

Choose a reason for hiding this comment

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

I agree with the idea of making the participation method more explicit but in this case it appears you are making now two requests to get the statuses separately which is more inefficient. Is there a way to get all the statuses at once and map them as options? Or is the ordering the issue here?

Also, I see the smart group component is specifically about idea statuses. I'm sure you considered this but just to double-check that we are sure it also works well with proposals.

@brentguf
Copy link
Contributor Author

brentguf commented Jan 21, 2025

@IvaKop

I agree with the idea of making the participation method more explicit but in this case it appears you are making now two requests to get the statuses separately which is more inefficient. Is there a way to get all the statuses at once and map them as options? Or is the ordering the issue here?

There was no issue if with "issue" you meant a bug.

Smart group creation was the only instance left where we were not passing a participation_method when using useIdeaStatuses. We were getting all statuses (both ideation and proposals statuses) at once. This PR makes things consistent by requiring the participation_method param, which means we'll have two requests now, indeed.

I think the performance cost of two requests vs. one request here is negligible. The requests only occur in the back office when we've selected the status filter on smart group creation and the number of statuses is usually limited.

Also, I see the smart group component is specifically about idea statuses. I'm sure you considered this but just to double-check that we are sure it also works well with proposals.

It's a list of ideation and proposal statuses combined. With the new code, we still list both ideation and proposal statuses, indeed. 🙂

@IvaKop
Copy link
Contributor

IvaKop commented Jan 21, 2025

@brentguf I see, thank you for the context and the explanation. 🙌
I think it would be preferable to have a mechanism to fetch all the statuses at once instead of having to make separate API calls. I agree the performance cost is low but there is also added complexity that comes with fetching them separately. In addition, the approach you are using will not work easily with pagination (which, granted, is not a problem today).

I agree with you the old approach is not super explicit, though. I suggest we add a third option for participation_method - something like all, instead of having to fetch them separately and then re-combine them. Adding all as an option feels like a minimal change that would keep in place the benefit of your work while circumventing the issues above. Wdyt?

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.

2 participants