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

Bug(Sales): MYO made characters show old MYO image #615

Merged
merged 7 commits into from
Jun 15, 2023

Conversation

AW0005
Copy link
Contributor

@AW0005 AW0005 commented Jun 4, 2023

Sales image attribute references the "first" image for a character, which means that if you have a character made on a MYO slot with the default image still attached to them, that's what shows up in the sale image instead of the correctly updated active image.

Switching the attribute to reference the attached character's set active image instead fixes it up.

This is somewhere in the grey area of being a bug vs a feature enhancement but I figured it was close enough to unwanted behavior to title it a bug.

@itinerare itinerare added bug Something isn't working needs review Pull requests that are pending community review enhancement New feature or request and removed bug Something isn't working labels Jun 4, 2023
@itinerare
Copy link
Collaborator

Hmmm. I think probably the thing to do would just to make it toggleable, as I feel strongly that it should remain the "first" image for archival purposes and think having a sales post with a character from a MYO slot profoundly weird, but I also recognize that that is not the beginning and end of use cases for the thing.

@SpeedyD
Copy link
Contributor

SpeedyD commented Jun 4, 2023

I second @itinerare's suggestion. If you add a toggle, this is an instant approve from me. For archival purposes, we'd prefer the old image..

..In all honesty, I feel like there may need to be something more intricate to preserve old sales, but using first is the easiest way to do it rn.

@itinerare
Copy link
Collaborator

A potential option re that is recording the image ID at the time the character is added to the sale (probably the best balance between ease-of-use and solving this kind of issue/covers the most use cases most readily), but that would be rather more work than just setting up a toggle.

@SpeedyD
Copy link
Contributor

SpeedyD commented Jun 4, 2023

For now, I'm suggesting @AW0005 just adds a toggle, and maybe we can look into that another time, @itinerare. :)

@itinerare
Copy link
Collaborator

Yeah, hence the [or you could just add a toggle and be done with it].

@AW0005
Copy link
Contributor Author

AW0005 commented Jun 4, 2023

@itinerare @SpeedyD
Updated to track the character image id at time of sale listing. I wasn't fond of the idea of leaving in something that works by coincidence over going ahead and implementing something that should work as expected for all cases.

app/Models/Sales/SalesCharacter.php Outdated Show resolved Hide resolved
app/Services/SalesService.php Outdated Show resolved Hide resolved
@SpeedyD
Copy link
Contributor

SpeedyD commented Jun 4, 2023

...I have nothing to add, I think Merc got everything that needed to be said. 😅

@itinerare itinerare added reviewed Pull requests that have received community review and are pending merge and removed needs review Pull requests that are pending community review labels Jun 14, 2023
app/Services/SalesService.php Outdated Show resolved Hide resolved
@itinerare itinerare added needs review Pull requests that are pending community review and removed reviewed Pull requests that have received community review and are pending merge labels Jun 14, 2023
@itinerare
Copy link
Collaborator

It gets us all sooner or later. |D

@itinerare itinerare removed the needs review Pull requests that are pending community review label Jun 15, 2023
@itinerare itinerare added the reviewed Pull requests that have received community review and are pending merge label Jun 15, 2023
@itinerare itinerare merged commit d34c842 into corowne:develop Jun 15, 2023
SpeedyD added a commit to SpeedyD/lorekeeper that referenced this pull request Jun 15, 2023
…fix/missing-character-sale

aka feat(sales): use character image at the time a character is added to a sale (corowne#615)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request reviewed Pull requests that have received community review and are pending merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants