story/VOGRE-54 #54
story/VOGRE-54 #54
Conversation
…utton dosent work after editing appellation, messages code added display not showing
|
Can one of the admins verify this patch? |
annotations/views/rest_views.py
Outdated
| concept = Concept.objects.get(uri=request.data['interpretation']) | ||
| instance.interpretation = concept | ||
| except Concept.DoesNotExist: | ||
| pass |
There was a problem hiding this comment.
an appellation without a concept should not be savable.
annotations/views/rest_views.py
Outdated
| if in_relations: | ||
| return Response( | ||
| {"detail": "Cannot delete an Appellation used in a Relation."}, | ||
| status=status.HTTP_403_FORBIDDEN |
There was a problem hiding this comment.
Forbidden is used for permission issues, here we maybe want to use 409 Conflict ("The 409 (Conflict) status code indicates that the request could not be completed due to a conflict with the current state of the target resource. This code is used in situations where the user might be able to resolve the conflict and resubmit the request. ")
annotations/views/rest_views.py
Outdated
| ) | ||
|
|
||
| self.perform_destroy(instance) | ||
| return Response(status=status.HTTP_204_NO_CONTENT) |
There was a problem hiding this comment.
if this indicates that the appellation has been deleted, it should return OK 200.
annotations/views/rest_views.py
Outdated
| logger.error(f"Error deleting appellation: {str(e)}", exc_info=True) | ||
| return Response( | ||
| {"detail": f"Error deleting appellation: {str(e)}"}, | ||
| status=status.HTTP_400_BAD_REQUEST |
There was a problem hiding this comment.
if this is for any exception that may happen, it should be 500.
…ed decorator in list relation template view
annotations/views/rest_views.py
Outdated
| if in_relations: | ||
| return Response( | ||
| {"detail": "Cannot delete an Appellation used in a Relation."}, | ||
| status=status.HTTP_409_CONFLICT |
There was a problem hiding this comment.
conflicts are typically send with there is a version of the sent resource that is different than the current one in the system (or different from what the sender based their changes on), so more like a GitHub conflict. Here I would send a bad request (since this should not be allowed in the first place to delete appellations used in a relation).
|
Added Julian as reviewer especially for the Vue stuff. |
| store.commit('resetCreateAppelltionsToText'); | ||
|
|
||
| // Notify parent components about deselection | ||
| this.$root.$emit('appellationDeselected', this.appellation); |
There was a problem hiding this comment.
It's fairly conflicting to use both Vuex and the $root instance to manage state. Generally considered a better idea to use Vuex but I would stick to one.
|
and now also conflicts |
annotations/views/rest_views.py
Outdated
| return True # Let has_object_permission handle it | ||
|
|
||
| if request.method in ['PUT', 'PATCH']: | ||
| return True # Let has_object_permission handle it |
There was a problem hiding this comment.
why not add all of the above to line 48? with a comment that includes the comments here?
| // Force a refresh of positions for updated appellation | ||
| this.$nextTick(() => { | ||
| this.updatePosition(); | ||
| EventBus.$emit('updateposition'); |
There was a problem hiding this comment.
where is this emitted event caught? Is that here?
If so, wouldn't there be two
updatePosition methods be called that are very similar?
Also, why are there two methods that are almost but not quite identical?
There was a problem hiding this comment.
This still. There seem to be two functions that are very similar.
There was a problem hiding this comment.
The functions do perform similar tasks, they take a representation text selection and use the utility function calculateOverlayPositions to convert those positions into screen coordinates suitable for showing the highlight or overlay over the text.
But they have a few differences:
updatePosition in AppellationDisplay calculates overlays for pre-existing, saved appellations (using this.appellation.position).
TextDisplay.js calculates overlays for the current, user selection (using this.selected) typically for creating new annotations or providing real-time selection feedback. It also includes logic to clear the selection display.
There was a problem hiding this comment.
I'm still confused. This code:
this.updatePosition();
EventBus.$emit('updateposition');
first calls below updatePosition function, right? Then it emits an event updateposition. Won't this event be caught in textdisplay.js line 45, which will then call the updatePosition function in textdisplay.js?
| this.deleteError = "Error deleting annotation, Please try again later."; | ||
| } | ||
|
|
||
| // Clear error message after 5 seconds |
There was a problem hiding this comment.
Error messages should not be automatically cleared. What if the user turns around or goes to get coffee and misses the error message?
| v-on:selectappellation="selectAppellation" | ||
| v-on:removeAppellation="removeAppellation($event)" | ||
| v-on:addAppellation="addAppellation($event)" | ||
| v-on:addAppellation="addAppellation($event)" |
…ion - will replace in textdisplay and appellation display
…and tests in this commit
| // Force a refresh of positions for updated appellation | ||
| this.$nextTick(() => { | ||
| this.updatePosition(); | ||
| EventBus.$emit('updateposition'); |
There was a problem hiding this comment.
This still. There seem to be two functions that are very similar.
| getFormattedDate: function (isodate) { | ||
| return moment(isodate).format('dddd LL [at] LT'); | ||
| } | ||
| if (typeof moment !== 'undefined') { |
There was a problem hiding this comment.
when would this happen? wouldn't this be something that can be avoided by ensuring moment is included?
| const deletedAppId = this.appellation.id; | ||
|
|
||
| if (typeof Appellation === 'undefined' || !Appellation.delete) { | ||
| this.deleteError = "Cannot delete: Appellation resource unavailable."; |
There was a problem hiding this comment.
What does this mean? Does this mean the appellation doesn't exist on the server? This is the error message that's being shown to the user, right? So it should be something the user can understand.
| try { | ||
| const editingApp = JSON.parse(editingAppData); | ||
| // Check if the parsed data has an id and it matches the deleted item's id | ||
| if (editingApp && typeof editingApp.id !== 'undefined' && editingApp.id === deletedAppId) { |
There was a problem hiding this comment.
is there a reason to use typeof editingApp.id !== 'undefined' over editingApp.id !== undefined?
There was a problem hiding this comment.
the typeof function returns the string "undefined" for variables that are declared but not initialized, or for object properties that don't exist so using "undefined" just adds an extra layer of checking.
There was a problem hiding this comment.
is there anything in this file that was actually changed? if no, it should not be changed.
There was a problem hiding this comment.
Im not sure why this is happening, I didn't change anything in this file. I even tried to copy paste the file from develop, but it still shows this.
| this.position = calc.position; | ||
| this.mid_lines = calc.mid_lines; | ||
| this.end_position = calc.end_position; | ||
| this.line_height = calc.line_height; |
There was a problem hiding this comment.
this exact same code is also called in textdisplay.js:
this.position = calc.position;
this.mid_lines = calc.mid_lines;
this.end_position = calc.end_position;
this.line_height = calc.line_height;
| // Force a refresh of positions for updated appellation | ||
| this.$nextTick(() => { | ||
| this.updatePosition(); | ||
| EventBus.$emit('updateposition'); |
There was a problem hiding this comment.
I'm still confused. This code:
this.updatePosition();
EventBus.$emit('updateposition');
first calls below updatePosition function, right? Then it emits an event updateposition. Won't this event be caught in textdisplay.js line 45, which will then call the updatePosition function in textdisplay.js?
|
So out here each component only updates its own position data. They don't call each other's methods. What both the updatePosition functions do:
Workflow for when a user creates a new appellation: INITIAL STATE:
FINAL STATE:
The Workflow for editing an appellation and how each function works: INITIAL STATE:
FINAL STATE:
Why TextSelectionDisplay Needs to Update:
Different updatePosition() Methods
Its not a circular loop:
|
4 participants
Guidelines for Pull Requests
If you haven't yet read our code review guidelines, please do so, You can find them here.
Please confirm the following by adding an x for each item (turn
[ ]into[x]).Please provide a brief description of your ticket
Unable to edit/delete appellations once created in Text Annotation View
There is no way to edit the created notation. One a notation is created, it is not addressed to edit / how to edit the created notation.
#46
VOGRE-54
Are there any other pull requests that this one depends on?
No
Anything else the reviewer needs to know?
... describe here ...