-
Notifications
You must be signed in to change notification settings - Fork 1
Use stela endpoints for getRecord
#669
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
Conversation
a2dd303
to
481b56a
Compare
481b56a
to
946ecd9
Compare
0548f17
to
819033a
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #669 +/- ##
==========================================
+ Coverage 45.14% 45.16% +0.02%
==========================================
Files 370 370
Lines 11256 11284 +28
Branches 1855 1860 +5
==========================================
+ Hits 5081 5096 +15
- Misses 6006 6012 +6
- Partials 169 176 +7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
819033a
to
bf2dc9d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very exciting! I was able to share a file and create a share link, and the site worked as expected, so it looks like this is working. Google Maps didn't load locally, but I suspect that's a local environment problem.
@cecilia-donnelly I'm doing some local QA and saw keywords acting funny -- going to go through with a fine-toothed-comb to see what similar "mocking" needs to be done to turn StelaRecords to RecordVos but overall the approach is still gonna be great |
a36a4c9
to
663a189
Compare
@cecilia-donnelly ready for a re-review! |
getWithChildren
and getRecord
It looks like tests may be actually failing! Taking a look now. |
This needs to be reviewed/QA'd more deeply.
46e9c89
to
de8cd2f
Compare
Fixed the tests (I had to update the test fixture response to reflect stela return format). I also fixed some leaking tests while I was in the area, and added that as another commit. |
de8cd2f
to
c817b74
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We talked about this a little bit elsewhere, but to recap here:
- Tags are not appearing specifically in the side/detail view on the file list (what you see when you single click an item in a workspace)
The other problems I saw resolved, so were likely related to queuing. I focused on QA-ing folder and record actions, so only lightly touched account, archive, and billing related aspects of the app. It's always possible that those are related in arcane ways, but seems less likely.
Here is my QA notes doc (sorry, not public): https://docs.google.com/document/d/1khJX_VNwCcES9dGHXDRwIsUXcnW06_uIuXi7L6oTrRM/edit?tab=t.0
I'll be adjusting this PR to use stela for both of these endpoints (rather than just record). I'm also going to split each migration to a separate commit. |
c817b74
to
f524c53
Compare
getWithChildren
and getRecord
getWithChildren
and getRecord
f524c53
to
b6caba4
Compare
getWithChildren
and getRecord
getRecord
b6caba4
to
bb2accf
Compare
7c48e64
to
4424885
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
QA finding on dev: a subtle confusion in the custom metadata. We can look at this together, but here's what I see:
- On a record, open the side detail view with a single click
- Under "Custom Metadata," click the "Click to add" and add a "field:value" (e.g. color:orange)
- See that the metadata is populated (yay!) but only with the "value"
- Refresh the page
- Open the "Manage Custom Metadata" modal in Archive Settings (or from the detail sidebar)
- See that the fields and values are populated correctly (also yay)
So I think everything is working fine, but the display is not showing the "field" part of the "field:value" pair. This is true in both the detail sidebar and the full page view.
Let me know if this makes sense.
4424885
to
10dd052
Compare
Okay, another tiny thing but it feels like we're getting to the end of it!
The double tag persists through refresh but it (correctly) cannot be added to the record, so far as I can tell. |
10dd052
to
c51157e
Compare
OK! I think that last bug was because stela returns tag id as a string, but PHP returns it as a number -- this means if there are interfaces that are combining some tags that were loaded by PHP endpoints + this record's tags it will treat them as two distinct tags! Added a type cast so stela tag IDs are converted to numbers. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can confirm that the last problem I found is fixed!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found a handful of IDs that are numbers here but strings in stela
67f1542
to
43c79d1
Compare
@slifty given that you gave this a pretty deep QA do you think could count as QA from a process PoV? |
43c79d1
to
d47ad15
Compare
I went through my QA list again in full in @omnignorant's absence. The only QA problem I have may or may not be related to these changes, but when trying to open a link to a public item, I am consistently redirected to the public gallery main page. To reproduce:
This might just be a dev problem with redirection in general, but it is not happening on staging (or prod). |
These are tests that were evaluating in a promise but there was no async or `done`. I did try making it async but that was causing failures, so instead I'm just using `done` here and moving on with my life.
d47ad15
to
d67a8f3
Compare
Strange... Public urls works locally (I had tested it / had to make some fixes to support it) but not in dev. I'm going to dig in more closely now to see what might be going on. |
We are beginning to migrate our new feature development to stela, which means we need to begin using those endpoints here. Eventually we may go even further and use the permanent SDK, but for now what we are doing is shimming stela's output to look as though it came from the PHP api. By mapping the stela type to the v1 type we avoid needing to refactor all of our components. Eventually we would want to refactor these components to use a more streamlined data type, but that is a larger lift than we want for now. Using stela endpoints will allow us to benefit from new authentication methods (e.g. unlisted share tokens) in the near future. Issue #683 Support unlisted shares Co-authored-by: crisnicandrei <62384997+crisnicandrei@users.noreply.github.com>
d67a8f3
to
685be13
Compare
Fixed up (the issue was that my Stela type didn't account for the fact that Time for a new round of QA! Going through the document. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @slifty !
This PR updates our
getRecord
call to use stela. This improvement is necessary to support unlisted share functionality, which only exists on stela.This PR represents a modest refactor from the one opened at #630; the primary difference is that we re-package v2 results so that the object returned is the same type as before (e.g. a
RecordResponse
vs aRecordVO
). This should prevent the need to refactor our components to have v1 and v2 versions as part of the unlisted shares improvements.Related to #683