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

check if piece can be saved before making any changes #2140

Open
wants to merge 1 commit into
base: development
Choose a base branch
from

Conversation

Copy link
Contributor

@ValenberghsSven ValenberghsSven left a comment

Choose a reason for hiding this comment

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

ModelWithModifier has it's own save override. so does piece model. How does that work?
piece does not have a modified-by property in resource but is needed for ModelWithModifier. Is there a PR missing for app-kaleidos?

Does this work locally? many failing tests because POST calls of /pieces fails.

Implementing ModelWithModifier on any model will require more changes and consideration.
For piece, I am reluctant to just merge it because so many processes (like services) are changing pieces but not updating the modified or modified-by so this will not work unless we also update our services to do that.

I need to look at the actual issue that is going on here.
Ember's aggressive save is saving old "file" relations when backend already changed it.
We only want to save file relation on creation, or on manual replace.
In all other cases, we shouldn't try to save the file.

For now, we can try to fix the issues so the ModelWithModifier works, but I might look into alternatives.

For older concurrency issues on agendaitem and subcase I had to resort to reloading possible stale relations before the save. That might be a better solution unless I can detect when the relation was actually updated in the frontend and only save it then.


/**
* !when adding a new inverse relation (piece belongsTo/hasMany someModel), make sure to add the polymorphic type {as: 'piece'}
* !When setting the relation here or in the other model (someModel belongsTo/hasMany piece), make sure to add {polymorphic: true}
* Referencing a relation between piece and piece requires both
*/

export default class Piece extends Model {
Copy link
Contributor

Choose a reason for hiding this comment

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

small change, large impact.

Comment on lines +102 to +107
try {
const oldPiece = yield this.args.piece;
yield oldPiece.save();
} catch (e) {
return;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

why save here? I'm guessing to test concurrency?
if you are going to use ModelWithModifier you can just use preEditOrSaveCheck() to check if anyone else made changes to piece before you start the process below.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants