Support project view --web with TTY

[harveysanders]

Fixes #8721

This PR fixes gh project view --web in TTY mode. The issue was that we were checking if web mode is enabled before prompting for the owner and project selections. This caused the prompt to be skipped and the resulting URL to be filled with empty values for owner and project.

Ex:

https://github.com/users//projects/0

The main fix was to move the prompt for owner and project selections to the top of the function, before checking if web mode is enabled.

I also removed the use of buildURL as all the GraphQL API responses already contain the URL for the project.

Let me know if buildURL is still needed for some reason and I'll refactor. I tested project view in both TTY and non-TTY modes and it's working as expected.

I add the url field in the GraphQL mocks in existing tests to mirror the real responses.

If you agree buildURL is not needed, I'll remove it and and it's associated test. It's only used in the test now.

[harveysanders]

For reference, here's the ProjectV2 object with the url field used on all the project queries:
https://github.com/harveysanders/cli/blob/313b482e7a24beaeb82a77ad31e3996a5f6228e4/pkg/cmd/project/shared/queries/queries.go#L1066-L1099

[williammartin]

Thanks for this, it generally looks really good. A couple of things jump out to me:

One downside of this approach is that if I provide the owner and project ID as flags, we make 2 API round trips in order to fetch the user/org info and then the project itself. Previously, it looks like buildURL only required 1 request.

One upside of this approach is that handles non-github.com URLs automatically because we're not trying to build them ourselves.

I feel like the upside strongly outweighs the downside here, and I don't think it's worth the maintenance burden of supporting a shortcircuit url building step if the flags are provided, so yes, please remove the buildURL function.

[harveysanders]

@williammartin Thanks for the prompt and thoughtful review.

Good catch on the extra HTTP call to fetch the project URL. I missed that while getting my bearings with the original code. I agree that removing the extra request is not worth the added maintenance, especially if the tool will support non-GitHub projects in the future.

I've removed buildURL and the associated test.

[harveysanders]

This is completely unrelated so feel free to table it, but starting today, the test for cli/api/http_client_test.go:225 is failing locally.

--- FAIL: TestHTTPClientSanitizeJSONControlCharactersC0 (0.00s)
    http_client_test.go:225: 
                Error Trace:    /[redacted]/cli/api/http_client_test.go:225
                Error:          Not equal: 
                                expected: "1^A 2^B 3^C 4^D 5^E 6^F 7^G 8^H 9\t A\r\n B\v C^L D\r\n E^N F^O"
                                actual  : "1^A 2^B 3^C 4^D 5^E 6^F 7^G 8\b 9\t A\r\n B\v C\f D\r\n E^N F^O"
                            
                                Diff:
                                --- Expected
                                +++ Actual
                                @@ -1,3 +1,3 @@
                                -1^A 2^B 3^C 4^D 5^E 6^F 7^G 8^H 9       A
                                - B
                                    C^L D
                                +1^A 2^B 3^C 4^D 5^E 6^F 7^G  9  A
                                + B
                                    C
                                      D
                                  E^N F^O
                Test:           TestHTTPClientSanitizeJSONControlCharactersC0
FAIL

I'm sure I'm missing something, but my actual output seems correct to me:

8\u0008 -> 8\b (backspace)

C\u000c -> C\f (form feed)

However, the tests expect 8^H and C^L respectively. Would you know why those are the expected values? I'm not sure if this is a bug or if I'm misunderstanding something.

[williammartin]

This is completely unrelated so feel free to table it, but starting today, the test for cli/api/http_client_test.go:225 is failing locally.

Reported here: #8668 thanks!

[harveysanders]

This is completely unrelated so feel free to table it, but starting today, the test for cli/api/http_client_test.go:225 is failing locally.

Reported here: #8668 thanks!

Ah, thanks!

[williammartin]

especially if the tool will support non-GitHub projects in the future.

Just to be clear, it relates to GitHub hosts that aren't github.com rather than non-GitHub projects.

More reading here: #8779

[harveysanders]

especially if the tool will support non-GitHub projects in the future.

Just to be clear, it relates to GitHub hosts that aren't github.com rather than non-GitHub projects.

More reading here: #8779

Ah got it. Thanks for the clarification and link 🙂

[williammartin]

No problem, just ping me here when you are ready for a re-review!

[harveysanders]

No problem, just ping me here when you are ready for a re-review!

@williammartin
Whooops, I forgot to hit the button earlier, but I have removed the buildURL function:
3c90a3f

[williammartin]

One minor suggestion, otherwise looks really good to me!

[williammartin]

Woohoo, thank you for your contribution!