Skip to content

2744: Collection minor issues - #3133

Merged
f1sh1918 merged 11 commits into
mainfrom
2744-collection-minor-issues
Sep 29, 2026
Merged

f1sh1918 merged 11 commits into
mainfrom
2744-collection-minor-issues

Conversation

@f1sh1918

@f1sh1918 f1sh1918 commented Sep 2, 2026 •

Copy link
Copy Markdown
Contributor

Short Description

We had some minor issues and design glitches, i've fixed here.

Proposed Changes

Side Effects

  • N/A

Testing

See issue description #2744

  • Test single card creation (gold and standard) and also bulk import cards. Best for all regions

Resolved Issues

Fixes: #2744

@f1sh1918
f1sh1918 force-pushed the 2744-collection-minor-issues branch from 1bbb250 to fd0aea2 Compare September 2, 2026 10:54
@github-actions

github-actions Bot commented Sep 2, 2026 •

Copy link
Copy Markdown

Coverage report for administration

St.❔
Category Percentage Covered / Total
🟡 Statements 78.09% 2505/3208
🔴 Branches 53.73% 648/1206
🟡 Functions 68.74% 431/627
🟡 Lines 77.3% 2323/3005

Test suite run success

510 tests passing in 58 suites.

Report generated by 🧪jest coverage report action from 21341ce

@f1sh1918
f1sh1918 marked this pull request as ready for review September 2, 2026 11:22
@seluianova

seluianova commented Sep 9, 2026 •

Copy link
Copy Markdown
Contributor

❓not sure if I correctly understand what was desired here:

🐛 No. 5 - Acceptance store management: When importing acceptance stores, the messages states that longitude and latitude are required. While the columns must be present, they don't need to contain any data. So they shouldn't be mentioned as required fields.

longitude and latitude were removed from the list:

image

but I would assume that we just need to remove the * from them?
because it’s still possible to load these fields; they’re just not mandatory.
or was the idea that we don’t want to allow users to upload pre-filled longitude and latitude at all?

@seluianova seluianova 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.

Looks nice to me 👍
One questionable point (in the comment above) and one unrelated finding:

I just noticed that we lost the usage of the dryRunImportResult translation at some point. We now show the same success message for both the dry run and the normal import. This might be adjusted separately, though, up to you.

Tested issues:

No. 2 - Project management (as external API user): The button for creating a new token IMO should be a primary button ✅
No. 3 - Application management: When deleting an application, a confirmation dialog is displayed. After confirmation, there is no notification shown (e.g. "Successfully deleted") ✅
No. 4 - Card creation: When importing cards from a CSV file, it is possible, to define a gold card with an expiry date ✅
No. 5 - Acceptance store management: When importing acceptance stores, the messages states that longitude and latitude are required ❓
No. 6 - Acceptance store management: When trying to import an empty file or a file with just the column titles, an error notification is shown. It feels like this notification vanishes much faster than others (approx. 2 seconds). ✅

@andrew8er andrew8er 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.

Loks good, nice cleanups.

@f1sh1918

Copy link
Copy Markdown
Contributor Author

❓not sure if I correctly understand what was desired here:

🐛 No. 5 - Acceptance store management: When importing acceptance stores, the messages states that longitude and latitude are required. While the columns must be present, they don't need to contain any data. So they shouldn't be mentioned as required fields.

longitude and latitude were removed from the list:

image but I would assume that we just need to remove the `*` from them? because it’s still possible to load these fields; they’re just not mandatory. or was the idea that we don’t want to allow users to upload pre-filled longitude and latitude at all?

Yes IMO the goal was to not show them as "required" fields but it should still be possible to upload csvs with that fields as optional fields @connium ?

@connium

connium commented Sep 24, 2026

Copy link
Copy Markdown

❓not sure if I correctly understand what was desired here:

🐛 No. 5 - Acceptance store management: When importing acceptance stores, the messages states that longitude and latitude are required. While the columns must be present, they don't need to contain any data. So they shouldn't be mentioned as required fields.

but I would assume that we just need to remove the * from them? because it’s still possible to load these fields; they’re just not mandatory. or was the idea that we don’t want to allow users to upload pre-filled longitude and latitude at all?

Yes IMO the goal was to not show them as "required" fields but it should still be possible to upload csvs with that fields as optional fields @connium ?

My intention was, to remove the required-indicator from the longitude and latitude, as it is with the other optional fields like email, homepage, … . The fields should still be mentioned in the list, as this is the specification of the import file.

I just recognized, that the label says "Erforderliche Spalten". 🙄

My goal is this:
Bildschirmfoto am 2026-09-24 um 11 25 05

Not quite sure about the "Maximalanzahl an Einträgen". We mention this for the card import. If there is no limit for acceptance stores, remove it.

@f1sh1918

f1sh1918 commented Sep 25, 2026 •

Copy link
Copy Markdown
Contributor Author

❓not sure if I correctly understand what was desired here:

🐛 No. 5 - Acceptance store management: When importing acceptance stores, the messages states that longitude and latitude are required. While the columns must be present, they don't need to contain any data. So they shouldn't be mentioned as required fields.

but I would assume that we just need to remove the * from them? because it’s still possible to load these fields; they’re just not mandatory. or was the idea that we don’t want to allow users to upload pre-filled longitude and latitude at all?

Yes IMO the goal was to not show them as "required" fields but it should still be possible to upload csvs with that fields as optional fields @connium ?

My intention was, to remove the required-indicator from the longitude and latitude, as it is with the other optional fields like email, homepage, … . The fields should still be mentioned in the list, as this is the specification of the import file.

I just recognized, that the label says "Erforderliche Spalten". 🙄

My goal is this: Bildschirmfoto am 2026-09-24 um 11 25 05

Not quite sure about the "Maximalanzahl an Einträgen". We mention this for the card import. If there is no limit for acceptance stores, remove it.

Alright it's done.
There is no entry limit. But we might come to its limits with 1000+ entries

image

@f1sh1918
f1sh1918 force-pushed the 2744-collection-minor-issues branch from 1a8d54b to 2fa9856 Compare September 25, 2026 09:25
@deliverino

deliverino Bot commented Sep 25, 2026 •

Copy link
Copy Markdown
Contributor

LLM Review (verdigado-think)

Administration (React/TypeScript)

Logic & Correctness

  • administration/src/cards/card.ts: The logic updates to handle "infinite lifetime" cards (Goldkarten) are correct. Specifically, updateCard now ensures that if a card is transitioned to a type with an infinite lifetime, the expirationDate is automatically cleared. This prevents a potentially invalid state where a Goldkarte has an expiration date.
  • administration/src/cards/card.ts: The updated isExpirationDateValid correctly returns false if a card has an infinite lifetime but still possesses an expiration date, which aligns with the new test cases in card.test.ts.

UI/UX & A11y

  • administration/src/components/FormAlert.tsx: Good addition of role='alert' for accessibility when the alert is not a toast.
  • administration/src/routes/stores/import/components/StoresImportAlert.tsx: The addition of <br /> at line 44 is a simple but effective fix for the misalignment mentioned in the commit history.
  • Consistency: The systematic replacement of MUI Alert components with the internal AlertBox across multiple views (ApplicationVerifierView, OrganizationForm, RegionForm) improves UI consistency.

Internationalization

  • All new user-facing strings (e.g., deleteApplicationSuccessMessage, columnFormat) have been correctly added to administration/src/translations/de.json and are accessed via t() or <Trans />.

Testing

  • administration/src/cards/card.test.ts: The new tests for updateCard and the validation of Goldkarten with/without expiration dates are comprehensive and cover the edge cases introduced in the logic.
  • administration/src/routes/stores/import/components/StoresRequirementsText.test.tsx: The test was correctly updated to reflect the change from "Erforderliche Spalten" to "Spaltenformat".

Whitelabel Correctness

  • The change to make FIELD_LATITUDE and FIELD_LONGITUDE optional in administration/src/project-configs/storesManagementConfig.ts is correctly placed within the project configuration rather than hardcoded in the logic.

Commit Style & Labels

  • Commit Messages: All commit messages follow the convention <issue number>: <message> (prefix 2744:), use the present tense, and are descriptive.
  • Labels: No labels are currently set. Since this PR contains user-facing changes (UI improvements, CSV import text changes, success messages), it should not have the maintenance or exclude-changelog labels. No suggestions needed.

@f1sh1918
f1sh1918 force-pushed the 2744-collection-minor-issues branch from 2fa9856 to 21341ce Compare September 25, 2026 09:30
@f1sh1918
f1sh1918 merged commit d58784d into main Sep 29, 2026
10 checks passed
@f1sh1918
f1sh1918 deleted the 2744-collection-minor-issues branch September 29, 2026 19:33
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.

Collection of minor issues in the administration area

4 participants