Skip to content

Internal/ci build react settings#330

Open
TLabutis wants to merge 3 commits into
masterfrom
INTERNAL/ci-build-react-settings
Open

Internal/ci build react settings#330
TLabutis wants to merge 3 commits into
masterfrom
INTERNAL/ci-build-react-settings

Conversation

@TLabutis

@TLabutis TLabutis commented Jun 4, 2026

Copy link
Copy Markdown
Contributor

Self-Checks

  • I have performed a self-review of my code.
  • I have updated/added necessary technical documentation in the README file.

JIRA task link

Summary

QA Checklist Labels

  • Bug fix?
  • New feature?
  • Improvement?
  • Technical debt?
  • Reusable?
  • Covered by tests?

QA Checklist

Additional Context

Frontend Changes

Justas and others added 3 commits February 24, 2026 13:52
The admin settings screen is a Vite/React app whose compiled bundle
(views/js/admin/dist) is gitignored. The packaging workflows only ran
composer, so released ZIPs shipped without saferpay-settings.js/.css,
leaving the Settings page blank.

Add a pnpm install + build step (pinned to pnpm 9 so esbuild's build
script runs) before zipping in release.yml, deploy.yml and create_zip.yml,
then strip node_modules/src. Mirrors the Mollie module pipeline.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request migrates the Saferpay module settings page to a modern React-based application, refactoring the admin controllers to handle AJAX requests for credentials, payment processing, email notifications, and payment methods. It also introduces a new service to fetch terminal IDs directly from the Saferpay API. The review feedback highlights several critical issues, including incorrect credential sanitization using pSQL() which can corrupt passwords, a state-handling bug in the React app when switching environments, a potential PHP 8 type error in payment method saving, type mismatches in multi-select ID comparisons, duplicate build steps in the Makefile, and unsafe object property access in the terminal retrieval service.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines +554 to +558
*/
private function getStringValue($data, $key)
{
return isset($data[$key]) ? pSQL((string) $data[$key]) : '';
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

Using pSQL() to sanitize configuration values is incorrect and dangerous. pSQL() is specifically designed for escaping SQL queries to prevent SQL injection. When saving values to PrestaShop's Configuration database table via $configuration->set(), PrestaShop handles the database escaping internally. Applying pSQL() here will corrupt credentials (such as API passwords, usernames, or tokens) if they contain special characters like single quotes or backslashes, leading to authentication failures.

    private function getStringValue($data, $key)
    {
        return isset($data[$key]) ? (string) $data[$key] : '';
    }

Comment on lines +48 to +52

useEffect(() => {
if (hasCredentials && terminals.length === 0) {
handleFetchTerminals()
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

The useEffect hook only triggers when environment changes, but it checks terminals.length === 0. Since the terminals state is not cleared when switching environments, switching from 'Test' to 'Live' (or vice versa) will retain the previous environment's terminals. This prevents the new environment's terminals from being fetched, causing the user to see and potentially save incorrect terminal IDs. Resetting and refetching the terminals properly on environment change fixes this.

  useEffect(() => {
    setTerminals([])
    if (username && password && customerId) {
      const fetchForEnv = async () => {
        setIsLoadingTerminals(true)
        try {
          const result = await fetchTerminals(environment, username, password, customerId)
          setTerminals(result)
        } finally {
          setIsLoadingTerminals(false)
        }
      }
      fetchForEnv()
    }
  }, [environment])

Comment on lines +248 to +251
if (!$data || !isset($data['paymentMethods'])) {
$this->ajaxResponse(false, 'Invalid request data');
return;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

In ajaxProcessSavePaymentMethods, isset($data['paymentMethods']) is checked, but it is not verified to be an array. If the request payload contains a non-array value for paymentMethods, the subsequent foreach loop will throw a TypeError in PHP 8. Adding an is_array check makes this more robust.

        if (!$data || !isset($data['paymentMethods']) || !is_array($data['paymentMethods'])) {
            $this->ajaxResponse(false, 'Invalid request data');
            return;
        }

Comment on lines +13 to +43
function MultiSelect({
options,
selected,
onChange,
placeholder,
label,
}: {
options: Array<{ id: number; name: string }>
selected: number[]
onChange: (values: number[]) => void
placeholder: string
label: string
}) {
const [search, setSearch] = useState('')

const filteredOptions = useMemo(
() => options.filter((opt) => opt.name.toLowerCase().includes(search.toLowerCase())),
[options, search],
)

const toggle = useCallback(
(value: number) => {
onChange(
selected.includes(value)
? selected.filter((s) => s !== value)
: [...selected, value],
)
},
[selected, onChange],
)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Mismatch between string IDs from the database and numeric IDs in React can cause strict comparison (includes) to fail. Normalizing selected to numbers ensures reliable matching.

function MultiSelect({
  options,
  selected,
  onChange,
  placeholder,
  label,
}: {
  options: Array<{ id: number; name: string }>
  selected: Array<number | string>
  onChange: (values: number[]) => void
  placeholder: string
  label: string
}) {
  const [search, setSearch] = useState('')
  const selectedNumbers = useMemo(() => selected.map(Number), [selected])

  const filteredOptions = useMemo(
    () => options.filter((opt) => opt.name.toLowerCase().includes(search.toLowerCase())),
    [options, search],
  )

  const toggle = useCallback(
    (value: number) => {
      onChange(
        selectedNumbers.includes(value)
          ? selectedNumbers.filter((s) => s !== value)
          : [...selectedNumbers, value],
      )
    },
    [selectedNumbers, onChange],
  )

Comment thread Makefile
rm -rf vendor && \
composer install --no-dev --optimize-autoloader && \
cp .github/.htaccess vendor/.htaccess && \
cd views/js/admin/settings-app && pnpm install && pnpm run build && cd ../../../.. && \

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The prepare-zip target duplicates the build steps for the React app. This is fragile and hard to maintain. It's better to reuse the build-react target.

	$(MAKE) build-react && \

);

$terminals = [];
$terminalList = isset($response->Terminals) ? $response->Terminals : [];

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Accessing $response->Terminals directly can trigger warnings or errors if $response is not an object (e.g., if the API call failed or returned an unexpected non-JSON response). It is safer to verify that $response is an object before checking for the property.

        $terminalList = (is_object($response) && isset($response->Terminals)) ? $response->Terminals : [];

@GytisZum GytisZum changed the base branch from master to release-2.0.3 June 17, 2026 08:53
@GytisZum GytisZum changed the base branch from release-2.0.3 to master June 17, 2026 08:53
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.

1 participant