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

Remove references to bing map #259 #451

Merged
merged 1 commit into from
Dec 23, 2024
Merged

Conversation

beesaferoot
Copy link
Contributor

  • Removes BingMapService (backend)
  • Removes bingMapKey field in MapSettings model
  • Removes front reference to bing map Key

Brief summary of the change made

Partially closes #259

Are there any other side effects of this change that we should be aware of?

Describe how you tested your changes?

Pull Request checklist

Please confirm you have completed any of the necessary steps below.

  • Meaningful Pull Request title and description
  • Changes tested as described above
  • Added appropriate documentation for the change.
  • Created GitHub issues for any relevant followup/future enhancements if appropriate.

- Removes BingMapService (backend)
- Removes bingMapKey field in MapSettings model
- Removes front reference to bing map Key
@beesaferoot beesaferoot merged commit fa63d9e into main Dec 23, 2024
10 checks passed
@beesaferoot beesaferoot deleted the remove-bingmaps-refs branch December 23, 2024 08:46
Comment on lines 21 to 22
$table->string('provider')->nullable();
$table->string('bingMapApiKey')->nullable();
$table->timestamps();
Copy link
Member

Choose a reason for hiding this comment

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

Generally, I'm not a fan of changing migration files retrospectively.

This will create confusion in the long run. For example, this change (i.e. the removal of column bingMapApiKey) will not be applied to existing instances of MPM. Only new instances will have the column removed.

I think it's always better to create a new migration file for this, this also allows us to roll back changes if need be.

Could you send a new PR that restores this file and create a new migration to remove the column? You can use php artisan make:migration-tenant to get a template of the migration.
`

Copy link
Contributor Author

@beesaferoot beesaferoot Dec 23, 2024

Choose a reason for hiding this comment

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

I went with the assumption there will be breaking changes that require us to do a fresh migration. Not a good approach, I must admit.

Could you send a new PR that restores this file and create a new migration to remove the column? You can use php artisan make:migration-tenant to get a template of the migration.

I will apply a new migration accordingly, thanks.

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.

[Bug]: Remove all BingMaps leftovers
3 participants