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

ProposalPolice™ GH Actions Workflow #41038

Merged
merged 30 commits into from
Jul 26, 2024

Conversation

ikevin127
Copy link
Contributor

Details

The ProposalPolice™ will help promote higher quality proposals and a more informed and trust-based review process 🤝, improving baseline fairness and allowing community members and Expensify employees to better focus on getting shit done!

For more details check out:

Fixed Issues

$ #35108
PROPOSAL: #35108 (comment)

Tests

N/A

  • Verify that no errors appear in the JS console

Offline tests

N/A

QA Steps

N/A

  • Verify that no errors appear in the JS console

PR Author Checklist

  • I linked the correct issue in the ### Fixed Issues section above
  • I wrote clear testing steps that cover the changes made in this PR
    • I added steps for local testing in the Tests section
    • I added steps for the expected offline behavior in the Offline steps section
    • I added steps for Staging and/or Production testing in the QA steps section
    • I added steps to cover failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
    • I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
    • I tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
  • I included screenshots or videos for tests on all platforms
  • I ran the tests on all platforms & verified they passed on:
    • Android: Native
    • Android: mWeb Chrome
    • iOS: Native
    • iOS: mWeb Safari
    • MacOS: Chrome / Safari
    • MacOS: Desktop
  • I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
  • I followed proper code patterns (see Reviewing the code)
    • I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick)
    • I verified that the left part of a conditional rendering a React component is a boolean and NOT a string, e.g. myBool && <MyComponent />.
    • I verified that comments were added to code that is not self explanatory
    • I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
    • I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
      • If any non-english text was added/modified, I verified the translation was requested/reviewed in #expensify-open-source and it was approved by an internal Expensify engineer. Link to Slack message:
    • I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
    • I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is either coming verbatim from figma or has been approved by marketing (in order to get marketing approval, ask the Bug Zero team member to add the Waiting for copy label to the issue)
    • I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
    • I verified the JSDocs style guidelines (in STYLE.md) were followed
  • If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
  • I followed the guidelines as stated in the Review Guidelines
  • I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
  • I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
  • I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
  • I verified that if a function's arguments changed that all usages have also been updated correctly
  • If any new file was added I verified that:
    • The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
  • If a new CSS style is added I verified that:
    • A similar style doesn't already exist
    • The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))
  • If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
  • If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
  • If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
  • If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
  • If the PR modifies the UI (e.g. new buttons, new UI components, changing the padding/spacing/sizing, moving components, etc) or modifies the form input styles:
    • I verified that all the inputs inside a form are aligned with each other.
    • I added Design label and/or tagged @Expensify/design so the design team can review the changes.
  • If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
  • If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.

Screenshots/Videos

N/A

@ikevin127 ikevin127 requested a review from a team as a code owner April 25, 2024 21:59
@melvin-bot melvin-bot bot requested review from DylanDylann and removed request for a team April 25, 2024 21:59
Copy link

melvin-bot bot commented Apr 25, 2024

@DylanDylann Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

@roryabraham
Copy link
Contributor

Not much for a C+ to test here so I think we'll forego it this time @DylanDylann

Copy link
Contributor

@roryabraham roryabraham left a comment

Choose a reason for hiding this comment

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

@ikevin127 how would you feel about implementing this in TypeScript and using ts-node instead? Especially since our manual testing is kind of limited here that seems like a nice little safety net to make changing this code safer.

@ikevin127
Copy link
Contributor Author

@ikevin127 how would you feel about implementing this in TypeScript and using ts-node instead? Especially since our manual testing is kind of limited here that seems like a nice little safety net to make changing this code safer.

@roryabraham Sure, I'll get on this and hopefully will have something by tomorrow EOD.

Copy link
Contributor

@roryabraham roryabraham left a comment

Choose a reason for hiding this comment

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

The way to implement this that would rock the boat less would be to:

  1. Remove the added package.json
  2. add any missing dependencies (i.e: openai) to the main package.json as dev dependencies
  3. Move the scripts from scripts/proposal-police to .github/actions/javascript, create a GitHub Action for each (or maybe just one that you control the behavior of with inputs).
  4. Compile the JS action with ncc by adding it to buildActions.sh

I'm not sure that's the best way to configure all the GitHub Actions in this repo, but it is the established way. So I think we should either continue with that pattern or create a S/P/S statement to change it. Hopefully that makes sense 👍🏼

.github/scripts/proposal-police/package.json Outdated Show resolved Hide resolved
@roryabraham
Copy link
Contributor

Also, do you think it would be valuable to add some automated tests here? Asking earnestly, not rhetorically

@ikevin127
Copy link
Contributor Author

Also, do you think it would be valuable to add some automated tests here?

Yes, I think it would be a good thing. Will look into it after the TS conversion and changes from #41038 (review).

@ikevin127
Copy link
Contributor Author

The way to implement this that would rock the boat less would be to:

  1. Remove the added package.json
  2. add any missing dependencies (i.e: openai) to the main package.json as dev dependencies
  3. Move the scripts from scripts/proposal-police to .github/actions/javascript, create a GitHub Action for each (or maybe just one that you control the behavior of with inputs).
  4. Compile the JS action with ncc by adding it to buildActions.sh

I'm not sure that's the best way to configure all the GitHub Actions in this repo, but it is the established way. So I think we should either continue with that pattern or create a S/P/S statement to change it. Hopefully that makes sense 👍🏼

@roryabraham Did this, just pushed the changes to align with the established way we handle GH actions. Is there any way we could test if this is working so far ? I'm not entirely sure the import paths would work once compiled / run started.

Once we confirm that it's working as expected, I can move on to writing some tests.

Copy link
Contributor

@roryabraham roryabraham left a comment

Choose a reason for hiding this comment

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

Hey @ikevin127, thanks for making the changes so far. It seems like you're a bit confused by my previous suggestions, so let me try to clarify.

There is a difference between GitHub Workflows and GitHub Actions. It's confusing that collectively the CI/CD product offered by GitHub is called GitHub Actions 😂

Here's my 2 minute explanation... Workflows are triggered by events, and use the syntax you're using in your yaml files to list a series of jobs. Each job runs on it's own machine, and is comprised of a series of steps. There are also callable workflows that can be triggered from other workflows, and they are like a job in the calling workflow and also run on their own machine (or set of machines, if the callable workflow has multiple jobs).

Meanwhile, Actions are essentially callable scripts that can be called at the step level. This means that Actions are executed on the machine that calls them. There are composite actions and javascript actions (and also docker actions, but we don't use any in this repo).

Workflows and Actions are both defined in yaml, but have different syntaxes:

Now, let's talk about what I'd like you to do here:

  1. You should create simple action metadata files for each of the TS actions you wrote. example.
  2. You should also run npm i && npm run gh-actions-build. This will use vercel/ncc to process the TS files you wrote and create a bundled executable Node.js script in pure JS. This means that you no longer need to run a node install to run that script - you just need Node. it has all the dependencies built-in. These massive files will be at .github/actions/javascript/proposalPoliceComment/index.js and .github/actions/javascript/proposalPoliceCommentEdit/index.js, respectively. Commit them and push them to this PR.
    • Curious why we have to compile these actions? Read on
  3. Then, reintroduce GitHub Workflow(s) in .github/workflows that are triggered by adding or comment to or editing a comment on an issue. These workflows can call your reusable TS actions you created.

Hopefully that helps clarify things a bit. I didn't dive into a review of what we've got here because of these kind of fundamental misunderstandings. But fret not, we shall get there 🙂

@roryabraham
Copy link
Contributor

Regarding tests, feel free to check out the README in the workflow_tests directory

Copy link
Contributor

@roryabraham roryabraham left a comment

Choose a reason for hiding this comment

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

also npm ci is failing, I think because you need to reinstall node_modules and commit the diff in package-lock.json

@ikevin127
Copy link
Contributor Author

I applied the requested changes and after syncing with main and performing a clean install looks like npm ci is not failing anymore. Though there are still some issues with ❌ Validate Github Actions regarding some diffs 🤔

Not sure how should I fix that one, should I run npm run gh-actions-build and push all index.js files from all the scripts ?
So far I've only pushed the index.js files of the 2 proposal police scripts.

@roryabraham
Copy link
Contributor

Yep, gh-actions-build and commit diff in index.js. Since they're bundled with all dependencies inline if you change dependencies or shared libs they can all change, even if you didn't touch the source code for that action

@roryabraham
Copy link
Contributor

Ah yeah good catch and thanks for testing 👍

@shawnborton shawnborton removed the request for review from a team June 24, 2024 08:29
roryabraham
roryabraham previously approved these changes Jun 26, 2024
Copy link
Contributor

@roryabraham roryabraham left a comment

Choose a reason for hiding this comment

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

Looks great now 👍🏼

@marcochavezf back to you, what do we need to do to get this merged?

@ikevin127
Copy link
Contributor Author

Circleing back to the Important part of this #35108 (comment) where I detailed exactly how to setup the AI assistant part 🚀

Copy link
Contributor

@marcochavezf marcochavezf left a comment

Choose a reason for hiding this comment

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

LGTM! Just a minor requested change (I think)

}

// Verify that the comment is not empty
if (!payload.comment?.body.trim()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we also check if the comment begins with # Proposal? So we can only check the comments that contain a proposal, otherwise I think we can trigger the OpenAI API in all the comments

Copy link
Contributor Author

@ikevin127 ikevin127 Jun 26, 2024

Choose a reason for hiding this comment

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

@marcochavezf This is handled by the AI assistant on both new comments which checks if a proposal is following the template and also on proposal edits to check for significant changes.

otherwise I think we can trigger the OpenAI API in all the comments

That's correct as per AI assistant instructions currently we run it on all comments of issues with Help Wanted label and if no proposal template is detected as part of the comment, the AI is instructed to respond in short with "NO_ACTION" and the bot don't do anything.

Can we also check if the comment begins with # Proposal?

This wouldn't be a good idea because currently for if AI assistant detects a proposal has significant changes, it will add a line like: Edited by proposal-police: This proposal was edited at 2024-06-22 22:28:40 UTC. at the top of the proposal which would break the logic proposed by you (see example in ikevin127/expensify-proposal-testing#1 (comment)).

For details like this we decided to rely on the AI assistant via instructions because we could have users add different proposal template markdown (or just text) which would still be valid like: Proposal, # Proposal, ## Proposal.

If we take a look at the given AI instructions you'll notice that we made sure to instruct for variations like these and also for if any other things are added above / below the proposal like the Edited by proposal-police... AI would look at the whole comment and if the proposal follows the template (+/- acceptable markdown / text variations), the AI would know about this and only respond if the proposal template is actually not followed (different RCA / solution / alt solution headings, ex: ikevin127/expensify-proposal-testing#1 (comment)).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we want to optimize this and not run the AI on every new comment, I guess we can be very strict and check if new issue comment contains # Proposal and only if true we run the AI, otherwise don't run at all ? But this would have the following edge case:

  • if user posts proposal using say ## Proposal or plain text Proposal and AI won't run on it -> nothing will happen (AI won't post the "you did not follow the proposal template" comment)

Copy link
Contributor

@marcochavezf marcochavezf Jun 27, 2024

Choose a reason for hiding this comment

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

What if we just check if the comment contains the word proposal (case-insensitive)? The problem is that we generate a considerable number of issues per month, and each issue could have multiple comments where it's not necessary to send an OpenAI API request

Copy link
Contributor Author

@ikevin127 ikevin127 Jun 27, 2024

Choose a reason for hiding this comment

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

Ya, I think that can definitely be done in order to reduce the amount of times we call the AI, therefore reduce costs. Will get on this tomorrow and make the change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@marcochavezf Done, synced w/ main and added the discussed change to make sure that the comment body includes the keyword Proposal case-sensitive.

@marcochavezf
Copy link
Contributor

Discussing internally the costs before setting the API keys as env variables

@marcochavezf
Copy link
Contributor

Asked to set up the API keys

Copy link
Contributor

@marcochavezf marcochavezf left a comment

Choose a reason for hiding this comment

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

The keys are set, but are set as PROPOSAL_POLICE_API_KEY and PROPOSAL_POLICE_ASSISTANT_ID

GITHUB_TOKEN:
description: 'Auth token for New Expensify Github'
required: true
OPENAI_API_KEY:
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's update this to PROPOSAL_POLICE_API_KEY

Suggested change
OPENAI_API_KEY:
PROPOSAL_POLICE_API_KEY:

OPENAI_API_KEY:
description: 'OpenAI API key for accessing AI services'
required: true
OPENAI_ASSISTANT_ID:
Copy link
Contributor

Choose a reason for hiding this comment

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

And this one to PROPOSAL_POLICE_ASSISTANT_ID

Suggested change
OPENAI_ASSISTANT_ID:
PROPOSAL_POLICE_ASSISTANT_ID:

private static assistantID: string;

static init(apiKey?: string, assistantID?: string) {
const key = apiKey ?? getInput('OPENAI_API_KEY', {required: true});
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const key = apiKey ?? getInput('OPENAI_API_KEY', {required: true});
const key = apiKey ?? getInput('PROPOSAL_POLICE_API_KEY', {required: true});

uses: ./.github/actions/javascript/proposalPoliceComment
with:
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
OPENAI_API_KEY: ${{ secrets.OPENAI_API_KEY }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
OPENAI_API_KEY: ${{ secrets.OPENAI_API_KEY }}
PROPOSAL_POLICE_API_KEY: ${{ secrets. PROPOSAL_POLICE_API_KEY }}

throw new Error('Could not initialize OpenAI: No key provided.');
}
this.ai = new OpenAI({apiKey: key});
this.assistantID = assistantID ?? getInput('OPENAI_ASSISTANT_ID', {required: true});
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
this.assistantID = assistantID ?? getInput('OPENAI_ASSISTANT_ID', {required: true});
this.assistantID = assistantID ?? getInput('PROPOSAL_POLICE_ASSISTANT_ID', {required: true});

with:
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
OPENAI_API_KEY: ${{ secrets.OPENAI_API_KEY }}
OPENAI_ASSISTANT_ID: ${{ secrets.OPENAI_ASSISTANT_ID }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
OPENAI_ASSISTANT_ID: ${{ secrets.OPENAI_ASSISTANT_ID }}
PROPOSAL_POLICE_ASSISTANT_ID: ${{ secrets. PROPOSAL_POLICE_ASSISTANT_ID }}

@ikevin127 ikevin127 changed the title ProposalPolice™ GH Actions Workflow [HOLD - Copy adjustments] ProposalPolice™ GH Actions Workflow Jul 5, 2024
@ikevin127
Copy link
Contributor Author

@marcochavezf I put this on HOLD because @mallenexpensify suggested copy changes related to the proposal enforcement comment on ND discussion.

Motivation for this can be found in ND discussion, stating:

I wonder if there's another way to phrase this that's a bit more friendly.

Here are the updated instructions ↩️

Simply replace all current OpenAI assistant instructions with the updated one from above and once confirmed, the HOLD can be removed and PR can be merged.

Note: The only change was on BOT ACTIONS -> 1. NEW COMMENTS where I changed the response copy in quotes from:

  • "{user} Your [proposal]({proposalLink}) will be dismissed because you did not follow the [proposal template](https://github.com/Expensify/App/blob/main/contributingGuides/PROPOSAL_TEMPLATE.md)." looking like this.

    to

  • "{user} Thanks for your [proposal]({proposalLink}). To be reviewed by Contributor+, please update your proposal comment using the [proposal template](https://github.com/Expensify/App/blob/main/contributingGuides/PROPOSAL_TEMPLATE.md)." looking like this.

No javascript code changes were required for this change.

@ikevin127
Copy link
Contributor Author

Merged w/ main and solved conflicts.

@roryabraham roryabraham changed the title [HOLD - Copy adjustments] ProposalPolice™ GH Actions Workflow ProposalPolice™ GH Actions Workflow Jul 26, 2024
@roryabraham roryabraham requested a review from marcochavezf July 26, 2024 01:08
Copy link
Contributor

@marcochavezf marcochavezf left a comment

Choose a reason for hiding this comment

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

Nice! The instructions were updated. Let's put it into action!

@marcochavezf
Copy link
Contributor

marcochavezf commented Jul 26, 2024

Reviewer Checklist

  • I have verified the author checklist is complete (all boxes are checked off).
  • I verified the correct issue is linked in the ### Fixed Issues section above
  • I verified testing steps are clear and they cover the changes made in this PR
    • I verified the steps for local testing are in the Tests section
    • I verified the steps for Staging and/or Production testing are in the QA steps section
    • I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
    • I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
  • I checked that screenshots or videos are included for tests on all platforms
  • I included screenshots or videos for tests on all platforms
  • I verified tests pass on all platforms & I tested again on:
    • Android: Native
    • Android: mWeb Chrome
    • iOS: Native
    • iOS: mWeb Safari
    • MacOS: Chrome / Safari
    • MacOS: Desktop
  • If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
  • I verified proper code patterns were followed (see Reviewing the code)
    • I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
    • I verified that the left part of a conditional rendering a React component is a boolean and NOT a string, e.g. myBool && <MyComponent />.
    • I verified that comments were added to code that is not self explanatory
    • I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
    • I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
    • I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
    • I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is either coming verbatim from figma or has been approved by marketing (in order to get marketing approval, ask the Bug Zero team member to add the Waiting for copy label to the issue)
    • I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
    • I verified the JSDocs style guidelines (in STYLE.md) were followed
  • If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
  • I verified that this PR follows the guidelines as stated in the Review Guidelines
  • I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
  • I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
  • I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
  • If a new component is created I verified that:
    • A similar component doesn't exist in the codebase
    • All props are defined accurately and each prop has a /** comment above it */
    • The file is named correctly
    • The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
    • The only data being stored in the state is data necessary for rendering and nothing else
    • For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
    • Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
    • All JSX used for rendering exists in the render method
    • The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
  • If any new file was added I verified that:
    • The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
  • If a new CSS style is added I verified that:
    • A similar style doesn't already exist
    • The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
  • If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
  • If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
  • If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
  • If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
  • If the PR modifies the UI (e.g. new buttons, new UI components, changing the padding/spacing/sizing, moving components, etc) or modifies the form input styles:
    • I verified that all the inputs inside a form are aligned with each other.
    • I added Design label and/or tagged @Expensify/design so the design team can review the changes.
  • If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
  • If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
  • I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop

@marcochavezf marcochavezf merged commit cc5a542 into Expensify:main Jul 26, 2024
19 checks passed
@OSBotify
Copy link
Contributor

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

@ikevin127
Copy link
Contributor Author

ikevin127 commented Jul 26, 2024

Curious to see if this actually works first try (merge) because with the same GH actions config I got errors in expensify-proposal-testing and had to adjust the scripts in order for the workflow to run error free 🤞

Looks like we got it on the first try 🎉 (below are some screenshots of the workflow running as expected)

Screenshot 2024-07-26 at 11 43 25 Screenshot 2024-07-26 at 11 42 22 Screenshot 2024-07-26 at 11 42 47

@OSBotify
Copy link
Contributor

🚀 Cherry-picked to staging by https://github.com/roryabraham in version: 9.0.14-1 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@Expensify/applauseleads please QA this PR and check it off on the deploy checklist if it passes.

@OSBotify
Copy link
Contributor

🚀 Deployed to production by https://github.com/roryabraham in version: 9.0.14-6 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

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.

4 participants