Account created on 5 February 2008, almost 17 years ago
  • Staff Software Engineer at AcquiaΒ 
#

Merge Requests

More

Recent comments

πŸ‡ΊπŸ‡ΈUnited States traviscarden

How about ^ this, @wim leers? It takes your proposal with two small changes:

  1. Instead of testing for a specific exception class, it just checks for a ::getVerboseMessage() method and uses it if it's there.
  2. It extracts the conditional local to a helper class.
πŸ‡ΊπŸ‡ΈUnited States traviscarden

@wim leers, there were actually some pretty significant problems with our openapi.yml--most notably, schema-related specification violations. It doesn't surprise me it hasn't been doing everything we want it to. I've identified a few opportunities for improvement, but it took kind of a long time to get the existing file passing validation, so I haven't had time to actually implement them. Here's an overview:

  1. Figure out why our tests didn't catch the specification validation errors on openapi.yml. I believe that's what OpenApiSpecValidationTest is supposed to be doing.
  2. Specify all responses, including errors. We haven't been rigorous about specifying error conditions. I believe there are some ways to specify defaults or templates that could reduce boilerplate.
  3. Specify parameter minimum, maximum, and default where appropriate.
  4. Add allowEmptyValue where possible.
  5. See if there are any other primitive schema types that can be changed from free values to enum because there are a definite, limited number of valid values.
  6. Specify (serialization) style on array parameters. See Describing Parameters > Schema vs Content. I haven't looked closely at the places this would apply yet to see how much value it would actually provide.
  7. Add missing descriptions. I added stubs for them with TODO values for now.

That's what I've got for now. I don't know what you want to accomplish within this issue specifically, @wim leers. Anything that doesn't seem urgent could be moved to πŸ“Œ Lay out a strategy for OpenAPI integration Active , if you like.

πŸ‡ΊπŸ‡ΈUnited States traviscarden

traviscarden β†’ made their first commit to this issue’s fork.

πŸ‡ΊπŸ‡ΈUnited States traviscarden

Looks right to me!

πŸ‡ΊπŸ‡ΈUnited States traviscarden

Mmm. I'm not sure that got it. I've just reverted the commit and re-applied it with the correct attribution. Happy Thursday. πŸ˜„

πŸ‡ΊπŸ‡ΈUnited States traviscarden

Oops! I didn't get you credited in the commit message, @thejimbirch. I think it'll pick up on the fact that you authored it. Let's credit you in a comment, too.

πŸ‡ΊπŸ‡ΈUnited States traviscarden

Cool! Fixing a few (mostly grammar) mistakes in the UI text and merging. Thanks!

πŸ‡ΊπŸ‡ΈUnited States traviscarden

I assume this should go back to @tedbow now.

πŸ‡ΊπŸ‡ΈUnited States traviscarden

Just the usual little formatting niggle and it looks good, @tedbow.

πŸ‡ΊπŸ‡ΈUnited States traviscarden

Ooh, interesting! I was sick all last week, so I haven't had a chance to look at this. I'll try to check it out soon.

πŸ‡ΊπŸ‡ΈUnited States traviscarden

@tedbow, for some reason I can't seem to create an MR. The button's just absent from the UI. (I feel like we've had a problem with this before, and it might've been a permissions issue.) In any case, I've pushed a branch with the fix. Could you see if you're able to create an MR and then review it?

πŸ‡ΊπŸ‡ΈUnited States traviscarden

traviscarden β†’ made their first commit to this issue’s fork.

πŸ‡ΊπŸ‡ΈUnited States traviscarden

Oops. You asked me to go ahead and merge. Done. πŸ™‚

πŸ‡ΊπŸ‡ΈUnited States traviscarden

Congratulations on your first Core commit, @secretsayan! πŸ˜„

πŸ‡ΊπŸ‡ΈUnited States traviscarden

Thanks, @jessebaker; this is huge. πŸ˜‰

πŸ‡ΊπŸ‡ΈUnited States traviscarden

Nice. πŸ™‚

πŸ‡ΊπŸ‡ΈUnited States traviscarden

Fix broken links to config files.

πŸ‡ΊπŸ‡ΈUnited States traviscarden

I've marked πŸ› Canvas flickers when adding a component to an empty slot Active as a duplicate of this issue and added the screenshot and additional detail from it to this issue's summary. I don't know if they have the same root cause. If they turn out not to and can't be easily solved at the same time, we can re-open that issue.

πŸ‡ΊπŸ‡ΈUnited States traviscarden

That makes sense, @lauriii. I could certainly imagining the root cause being the same. I'll close this one as a duplicate and add the added detail to that one. Thanks.

πŸ‡ΊπŸ‡ΈUnited States traviscarden

@secretsayan asked me to push this change for him real quick because he's in the process of moving to a new laptop.

πŸ‡ΊπŸ‡ΈUnited States traviscarden

Thanks, @wim leers. I agree with your assessment. I think the original impetus of this issue was based on a misunderstanding, and the value to be derived now is just findability in the short term. I've updated the summary accordingly.

Also, like you, I assumed that adding the local task would be a matter of minutes, but I must be missing something. I can get a local task to appear if I hard-code the node ID, but I can't get it dynamically. Anything I try results in either a fatal error or just not showing the task. I'll create an MR with the hard-coded nid. I need someone to help me figure out the last part.

πŸ‡ΊπŸ‡ΈUnited States traviscarden

Thanks @omkar-pd; that fixes the presenting problem, but apparently it was supposed to be red.

πŸ‡ΊπŸ‡ΈUnited States traviscarden

I think that's intrinsic to the scope of this issue. Nodes are the presenting question (and the answer seems obvious to me). I assume most other entity types already have their own conventions--Views, blocks, media, etc. Maybe we should break this issue up a little bit: 1) keep this one but scope it at Nodes only, 2) create one for deciding on other entity types when we're getting ready to implement them, and 3) expose the ability the define the behavior in the API. What do you think?

πŸ‡ΊπŸ‡ΈUnited States traviscarden

Nice! I prettified openapi.yml and approved it.

πŸ‡ΊπŸ‡ΈUnited States traviscarden

Re: #29

🟒 Scenario 5.1: Deleting created entities when abandoning progress

There should be a way to delete the draft by using a delete action but once the user has entered the content form and inserted any content there, we should retain the saved draft.

I don't understand. I assume "saved draft" means an auto-save state. Is that right? What do you mean by "content form"?

πŸ”΅ Scenario 6.1: Deleting created entities when abandoning progress

There might be different expectation between [Media entities and taxonomy terms]. [In] the use case of Taxonomy Term because… you're just adding a tag inline using an autocomplete.

But you can also configure Taxonomy term fields to "create referenced entities if they don't already exist". The point is that it’s possible to create separate entities from a node edit screen. Does the rest of the scenario look right?

🟑 Auto-save only applies to saved entities. It doesn't make sense in any other context, e.g., an un-submitted node/add/* form, since (it is also assumed) Experience Builder can't even be opened except on an existing entity.

I don't think this is the right assumption. Content types that are using Experience Builder should open in XB even when creating content.

Okay, so when you create an XB-enabled node, for example, there is no standard node/add form at all? You go straight to Experience Builder and that's the only way to interact with the entity? If so, that should be added to the assumptions. And we probably need a new scenario for editing the overall entity information--such as title, URL alias, and menu settings--from within Entity Builder. Would you agree?

🟑 If a user wants to revert back to a specific point in history, that will be handled with revisions.

It should be possible to revert back to previous revisions and auto-save states. It would be okay if auto-save states get truncated into a single revision once content gets published.

Wouldn't that mean that the "truncated" (meaning, squashed or merged, I assume) auto-save states are always identical to the list of revisions--making them effectively useless? Wouldn't that mean that every revision would have an identical auto-save state?

🟑 What happens if the same user has the same node open in Experience Builder at the same time on two different computers? For example, what if a user goes back and forth between home and the office? Does one computer get locked out of editing and become "read only"? How do we avoid the two computers repeatedly overwriting each other's auto-saves?

We could do something similar to: https://www.youtube.com/watch?v=CsRBypaT_hM&t=27s. We could apply this both for the scenarios where the user is the same and where it's different.

Given the workflow depicted in the video, I suppose that would mean a user on computer two would have to request control from computer one. And ignoring the logical challenges with that, it would mean that our auto-save states would no longer be associated with a user but rather with a browser session, right? Otherwise the concept of locking doesn't make any sense and we haven't solved the problem in question at all.

🟑 What happens to one user's auto-save states if a node or revision is changed by another user?

Now that I understand that only one user can be working on a single entity at a time, problem can't actually materialize, then as long as starting an editing session in Experience Builder locks the whole entity, this is irrelevant.

πŸ‡ΊπŸ‡ΈUnited States traviscarden

"Wounded." Haha. No, it was actually dead. As I understand @tedbow per a conversation we had outside this issue, it was removed on purpose and put back in by accident during a merge or rebase. Trust us, @wim leers : we don't make mistakes around here. At least not when we're correcting our mistakes. πŸ˜‰

πŸ‡ΊπŸ‡ΈUnited States traviscarden

A lot of the details in this issue (that assumption included) are pending confirmation from @lauriii. See the legend for interpreting the status markers.

Granting our assumptions, it seems like we're more or less in agreement on the proposed resolution at this point. (Tell me if I'm wrong.) I'll see if I can get @lauriii's eyes on the issue, and in the meantime, I'll find a good place in the codebase to document our architectural direction. Since the scope of this issue is research, that should resolve it, and we can create follow-ups to start on implementation.

πŸ‡ΊπŸ‡ΈUnited States traviscarden

The wording of these user stories assumes that the entity has already been explicitly saved (i.e. exists as an entity). autosave_form also handles the case where nothing has been explicitly saved ... I don't know if that is an XB requirement but it would be good to explicitly rule it in or out, currently it's not mentioned.

Good thinking, @catch. I assume it is not a requirement since, as far as I know, there's no way to use Experience Builder except on a piece of content that already exists.

I've updated the issue summary to include this assumption.

πŸ‡ΊπŸ‡ΈUnited States traviscarden

Is that a question, @wim leers? πŸ™‚

πŸ‡ΊπŸ‡ΈUnited States traviscarden

Okay, my updates to the issue summary are complete. I think I've captured more-or-less everything. Assigning to @lauriii for input on the unconfirmed requirement details.

@lauriii, we've documented our current understanding of the requirements in the issue summary. We would like your confirmation so we can confidently finalize our architectural decisions. Every detail we've identified is marked with a colored circle indicating its status (confirmed or otherwise). (See the legend.) Basically, anything that isn't green needs confirmation--mostly from you.

Incidentally, the issue summary is now rather massive, and I could see it being extracted to its own issue for reference and discussion, if desired. I'll happily do that if asked.

πŸ‡ΊπŸ‡ΈUnited States traviscarden

I think this was only waiting on my weigh-in. Approved. πŸ™‚ I don't know who should merge it.

πŸ‡ΊπŸ‡ΈUnited States traviscarden

...and again to updated the proposed resolution. I'll leave it assigned to me to continue fleshing out the distinct user stories for drafts and publishing.

πŸ‡ΊπŸ‡ΈUnited States traviscarden

Updating the issue summary to reflect the elimination of Content Moderation as an option per #28.

πŸ‡ΊπŸ‡ΈUnited States traviscarden

Oops. I omitted the commit credit on the merge commit. Reverting and redoing correctly. (I don't know why that appears in reverse above, but it is correct now.) And now...

πŸ‘ Fixed!

πŸ‡ΊπŸ‡ΈUnited States traviscarden

Okay, I've fleshed out the requirements for the Auto-save feature, and this is on track to become the longest issue summary in history. 😬 (I blame d.o's CSS for the odd markup choices I made to make it more readable.) I plan to do the same for each of the other two features and then distill the discussion in the comments. Also, it's past 1:00am my time and too late for proofreading. Here's hoping for the best. πŸ˜›

πŸ‡ΊπŸ‡ΈUnited States traviscarden

I'm going to try to process all the details here and update the issue summary.

πŸ‡ΊπŸ‡ΈUnited States traviscarden

Oh, interesting. I haven't done this before either, but it looks right based on the documentation you pointed to. Fixed.

πŸ‡ΊπŸ‡ΈUnited States traviscarden

Nice! Merged. Good work, guys; this is exciting. πŸ™‚

πŸ‡ΊπŸ‡ΈUnited States traviscarden

traviscarden β†’ made their first commit to this issue’s fork.

πŸ‡ΊπŸ‡ΈUnited States traviscarden

I've been trying to get access to merge this MR all day, and despite apparently having all the necessary permissions, I still cannot. So if someone who can merge it kindly would, that would be great. You have my approval to do so.

πŸ‡ΊπŸ‡ΈUnited States traviscarden

Nice. This should immediately improve the experience for everyone.

I don't seem to have the ability to actually merge the MR, so I'll mark this RTBC until I can get it.

πŸ‡ΊπŸ‡ΈUnited States traviscarden

I can RTBC this as soon as the (trivial) requested change is made.

πŸ‡ΊπŸ‡ΈUnited States traviscarden

Re: #66

4. Blocker: Evaluate dev dependencies Β· Issue #78 Β· php-tuf/composer-stager

For this issue, the release managers think that most if not all of the remaining dev dependencies could also be removed. The original issue summary is out of date (e.g., Infection has already been removed). A contributor could help by updating this issue to reflect the current status, so that release and framework managers can make final decisions about which dev dependencies to remove.

I've updated the issue summary there.

πŸ‡ΊπŸ‡ΈUnited States traviscarden

I assume this is related: πŸ› String props that are integer values aren't treated as strings Postponed

Yes. That issue has been marked as a duplicate of this one.

I've manually tested again and confirmed that, of course, it still works. A quick review of the JS code (not my area of expertise) should wrap this up.

πŸ‡ΊπŸ‡ΈUnited States traviscarden

Thanks, @phenaproxima! Just to document the fact: you and I had a conversation offline in which I invited you to make this proposal.

I like this a lot at the conceptual level. I'm not equipped to weigh in on every detail of the proposed implementation yet. Can you comment on how your proposal might fit into 🌱 [META] Roadmap Active ?

Note: I've created a 3.0.x branch for this.

πŸ‡ΊπŸ‡ΈUnited States traviscarden

Actually, the fieldable use case may actually be out of scope for that issue. Let's call it a dependency instead.

πŸ‡ΊπŸ‡ΈUnited States traviscarden

I pushed some code that seems to work locally. I can't run Cypress tests right now for some reason; we'll see what happens on CI. The code probably isn't very elegant, but red => green => refactor, you know. πŸ™‚

πŸ‡ΊπŸ‡ΈUnited States traviscarden

Update: I'm on track with a solution, but there's an ugly merge conflict with 0.x in xb-general.cy.js that I'm trying to rebase away.

Production build 0.71.5 2024