perforce: Display changelists in repo and view commits page by indradhanush · Pull Request #51195 · sourcegraph/sourcegraph-public-snapshot

indradhanush · 2023-04-27T04:19:02Z

What

Part of #46488. Blocked by #51291.

We want to display the perforce changelists in the repo landing page and the view commits page instead of the commit SHA because to a perforce developer looking at this code, a commit SHA is meaningless.

Description of the changes

UI changes

For perforce depots converted to git:

Repo landing and the view commits pages now display changelist, changelists and changelist ID instead of commit, commits and commit SHA respectively
View commits page now displays the changelist ID instead of the commit SHA
Commit subject now displays the original changelist subject instead of the git commit message
Commit view page also displays the changelist related terms with changelist ID instead of the commit SHA and parent commit SHA

Also fixed the typos in the word committed across the whole codebase as I noticed it started to fail when I made a change in the existing code.

Meta UI changes

My UI skills are nascent and come with some limitations:

Instead of riddling the existing GitCommitNode component with if else conditions to display commit/s or changelist/s, I decided to create a new component which is pretty much a copy-paste of the GitCommitNode component with changes to display perforce terms and some minor changes to tweak the CSS and deleting some lines of code which were obviously not related to Perforce (some GitHub related checks)
I thought about adding an argument to the GitCommitNode to customise the message text, but I wanted to add the least amount of friction possible to the current code in case I end up breaking something, also passing changelist to a commitnode component felt wrong
getRefType: This function returns commit or changelist based on the repo type and I have two kinds of implementations in two different modules

I'm sure there are ways to do this cleaner in less lines of code and am happy to iterate on those. :)

Edit: I am reusing the GitCommitNode component. This was easy.

Run sg run storybook and check out the storybooks here:

http://localhost:9001/?path=/story/web-perforcedepotchangelistnode--perforce-depot-changelist-item

http://localhost:9001/?path=/story/web-repositorycommitspage--git-commits-story

http://localhost:9001/?path=/story/web-repositorycommitspage--perforce-changelists-story

Previously

Now

Test plan

Added tests
Added storybooks
Builds should pass

sg-e2e-regression-test-bob · 2023-04-27T04:26:12Z

Bundle size report 📦

Initial size	Total size	Async size	Modules
0.00% (+0.05 kb)	0.01% (+1.37 kb)	0.01% (+1.32 kb)	0.00% (0)

Look at the Statoscope report for a full comparison between the commits 32aa12d and 86a234d or learn more.

Open explanation

Initial size is the size of the initial bundle (the one that is loaded when you open the page)
Total size is the size of the initial bundle + all the async loaded chunks
Async size is the size of all the async loaded chunks
Modules is the number of modules in the initial bundle

indradhanush · 2023-04-27T04:33:21Z

Return the commit SHA instead of failed to parse for now. Create a GitHub issue to track this for the future. - @peterguy

indradhanush · 2023-04-27T04:35:13Z

TODO: Test a changelist with multiple depot-paths and verify that this pattern still works for it if that's the case.

indradhanush · 2023-04-27T17:49:52Z

@keegancsmith Requesting review for the minimal but important backend API changes.

peterguy · 2023-04-28T05:16:44Z

After clicking on a particular commit in the "View changelists from this repository" view, the wording is still somewhat Git-centric. See attached screenshot. Can any of that be changed also?

sashaostrikov

Amazing job!
Love the storybook coverage and backend tests 👍🏻
I agree with Peter's note about another UI view which should be covered with commit/changeset duality :)

Left some comments, PTAL.

sashaostrikov · 2023-04-28T10:00:16Z

 	return &repoUrl
 }
+
+var p4FusionCommitSubjectPattern = regexp.MustCompile(`^(\d+) - (.*)$`)


should we use a lazyregexp here and on L492?

Hmm. TIL about lazyregexp. I think I prefer using regexp.MustCompile because it will guarantee an invalid regex can never be compiled, which I think is missing with lazyregexp.

Let's use lazyregexp for its benefit of being lazy and cover the validity by a unit test (which will fail in case of an invalid regex being used). Anyway, the regex is static and once it compiles during tests (during CI run) before being merged to main, we're safe in production.

WDYT?

indradhanush · 2023-04-28T14:17:13Z

@peterguy, @sashaostrikov As discussed on Slack, will be following up in a new PR with updates to the commit view page.

sashaostrikov

Nice, LGTM!

indradhanush · 2023-05-01T09:21:46Z

Update: The approach of modifying the the OID to return a perforce changelist ID breaks for use cases where we need to fetch details about the commit since subsequent API calls now start referring to the changelist ID as the "commit ID" which results in 404 requests.

I'm reworking the API changes, converting this to a draft and in the meantime please hold on to reviews.

indradhanush · 2023-05-02T14:00:19Z

@peterguy, @sashaostrikov Check this out - I reused the existing component and got rid of the code duplication. All a result of the API changes I made in #51291! How cool is that?! 😎

Also that means now the commit view page also gets these improvements! See below 👇🏽👇🏽👇🏽

The link to the parent also works, the only downside (not in scope of this work) is that the link still works through commit SHAs so you cannot:

Search by changelist ID (needs more work)
Actually view what's the changelist ID when you click "browse files at this changelist"

Because the resulting page still shows up with commit SHAs.

sourcegraph-bot · 2023-05-02T14:02:18Z

Codenotify: Notifying subscribers in CODENOTIFY files for diff d60c0c3...32aa12d.

Notify	File(s)
@BolajiOlajide	client/web/src/enterprise/batches/repo/BatchChangeRepoPage.story.tsx
@courier-new	client/web/src/enterprise/batches/repo/BatchChangeRepoPage.story.tsx
@eseliger	client/web/src/enterprise/batches/repo/BatchChangeRepoPage.story.tsx
@fkling	client/branded/src/search-ui/components/CommitSearchResult.tsx
@limitedmage	client/branded/src/search-ui/components/CommitSearchResult.tsx

keegancsmith · 2023-05-02T14:23:55Z

Removing myself since there are no go changes now.

indradhanush · 2023-05-02T14:24:36Z

@keegancsmith Sorry about that. The Go changes have now moved to this PR: https://github.com/sourcegraph/sourcegraph/pull/51291.

sashaostrikov

hmm, I thought I already approved it after latest changes...

Approved now!

indradhanush · 2023-05-05T09:24:21Z

hmm, I thought I already approved it after latest changes...

I need to rework this PR after the backend API changed quite a bit based on reviews in #51291.

Moving this to draft for the time being.

Co-authored-by: Milan Freml <kopancek@users.noreply.github.com>

sashaostrikov

Last fixes LGTM!

sourcegraph-bot · 2023-05-09T07:12:58Z

📖 Storybook live preview

sashaostrikov

Took another look! 👍🏻

cla-bot Bot added the cla-signed label Apr 27, 2023

indradhanush commented Apr 27, 2023

View reviewed changes

indradhanush force-pushed the ig/perforce-changelists branch from c72cefb to c515764 Compare April 27, 2023 16:56

indradhanush marked this pull request as ready for review April 27, 2023 17:48

indradhanush changed the title ~~[WIP] Perforce changelists~~ perforce: Display changelists in repo and view commits page Apr 27, 2023

indradhanush requested review from a team, keegancsmith and peterguy April 27, 2023 17:49

sashaostrikov reviewed Apr 28, 2023

View reviewed changes

indradhanush requested a review from sashaostrikov April 28, 2023 14:14

sashaostrikov approved these changes May 1, 2023

View reviewed changes

indradhanush marked this pull request as draft May 1, 2023 09:21

indradhanush mentioned this pull request May 1, 2023

graphqlbackend: Support perforce changelists in repo and commits API #51291

Merged

indradhanush force-pushed the ig/perforce-changelists branch from a2ca952 to d15b441 Compare May 2, 2023 09:00

indradhanush changed the base branch from main to ig/perforce-changelists-api May 2, 2023 09:00

indradhanush marked this pull request as ready for review May 2, 2023 14:00

indradhanush requested review from a team and sashaostrikov May 2, 2023 14:00

keegancsmith removed their request for review May 2, 2023 14:24

indradhanush force-pushed the ig/perforce-changelists-api branch from b90ebe8 to 529c8d0 Compare May 3, 2023 10:46

Base automatically changed from ig/perforce-changelists-api to main May 4, 2023 17:18

sashaostrikov approved these changes May 5, 2023

View reviewed changes

indradhanush marked this pull request as draft May 5, 2023 09:24

web: Use changelist(s) for perforce depots

77f97f4

Co-authored-by: Milan Freml <kopancek@users.noreply.github.com>

indradhanush force-pushed the ig/perforce-changelists branch from 51d722d to 77f97f4 Compare May 8, 2023 17:15

indradhanush added 2 commits May 9, 2023 12:03

Fix type checks, bad imports and lints

9b72947

Fix tests

32aa12d

indradhanush marked this pull request as ready for review May 9, 2023 07:02

indradhanush requested a review from a team May 9, 2023 07:02

sashaostrikov approved these changes May 9, 2023

View reviewed changes

indradhanush merged commit fe73c1d into main May 9, 2023

indradhanush deleted the ig/perforce-changelists branch May 9, 2023 07:31

Conversation

indradhanush commented Apr 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What

Description of the changes

Test plan

Uh oh!

sg-e2e-regression-test-bob commented Apr 27, 2023 • edited by sourcegraph-buildkite Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Bundle size report 📦

Uh oh!

Uh oh!

indradhanush Apr 27, 2023

Choose a reason for hiding this comment

Uh oh!

indradhanush Apr 27, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

indradhanush commented Apr 27, 2023

Uh oh!

peterguy commented Apr 28, 2023

Uh oh!

sashaostrikov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sashaostrikov Apr 28, 2023

Choose a reason for hiding this comment

Uh oh!

indradhanush Apr 28, 2023

Choose a reason for hiding this comment

Uh oh!

sashaostrikov May 1, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

indradhanush commented Apr 28, 2023

Uh oh!

sashaostrikov left a comment

Choose a reason for hiding this comment

Uh oh!

indradhanush commented May 1, 2023

Uh oh!

indradhanush commented May 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sourcegraph-bot commented May 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

keegancsmith commented May 2, 2023

Uh oh!

indradhanush commented May 2, 2023

Uh oh!

sashaostrikov left a comment

Choose a reason for hiding this comment

Uh oh!

indradhanush commented May 5, 2023

Uh oh!

sashaostrikov left a comment

Choose a reason for hiding this comment

Uh oh!

sourcegraph-bot commented May 9, 2023

Uh oh!

sashaostrikov left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

indradhanush commented Apr 27, 2023 •

edited

Loading

sg-e2e-regression-test-bob commented Apr 27, 2023 •

edited by sourcegraph-buildkite

Loading

indradhanush commented May 2, 2023 •

edited

Loading

sourcegraph-bot commented May 2, 2023 •

edited

Loading