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

[import] mg exhibiton, box, location #935

Merged
merged 1 commit into from
Dec 20, 2023
Merged

Conversation

rastislav-chynoransky
Copy link
Contributor

Copy link
Contributor

@eronisko eronisko left a comment

Choose a reason for hiding this comment

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

No real blockers for me here, I'd just consider more specific field names.

]);

$item = Item::find('CZE:MG.rada_s_123-lomeni_s');
$this->assertEquals('BOX F04', $item->box);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not F04 (instead of BOX F04)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's how it should get displayed in artwork details on the FE side.
https://sbirky.moravska-galerie.cz/dielo/CZE:MG.U_14802

Copy link
Contributor

Choose a reason for hiding this comment

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

But is there a box not called BOX xx? (If not, the BOX part probably belongs to the presentation layer)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, in case of Design 2000+ exhibition the box field doesn’t contain BOX word.

{
Schema::table('items', function (Blueprint $table) {
$table->string('exhibition')->nullable();
$table->string('box')->nullable();
Copy link
Contributor

Choose a reason for hiding this comment

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

The box part feels specific to MG. Maybe exhibition_placement? Or even combining exhibition and box into a single JSON column exhibition, e.g.:

{
  "name": "BLACK DEPO",
  "placement": "BOX 04",
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You wouldn't be able to get aggregations for those fields if the data would be nested in json.

Schema::table('items', function (Blueprint $table) {
$table->string('exhibition')->nullable();
$table->string('box')->nullable();
$table->string('location')->nullable();
Copy link
Contributor

Choose a reason for hiding this comment

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

What kind of location is this? Could we use a more specific name? (In my mind, a location could be an address, country, current exhibition, GPS location,...)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's an identifier in the format of e.g. UPM/203/POSTMODERNA (specific to MG)

Base automatically changed from MG-13 to develop December 20, 2023 16:55
@rastislav-chynoransky rastislav-chynoransky merged commit 268de8b into develop Dec 20, 2023
1 check passed
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.

2 participants